Lokomotive: New worker pools cannot be added with TLS bootstrap enabled

[invidian]

Currently, adding new worker pools to clusters with TLS bootstrapping enabled does not work, as the execution flow goes like:

1. Terraform creates new worker pool.
2. We verify that expected number of nodes is ready.
3. We update tls-boostrapping Helm chart to include new bootstrapping token.

Step 2. will always fail, as step 3. should be applied before it.

#

Currently, we call verifyCluster() before we upgrade controlplane components. This means, that if one for example uninstalls Calico helm chart, nodes will become NotReady and lokoctl cluster apply won't be able to recover it, unless you re-install Calico helm chart manually from assets directory.

The same applies for TLS bootstrapping tokens. If they get removed and user tries to add new node, cluster won't be able to recover anymore.

I suggest we move verification step after we installed the upgrades, so the process is more robust. The downside is that this will make updates being applied on possibly not healthy cluster, though I don't see a major risk in that, as applying cluster upgrades should at least fix some issues with the cluster.

Following patch can be used:

diff --git a/cli/cmd/cluster-apply.go b/cli/cmd/cluster-apply.go
index 7aed312e..0fadcea7 100644
--- a/cli/cmd/cluster-apply.go
+++ b/cli/cmd/cluster-apply.go
@@ -86,10 +86,6 @@ func runClusterApply(cmd *cobra.Command, args []string) {
                ctxLogger.Fatalf("Failed to get kubeconfig: %v", err)
        }

-       if err := verifyCluster(kubeconfig, p.Meta().ExpectedNodes); err != nil {
-               ctxLogger.Fatalf("Verify cluster: %v", err)
-       }
-
        // Update all the pre installed namespaces with lokomotive specific label.
        // `lokomotive.kinvolk.io/name: <namespace_name>`.
        if err := updateInstalledNamespaces(kubeconfig); err != nil {
@@ -121,6 +117,10 @@ func runClusterApply(cmd *cobra.Command, args []string) {
                }
        }

+       if err := verifyCluster(kubeconfig, p.Meta().ExpectedNodes); err != nil {
+               ctxLogger.Fatalf("Verify cluster: %v", err)
+       }
+
        if ph, ok := p.(platform.PlatformWithPostApplyHook); ok {
                if err := ph.PostApplyHook(kubeconfig); err != nil {
                        ctxLogger.Fatalf("Running platform post install hook failed: %v", err)

To decide:

• Do we want to block upgrades on unhealthy clusters, so user is forced to repair the cluster themselves before rolling an update?
• As we do not differentiate (yet) between making cluster aligned with the configuration and performing upgrades, do we want to try doing it before we verify that the cluster is healthy, to possible fix some issues with the cluster?

It seems that all controlplane components are required for TLS bootstrapping, so splitting the update process doesn't make much sense.

[surajssd]

Is all worker nodes up == healthy cluster? Should we just check the health of controllers first, then apply the control plane charts, then check the health of workers? Can we break that functionality into two instead of putting all into one?

[invidian]

Is all worker nodes up == healthy cluster? Should we just check the health of controllers first, then apply the control plane charts, then check the health of workers? Can we break that functionality into two instead of putting all into one?

Yes, that make sense, but it seems to me that goes a bit beyond bug-fixing and introduces new feature. Also checking if the control plane components are healthy is not trivial at the moment, but it is something which would definitely be worth implementing. Something like:

• check if API is reachable (easy)
• check if self-hosted kube-apiserver is running
• check if some kube-scheduler and kube-controller-manager are running (e.g. via lease check)
• check if self-hosted kube-scheduler and kube-controller-manager are running
• check if self-hosted kubelets are running?

[surajssd]

I am not recommending doing a full blown checks right now but just the usual breakdown into two.

• See if all the controllers are up
• Apply charts
• See if all the workers are up

[invidian]

See if all the controllers are up

What would be the point of checking if the controllers are up? And by up you mean that Node objects on nodes labeled with controller labels are in Ready state?

Perhaps with #312 solved, it would make much more sense to apply existing release on the cluster, then ensure that everything is right and then perform an upgrade.

[surajssd]

For someone hitting this issue the manual resolution is to run the following commands:

cd $ASSETS_DIR/cluster-assets/charts/kube-system/
helm upgrade -n kube-system bootstrap-secrets --values=bootstrap-secrets.yaml bootstrap-secrets

[invidian]

Okay, it seems it is actually possible to ensure that current deployment is aligned by running the following command:

$ helm rollback flatcar-linux-update-operator --wait $(helm history flatcar-linux-update-operator | tail -n1 | awk '{print $1}')

It is sort of idempotent, as it creates a new rollback release, but other than creating new Secret object with the release, it shouldn't have major side-effects. Also the releases history is limited to 10 by default, so if you would run lokoctl cluster apply, you would lose the information about the previous releases, but we don't really care much about them anyway.

[invidian]

Hmm, if one uninstalls a controlplane chart AND removes their assets directory, then recovering from that state is problematic.