From 95bc6ecbb0bdd09379b87e22a242a2ff34667e2e Mon Sep 17 00:00:00 2001 From: Bryce Soghigian Date: Tue, 13 Aug 2024 15:05:08 -0700 Subject: [PATCH 1/4] fix: removing --keep-terminated-pod-volumes from kubelet flags --- .github/workflows/ci-test.yml | 2 +- .../imagefamily/bootstrap/aksbootstrap.go | 7 +- pkg/providers/instancetype/suite_test.go | 81 ++++++++++++------- 3 files changed, 57 insertions(+), 33 deletions(-) diff --git a/.github/workflows/ci-test.yml b/.github/workflows/ci-test.yml index 9d95e867a..48acdbd50 100644 --- a/.github/workflows/ci-test.yml +++ b/.github/workflows/ci-test.yml @@ -14,7 +14,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - k8sVersion: ["1.25.x", "1.26.x", "1.27.x", "1.28.x", "1.29.x", "1.30.x"] + k8sVersion: ["1.25.x", "1.26.x", "1.27.x", "1.28.x", "1.29.x", "1.30.x", "1.31.x"] env: K8S_VERSION: ${{ matrix.k8sVersion }} steps: diff --git a/pkg/providers/imagefamily/bootstrap/aksbootstrap.go b/pkg/providers/imagefamily/bootstrap/aksbootstrap.go index c064a8f7b..9260bf516 100644 --- a/pkg/providers/imagefamily/bootstrap/aksbootstrap.go +++ b/pkg/providers/imagefamily/bootstrap/aksbootstrap.go @@ -241,6 +241,7 @@ var ( // removed --image-pull-progress-deadline=30m (not in 1.24?) // removed --network-plugin=cni (not in 1.24?) // removed --azure-container-registry-config (not in 1.30) + // removed --keep-terminated-pod-volumes (not in 1.31) kubeletFlagsBase = map[string]string{ "--address": "0.0.0.0", "--anonymous-auth": "false", @@ -257,7 +258,6 @@ var ( "--eviction-hard": "memory.available<750Mi,nodefs.available<10%,nodefs.inodesFree<5%", "--image-gc-high-threshold": "85", "--image-gc-low-threshold": "80", - "--keep-terminated-pod-volumes": "false", "--kubeconfig": "/var/lib/kubelet/kubeconfig", "--max-pods": "110", "--node-status-update-frequency": "10s", @@ -486,6 +486,11 @@ func (a AKS) applyOptions(nbv *NodeBootstrapVariables) { }), ",") // Assign Per K8s version kubelet flags + minorVersion := semver.MustParse(a.KubernetesVersion).Minor + if minorVersion > 31 { + kubeletFlagsBase["--keep-terminated-pod-volumes"] = "false" + } + credentialProviderURL := CredentialProviderURL(a.KubernetesVersion, a.Arch) if credentialProviderURL != "" { // use OOT credential provider nbv.CredentialProviderDownloadURL = credentialProviderURL diff --git a/pkg/providers/instancetype/suite_test.go b/pkg/providers/instancetype/suite_test.go index 6e03d42de..fdd1b5e7b 100644 --- a/pkg/providers/instancetype/suite_test.go +++ b/pkg/providers/instancetype/suite_test.go @@ -27,6 +27,7 @@ import ( "testing" "time" + "github.com/blang/semver/v4" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/samber/lo" @@ -1091,41 +1092,59 @@ var _ = Describe("InstanceType Provider", func() { }) }) - Context("Bootstrap", func() { - It("should gate kubelet flags that are dependent on kubelet version", func() { - ExpectApplied(ctx, env.Client, nodePool, nodeClass) - pod := coretest.UnschedulablePod() - ExpectProvisioned(ctx, env.Client, cluster, cloudProvider, coreProvisioner, pod) - ExpectScheduled(ctx, env.Client, pod) +Context("Bootstrap", func() { + var ( + kubeletFlags string + decodedString string + minorVersion uint64 + credentialProviderURL string + ) + BeforeEach(func() { + ExpectApplied(ctx, env.Client, nodePool, nodeClass) + pod := coretest.UnschedulablePod() + ExpectProvisioned(ctx, env.Client, cluster, cloudProvider, coreProvisioner, pod) + ExpectScheduled(ctx, env.Client, pod) + + Expect(azureEnv.VirtualMachinesAPI.VirtualMachineCreateOrUpdateBehavior.CalledWithInput.Len()).To(Equal(1)) + vm := azureEnv.VirtualMachinesAPI.VirtualMachineCreateOrUpdateBehavior.CalledWithInput.Pop().VM + customData := *vm.Properties.OSProfile.CustomData + Expect(customData).ToNot(BeNil()) + decodedBytes, err := base64.StdEncoding.DecodeString(customData) + Expect(err).To(Succeed()) + decodedString := string(decodedBytes[:]) + kubeletFlags = decodedString[strings.Index(decodedString, "KUBELET_FLAGS=")+len("KUBELET_FLAGS="):] + + k8sVersion, err := azureEnv.ImageProvider.KubeServerVersion(ctx) + Expect(err).To(BeNil()) + minorVersion = semver.MustParse(k8sVersion).Minor + credentialProviderURL = bootstrap.CredentialProviderURL(k8sVersion, "amd64") + }) - Expect(azureEnv.VirtualMachinesAPI.VirtualMachineCreateOrUpdateBehavior.CalledWithInput.Len()).To(Equal(1)) - vm := azureEnv.VirtualMachinesAPI.VirtualMachineCreateOrUpdateBehavior.CalledWithInput.Pop().VM - customData := *vm.Properties.OSProfile.CustomData - Expect(customData).ToNot(BeNil()) - decodedBytes, err := base64.StdEncoding.DecodeString(customData) - Expect(err).To(Succeed()) - decodedString := string(decodedBytes[:]) - Expect(decodedString).To(ContainSubstring("CREDENTIAL_PROVIDER_DOWNLOAD_URL")) - kubeletFlags := decodedString[strings.Index(decodedString, "KUBELET_FLAGS=")+len("KUBELET_FLAGS="):] + It("should include or exclude --keep-terminated-pod-volumes based on kubelet version", func() { + if minorVersion < 31 { + Expect(kubeletFlags).To(ContainSubstring("--keep-terminated-pod-volumes")) + } else { + Expect(kubeletFlags).ToNot(ContainSubstring("--keep-terminated-pod-volumes")) + } + }) - // TODO: (bsoghigian) leverage the helpers from the azure cni pr once they get in instead for testing kubelet flags - // NOTE: env.Version may differ from the version we get for the apiserver - k8sVersion, err := azureEnv.ImageProvider.KubeServerVersion(ctx) - Expect(err).To(BeNil()) - crendetialProviderURL := bootstrap.CredentialProviderURL(k8sVersion, "amd64") - if crendetialProviderURL != "" { - Expect(kubeletFlags).ToNot(ContainSubstring("--azure-container-registry-config")) - Expect(kubeletFlags).To(ContainSubstring("--image-credential-provider-config=/var/lib/kubelet/credential-provider-config.yaml")) - Expect(kubeletFlags).To(ContainSubstring("--image-credential-provider-bin-dir=/var/lib/kubelet/credential-provider")) - Expect(decodedString).To(ContainSubstring(crendetialProviderURL)) - } else { - Expect(kubeletFlags).To(ContainSubstring("--azure-container-registry-config")) - Expect(kubeletFlags).ToNot(ContainSubstring("--image-credential-provider-config")) - Expect(kubeletFlags).ToNot(ContainSubstring("--image-credential-provider-bin-dir")) - } - }) + It("should include correct flags and credential provider URL when CredentialProviderURL is not empty", func() { + if credentialProviderURL != "" { + Expect(kubeletFlags).ToNot(ContainSubstring("--azure-container-registry-config")) + Expect(kubeletFlags).To(ContainSubstring("--image-credential-provider-config=/var/lib/kubelet/credential-provider-config.yaml")) + Expect(kubeletFlags).To(ContainSubstring("--image-credential-provider-bin-dir=/var/lib/kubelet/credential-provider")) + Expect(decodedString).To(ContainSubstring(credentialProviderURL)) + } }) + It("should include correct flags when CredentialProviderURL is empty", func() { + if credentialProviderURL == "" { + Expect(kubeletFlags).To(ContainSubstring("--azure-container-registry-config")) + Expect(kubeletFlags).ToNot(ContainSubstring("--image-credential-provider-config")) + Expect(kubeletFlags).ToNot(ContainSubstring("--image-credential-provider-bin-dir")) + } + }) +}) Context("LoadBalancer", func() { resourceGroup := "test-resourceGroup" From 717bc6cd68ecb46d8138c1b25dde41a6b534be21 Mon Sep 17 00:00:00 2001 From: Bryce Soghigian Date: Tue, 13 Aug 2024 18:33:22 -0700 Subject: [PATCH 2/4] fix: test updating case to properly get the --keep-terminated-pod-volumes on older k8s versions --- .../imagefamily/bootstrap/aksbootstrap.go | 2 +- pkg/providers/instancetype/suite_test.go | 99 ++++++++++--------- 2 files changed, 51 insertions(+), 50 deletions(-) diff --git a/pkg/providers/imagefamily/bootstrap/aksbootstrap.go b/pkg/providers/imagefamily/bootstrap/aksbootstrap.go index 9260bf516..83014232b 100644 --- a/pkg/providers/imagefamily/bootstrap/aksbootstrap.go +++ b/pkg/providers/imagefamily/bootstrap/aksbootstrap.go @@ -487,7 +487,7 @@ func (a AKS) applyOptions(nbv *NodeBootstrapVariables) { // Assign Per K8s version kubelet flags minorVersion := semver.MustParse(a.KubernetesVersion).Minor - if minorVersion > 31 { + if minorVersion < 31 { kubeletFlagsBase["--keep-terminated-pod-volumes"] = "false" } diff --git a/pkg/providers/instancetype/suite_test.go b/pkg/providers/instancetype/suite_test.go index fdd1b5e7b..265ec5b15 100644 --- a/pkg/providers/instancetype/suite_test.go +++ b/pkg/providers/instancetype/suite_test.go @@ -1092,59 +1092,60 @@ var _ = Describe("InstanceType Provider", func() { }) }) -Context("Bootstrap", func() { - var ( - kubeletFlags string - decodedString string - minorVersion uint64 - credentialProviderURL string - ) - BeforeEach(func() { - ExpectApplied(ctx, env.Client, nodePool, nodeClass) - pod := coretest.UnschedulablePod() - ExpectProvisioned(ctx, env.Client, cluster, cloudProvider, coreProvisioner, pod) - ExpectScheduled(ctx, env.Client, pod) - - Expect(azureEnv.VirtualMachinesAPI.VirtualMachineCreateOrUpdateBehavior.CalledWithInput.Len()).To(Equal(1)) - vm := azureEnv.VirtualMachinesAPI.VirtualMachineCreateOrUpdateBehavior.CalledWithInput.Pop().VM - customData := *vm.Properties.OSProfile.CustomData - Expect(customData).ToNot(BeNil()) - decodedBytes, err := base64.StdEncoding.DecodeString(customData) - Expect(err).To(Succeed()) - decodedString := string(decodedBytes[:]) - kubeletFlags = decodedString[strings.Index(decodedString, "KUBELET_FLAGS=")+len("KUBELET_FLAGS="):] - - k8sVersion, err := azureEnv.ImageProvider.KubeServerVersion(ctx) - Expect(err).To(BeNil()) - minorVersion = semver.MustParse(k8sVersion).Minor - credentialProviderURL = bootstrap.CredentialProviderURL(k8sVersion, "amd64") - }) + Context("Bootstrap", func() { + var ( + kubeletFlags string + decodedString string + minorVersion uint64 + credentialProviderURL string + ) + BeforeEach(func() { + ExpectApplied(ctx, env.Client, nodePool, nodeClass) + pod := coretest.UnschedulablePod() + ExpectProvisioned(ctx, env.Client, cluster, cloudProvider, coreProvisioner, pod) + ExpectScheduled(ctx, env.Client, pod) - It("should include or exclude --keep-terminated-pod-volumes based on kubelet version", func() { - if minorVersion < 31 { - Expect(kubeletFlags).To(ContainSubstring("--keep-terminated-pod-volumes")) - } else { - Expect(kubeletFlags).ToNot(ContainSubstring("--keep-terminated-pod-volumes")) - } - }) + Expect(azureEnv.VirtualMachinesAPI.VirtualMachineCreateOrUpdateBehavior.CalledWithInput.Len()).To(Equal(1)) + vm := azureEnv.VirtualMachinesAPI.VirtualMachineCreateOrUpdateBehavior.CalledWithInput.Pop().VM + customData := *vm.Properties.OSProfile.CustomData + Expect(customData).ToNot(BeNil()) + decodedBytes, err := base64.StdEncoding.DecodeString(customData) + Expect(err).To(Succeed()) + decodedString = string(decodedBytes[:]) + kubeletFlags = decodedString[strings.Index(decodedString, "KUBELET_FLAGS=")+len("KUBELET_FLAGS="):] - It("should include correct flags and credential provider URL when CredentialProviderURL is not empty", func() { - if credentialProviderURL != "" { - Expect(kubeletFlags).ToNot(ContainSubstring("--azure-container-registry-config")) - Expect(kubeletFlags).To(ContainSubstring("--image-credential-provider-config=/var/lib/kubelet/credential-provider-config.yaml")) - Expect(kubeletFlags).To(ContainSubstring("--image-credential-provider-bin-dir=/var/lib/kubelet/credential-provider")) - Expect(decodedString).To(ContainSubstring(credentialProviderURL)) - } - }) + k8sVersion, err := azureEnv.ImageProvider.KubeServerVersion(ctx) + Expect(err).To(BeNil()) + minorVersion = semver.MustParse(k8sVersion).Minor + credentialProviderURL = bootstrap.CredentialProviderURL(k8sVersion, "amd64") + }) + + It("should include or exclude --keep-terminated-pod-volumes based on kubelet version", func() { + fmt.Println("henlo", minorVersion) + if minorVersion < 31 { + Expect(kubeletFlags).To(ContainSubstring("--keep-terminated-pod-volumes")) + } else { + Expect(kubeletFlags).ToNot(ContainSubstring("--keep-terminated-pod-volumes")) + } + }) + + It("should include correct flags and credential provider URL when CredentialProviderURL is not empty", func() { + if credentialProviderURL != "" { + Expect(kubeletFlags).ToNot(ContainSubstring("--azure-container-registry-config")) + Expect(kubeletFlags).To(ContainSubstring("--image-credential-provider-config=/var/lib/kubelet/credential-provider-config.yaml")) + Expect(kubeletFlags).To(ContainSubstring("--image-credential-provider-bin-dir=/var/lib/kubelet/credential-provider")) + Expect(decodedString).To(ContainSubstring(credentialProviderURL)) + } + }) - It("should include correct flags when CredentialProviderURL is empty", func() { - if credentialProviderURL == "" { - Expect(kubeletFlags).To(ContainSubstring("--azure-container-registry-config")) - Expect(kubeletFlags).ToNot(ContainSubstring("--image-credential-provider-config")) - Expect(kubeletFlags).ToNot(ContainSubstring("--image-credential-provider-bin-dir")) - } + It("should include correct flags when CredentialProviderURL is empty", func() { + if credentialProviderURL == "" { + Expect(kubeletFlags).To(ContainSubstring("--azure-container-registry-config")) + Expect(kubeletFlags).ToNot(ContainSubstring("--image-credential-provider-config")) + Expect(kubeletFlags).ToNot(ContainSubstring("--image-credential-provider-bin-dir")) + } + }) }) -}) Context("LoadBalancer", func() { resourceGroup := "test-resourceGroup" From 2d16f2b0ff2b6d93b76ed8cb4e4aeda839fbf66c Mon Sep 17 00:00:00 2001 From: Bryce Soghigian Date: Tue, 13 Aug 2024 18:35:29 -0700 Subject: [PATCH 3/4] fix: removing print statement --- pkg/providers/instancetype/suite_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/providers/instancetype/suite_test.go b/pkg/providers/instancetype/suite_test.go index 265ec5b15..05570b78e 100644 --- a/pkg/providers/instancetype/suite_test.go +++ b/pkg/providers/instancetype/suite_test.go @@ -1121,7 +1121,6 @@ var _ = Describe("InstanceType Provider", func() { }) It("should include or exclude --keep-terminated-pod-volumes based on kubelet version", func() { - fmt.Println("henlo", minorVersion) if minorVersion < 31 { Expect(kubeletFlags).To(ContainSubstring("--keep-terminated-pod-volumes")) } else { From 5db7cede2dd3b97f519f0dab4cb47bf161cec428 Mon Sep 17 00:00:00 2001 From: Bryce Soghigian Date: Tue, 13 Aug 2024 19:07:53 -0700 Subject: [PATCH 4/4] test: upgrading controller-runtime/tools/envtest to leverage the new envtest 1.31 binaries --- hack/toolchain.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hack/toolchain.sh b/hack/toolchain.sh index b28f2c9f7..0d8138308 100755 --- a/hack/toolchain.sh +++ b/hack/toolchain.sh @@ -15,7 +15,7 @@ tools() { go install github.com/google/ko@v0.15.2 go install github.com/mikefarah/yq/v4@v4.43.1 go install github.com/norwoodj/helm-docs/cmd/helm-docs@v1.13.1 - go install sigs.k8s.io/controller-runtime/tools/setup-envtest@v0.0.0-20240409134613-20f3f4bed925 + go install sigs.k8s.io/controller-runtime/tools/setup-envtest@0c7827e417acc15f29e7c4bfccede809d372676a go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.14.0 go install github.com/sigstore/cosign/v2/cmd/cosign@v2.2.4 # go install -tags extended github.com/gohugoio/hugo@v0.110.0