Fix installFlags change detection for --enable-worker flag (#803)

* Use "true" as value for "--enable-worker" Signed-off-by: Kimmo Lehto <klehto@mirantis.com> * Add a test Signed-off-by: Kimmo Lehto <klehto@mirantis.com> * Same for --no-taints Signed-off-by: Kimmo Lehto <klehto@mirantis.com> * The --enable-worker also adds --kubelet-extra-args=--node-ip=x Signed-off-by: Kimmo Lehto <klehto@mirantis.com> * More robust --kubelet-extra-args comparison, drop --env and --force Signed-off-by: Kimmo Lehto <klehto@mirantis.com> * Unquote for good measure Signed-off-by: Kimmo Lehto <klehto@mirantis.com> * Make reinstall smoke test check that there is no reinstall when no changes Signed-off-by: Kimmo Lehto <klehto@mirantis.com> * Make the controller in reinstall-smoke a controller+worker Signed-off-by: Kimmo Lehto <klehto@mirantis.com> * Further validate when a reinstall happens Signed-off-by: Kimmo Lehto <klehto@mirantis.com> * Ignore --data-dir --token-file and --config in flag check Signed-off-by: Kimmo Lehto <klehto@mirantis.com> * Test empty NewFlags Signed-off-by: Kimmo Lehto <klehto@mirantis.com> --------- Signed-off-by: Kimmo Lehto <klehto@mirantis.com>
k0sproject · Dec 2, 2024 · 0aa7b09 · 0aa7b09
1 parent ae1932d
commit 0aa7b09
Show file tree

Hide file tree

Showing 6 changed files with 88 additions and 19 deletions.
diff --git a/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/flags.go b/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/flags.go
@@ -211,3 +211,20 @@ func (f Flags) Equals(b Flags) bool {
 	}
 	return true
 }
+
+// NewFlags shell-splits and parses a string and returns new Flags or an error if splitting fails
+func NewFlags(s string) (Flags, error) {
+	var flags Flags
+	unq, err := shell.Unquote(s)
+	if err != nil {
+		return flags, fmt.Errorf("failed to unquote flags %q: %w", s, err)
+	}
+	parts, err := shell.Split(unq)
+	if err != nil {
+		return flags, fmt.Errorf("failed to split flags %q: %w", s, err)
+	}
+	for _, part := range parts {
+		flags.Add(part)
+	}
+	return flags, nil
+}
diff --git a/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/flags_test.go b/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/flags_test.go
@@ -135,3 +135,16 @@ func TestEquals(t *testing.T) {
 	flags2 = Flags{"-f", "--flag2=foo", "--flag3=baz"}
 	require.False(t, flags1.Equals(flags2))
 }
