From 0aa7b09a7e662a19edab1dd6c4ab78f535cf22ff Mon Sep 17 00:00:00 2001 From: Kimmo Lehto Date: Mon, 2 Dec 2024 15:20:34 +0200 Subject: [PATCH] Fix installFlags change detection for --enable-worker flag (#803) * Use "true" as value for "--enable-worker" Signed-off-by: Kimmo Lehto * Add a test Signed-off-by: Kimmo Lehto * Same for --no-taints Signed-off-by: Kimmo Lehto * The --enable-worker also adds --kubelet-extra-args=--node-ip=x Signed-off-by: Kimmo Lehto * More robust --kubelet-extra-args comparison, drop --env and --force Signed-off-by: Kimmo Lehto * Unquote for good measure Signed-off-by: Kimmo Lehto * Make reinstall smoke test check that there is no reinstall when no changes Signed-off-by: Kimmo Lehto * Make the controller in reinstall-smoke a controller+worker Signed-off-by: Kimmo Lehto * Further validate when a reinstall happens Signed-off-by: Kimmo Lehto * Ignore --data-dir --token-file and --config in flag check Signed-off-by: Kimmo Lehto * Test empty NewFlags Signed-off-by: Kimmo Lehto --------- Signed-off-by: Kimmo Lehto --- .../v1beta1/cluster/flags.go | 17 ++++++++ .../v1beta1/cluster/flags_test.go | 13 ++++++ .../v1beta1/cluster/host.go | 43 ++++++++++++++----- .../v1beta1/cluster/host_test.go | 12 +++--- smoke-test/k0sctl-installflags.yaml.tpl | 2 +- smoke-test/smoke-reinstall.sh | 20 ++++++++- 6 files changed, 88 insertions(+), 19 deletions(-) diff --git a/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/flags.go b/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/flags.go index 984696037..1c8f805e2 100644 --- 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 index f522fd34a..6da863efd 100644 --- 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 index 3459fde8f..8efa709cd 100644 --- 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 index dad613c4c..7a8700242 100644 --- 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 index f5a9d476e..2ca805752 100644 --- 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 index 7ca2c1646..0451a5552 100755 --- 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}"