Improve the stability of the operator in big clusters #283
Conversation
- This adds the initPolicy of CreateSkipPackageInstall. This option will allow us to create a DB and skip the package installation. - Batch up commands to the pods for directory prep. This will make it faster to setup the pods for the create.
This hides some chatty messages from the operator log. We normally would dump out the contents of the admintools.conf. But this is suppressed, unless the devMode flag is set in the operator. Changes to the operator main.go to flow down the entire config struct (now called OperatorConfig).
ExecInPod is quite expensive both from a runtime perspective and a memory perspective. Batching up as many of those has shown that the operator is more stable on large clusters.
| var ( | ||
| execOut bytes.Buffer | ||
| execErr bytes.Buffer | ||
| ) | ||
|
|
||
| // Copying a file is simply a cat of the contents from stdin | ||
| command := []string{"sh", "-c", fmt.Sprintf("cat > %s", destFile)} | ||
| var sb strings.Builder |
There was a problem hiding this comment.
Could you explain me the following lines? Do we always expect a single command. Looks like with several commands it will crash.
There was a problem hiding this comment.
executeCmd could be empty, in which case the nil check would skip things. If there are multiple things passed in for executCmd, they just get concatenated together to form the command to run. Think of it like extra arguments for the command.
The end result will look like:
cat > destFile && cmd arg1 arg2 ...
| } | ||
| return nil | ||
| // Copy and execute the script | ||
| _, _, err = pRunner.CopyToPod(ctx, pf.name, names.ServerContainer, tmp.Name(), paths.EulaAcceptanceScript, |
There was a problem hiding this comment.
So now you copy and execute the script in a single function. Is that as you mentioned to reduce the number of "exec"?
There was a problem hiding this comment.
Yes. Before we would do a 'kubectl exec' to copy the file in. Then another 'kubectl exec' to run the command we copied in. This just combines the two.
| mkdir -p $INT_TEST_OUTPUT_DIR | ||
| OP="${INT_TEST_OUTPUT_DIR}/local-verticadb-operator.log" | ||
|
|
||
| if helm list -o json | jq --exit-status '.[] | select(.chart|test("^verticadb-operator")) | .name' |
There was a problem hiding this comment.
Why do we check this if we run the operator in our local env?
There was a problem hiding this comment.
This is just to make sure the operator isn't already deployed through a helm chart. I wasted a bunch of time because I was running the operator locally and had it deployed in helm too. This is just a check to make sure two operators aren't running at the same time.
This fixes a bug that was introduced earlier in this release (#283) when we did the podfacts refactor. We were failing to create the http conf dir in the installer reconciler if the http server is enabled. The plan is enable the http e2e tests, which would have caught this, when we use the 12.0.2 server release in the CI.
This change improves the stability of the operator for big clusters. When used to create big clusters (> 40 nodes), the number of things it had to do per pod starts adding up. Calls to 'kubectl exec' to get various state (cmds package) can take a while. There is also a lot of memory pressure when 'kubectl exec' is called frequently -- we send the command to the REST endpoint that uses gzip to compress/uncompress the input/output. The compression library, which we don't have any control over, uses a lot of memory. Go is a garbage collection language, so the memory is eventually freed, but the GC can take some time to kick in.
The approach taken in this change is to batch as many 'kubectl exec' commands as we can. This change impacts the podfacts collector, installer reconciler and create db reconciler. The end result is that there is less memory pressure and it is quicker for a reconcile iteration to get through its various state checks.
Refactor of the main.go module. Most of the command line argument handling was moved to a separate package called opcfg. This was changed because I wanted to flow down the command line arguments into the controllers so that we can log things only when in dev mode. So, we need to put the arguments in an exported struct from a new package that the controllers would have access to.
Included in this is a change to make it easier to run the operator locally. A new script was added that handles proper setup and teardown.