+
+func TestNewFlags(t *testing.T) {
+	t.Run("basic", func(t *testing.T) {
+		flags, err := NewFlags("--hello=world --bar=baz")
+		require.NoError(t, err)
+		require.Equal(t, "world", flags.GetValue("--hello"))
+		require.Equal(t, "baz", flags.GetValue("--bar"))
+	})
+	t.Run("empty", func(t *testing.T) {
+		_, err := NewFlags("")
+		require.NoError(t, err)
+	})
+}
diff --git a/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/host.go b/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/host.go
@@ -13,7 +13,6 @@ import (
 	"github.com/go-playground/validator/v10"
 	"github.com/jellydator/validation"
 	"github.com/jellydator/validation/is"
-	"github.com/k0sproject/k0sctl/internal/shell"
 	"github.com/k0sproject/rig"
 	"github.com/k0sproject/rig/exec"
 	"github.com/k0sproject/rig/os"
@@ -289,9 +288,9 @@ func (h *Host) K0sInstallFlags() (Flags, error) {
 
 	switch h.Role {
 	case "controller+worker":
-		flags.AddUnlessExist("--enable-worker")
+		flags.AddUnlessExist("--enable-worker=true")
 		if h.NoTaints {
-			flags.AddUnlessExist("--no-taints")
+			flags.AddUnlessExist("--no-taints=true")
 		}
 	case "single":
 		flags.AddUnlessExist("--single=true")
@@ -308,13 +307,11 @@ func (h *Host) K0sInstallFlags() (Flags, error) {
 	if strings.HasSuffix(h.Role, "worker") {
 		var extra Flags
 		if old := flags.GetValue("--kubelet-extra-args"); old != "" {
-			parts, err := shell.Split(old)
+			ex, err := NewFlags(old)
 			if err != nil {
 				return flags, fmt.Errorf("failed to split kubelet-extra-args: %w", err)
 			}
-			for _, part := range parts {
-				extra.Add(part)
-			}
+			extra = ex
 		}
 		// set worker's private address to --node-ip in --extra-kubelet-args if cloud ins't enabled
 		enableCloudProvider, err := h.InstallFlags.GetBoolean("--enable-cloud-provider")
@@ -581,17 +578,41 @@ func (h *Host) ExpandTokens(input string, k0sVersion *version.Version) string {
 
 // FlagsChanged returns true when the flags have changed by comparing the host.Metadata.K0sStatusArgs to what host.InstallFlags would produce
 func (h *Host) FlagsChanged() bool {
-	installFlags, err := h.K0sInstallFlags()
+	our, err := h.K0sInstallFlags()
 	if err != nil {
 		log.Warnf("%s: could not get install flags: %s", h, err)
-		installFlags = Flags{}
+		our = Flags{}
+	}
+	ex := our.GetValue("--kubelet-extra-args")
+	ourExtra, err := NewFlags(ex)
+	if err != nil {
+		log.Warnf("%s: could not parse local --kubelet-extra-args value %q: %s", h, ex, err)
+	}
+
+	var their Flags
+	their = append(their, h.Metadata.K0sStatusArgs...)
+	ex = their.GetValue("--kubelet-extra-args")
+	theirExtra, err := NewFlags(ex)
+	if err != nil {
+		log.Warnf("%s: could not parse remote --kubelet-extra-args value %q: %s", h, ex, err)
+	}
+
+	if !ourExtra.Equals(theirExtra) {
+		log.Debugf("%s: installFlags --kubelet-extra-args seem to have changed: %+v vs %+v", h, theirExtra.Map(), ourExtra.Map())
+		return true
+	}
+
+	// remove flags that are dropped by k0s or are handled specially
+	for _, f := range []string{"--force", "--kubelet-extra-args", "--env", "--data-dir", "--token-file", "--config"} {
+		our.Delete(f)
+		their.Delete(f)
 	}
 
-	if installFlags.Equals(h.Metadata.K0sStatusArgs) {
+	if our.Equals(their) {
 		log.Debugf("%s: installFlags have not changed", h)
 		return false
 	}
 
-	log.Debugf("%s: installFlags seem to have changed. existing: %+v new: %+v", h, h.Metadata.K0sStatusArgs.Map(), installFlags.Map())
+	log.Debugf("%s: installFlags seem to have changed. existing: %+v new: %+v", h, their.Map(), our.Map())
 	return true
 }
diff --git a/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/host_test.go b/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/host_test.go
@@ -90,12 +90,12 @@ func TestK0sInstallCommand(t *testing.T) {
 	h.Metadata.IsK0sLeader = true
 	cmd, err = h.K0sInstallCommand()
 	require.NoError(t, err)
-	require.Equal(t, `k0s install controller --data-dir=/tmp/k0s --enable-worker --config=from-configurer`, cmd)
+	require.Equal(t, `k0s install controller --data-dir=/tmp/k0s --enable-worker=true --config=from-configurer`, cmd)
 
 	h.Metadata.IsK0sLeader = false
 	cmd, err = h.K0sInstallCommand()
 	require.NoError(t, err)
-	require.Equal(t, `k0s install controller --data-dir=/tmp/k0s --enable-worker --token-file=from-configurer --config=from-configurer`, cmd)
+	require.Equal(t, `k0s install controller --data-dir=/tmp/k0s --enable-worker=true --token-file=from-configurer --config=from-configurer`, cmd)
 
 	h.Role = "worker"
 	h.PrivateAddress = "10.0.0.9"
@@ -177,14 +177,16 @@ func TestFlagsChanged(t *testing.T) {
 		h := Host{
 			Configurer:     cfg,
 			DataDir:        "/tmp/data",
-			Role:           "controller",
+			Role:           "controller+worker",
 			PrivateAddress: "10.0.0.1",
 			InstallFlags:   []string{"--foo='bar'", "--bar=foo"},
 			Metadata: HostMetadata{
-				K0sStatusArgs: []string{"--foo=bar", `--bar="foo"`, "--data-dir=/tmp/data", "--token-file=/tmp/token", "--config=/tmp/foo.yaml"},
+				K0sStatusArgs: []string{"--foo=bar", `--bar="foo"`, "--enable-worker=true", "--data-dir=/tmp/data", "--token-file=/tmp/token", "--config=/tmp/foo.yaml", "--kubelet-extra-args=--node-ip=10.0.0.1"},
 			},
 		}
-		require.False(t, h.FlagsChanged())
+		newFlags, err := h.K0sInstallFlags()
+		require.NoError(t, err)
+		require.False(t, h.FlagsChanged(), "flags %+v should not be considered different from %+v", newFlags, h.Metadata.K0sStatusArgs)
 		h.InstallFlags = []string{"--foo=bar", `--bar="foo"`}
 		require.False(t, h.FlagsChanged())
 		h.InstallFlags = []string{"--foo=baz", `--bar="foo"`}

diff --git a/smoke-test/k0sctl-installflags.yaml.tpl b/smoke-test/k0sctl-installflags.yaml.tpl
@@ -2,7 +2,7 @@ apiVersion: k0sctl.k0sproject.io/v1beta1
 kind: cluster
 spec:
   hosts:
-    - role: controller
+    - role: controller+worker
       uploadBinary: true
       installFlags:
         - "${K0S_CONTROLLER_FLAG}"

diff --git a/smoke-test/smoke-reinstall.sh b/smoke-test/smoke-reinstall.sh
@@ -22,15 +22,31 @@ remoteCommand() {
 }
 
 echo "Installing ${K0S_VERSION}"
-../k0sctl apply --config "${K0SCTL_CONFIG}" --debug
+../k0sctl apply --config "${K0SCTL_CONFIG}" --debug | tee apply.log
+echo "Initial apply should not perform a re-install"
+grep -ivq "reinstalling" apply.log
+
+echo "Install flags should contain the expected flag on a controller"
 remoteCommand "root@manager0" "k0s status -o json | grep -q -- ${K0S_CONTROLLER_FLAG}"
+
+echo "Install flags should contain the expected flag on a worker"
 remoteCommand "root@worker0" "k0s status -o json | grep -q -- ${K0S_WORKER_FLAG}"
 
+echo "A re-apply should not re-install if there are no changes"
+../k0sctl apply --config "${K0SCTL_CONFIG}" --debug | tee apply.log
+grep -ivq "reinstalling" apply.log
+
 export K0S_CONTROLLER_FLAG="--labels=smoke-stage=2" 
 export K0S_WORKER_FLAG="--labels=smoke-stage=2" 
 envsubst < "k0sctl-installflags.yaml.tpl" > "${K0SCTL_CONFIG}"
 
 echo "Re-applying ${K0S_VERSION} with modified installFlags"
-../k0sctl apply --config "${K0SCTL_CONFIG}" --debug
+../k0sctl apply --config "${K0SCTL_CONFIG}" --debug | tee apply.log
+echo "A re-apply should perform a re-install if there are changes"
+grep -iq "reinstalling" apply.log
+
+echo "Install flags should change for controller"
 remoteCommand "root@manager0" "k0s status -o json | grep -q -- ${K0S_CONTROLLER_FLAG}"
+
+echo "Install flags should change for worker"
 remoteCommand "root@worker0" "k0s status -o json | grep -q -- ${K0S_WORKER_FLAG}"