From b2bc5f4b3943684da75b1a6fa5d728f6c65247e7 Mon Sep 17 00:00:00 2001 From: Bryce Soghigian <49734722+Bryce-Soghigian@users.noreply.github.com> Date: Tue, 13 Aug 2024 19:27:58 -0700 Subject: [PATCH] fix: removing --keep-terminated-pod-volumes from kubelet flags and adding k8s 1.31 to ci-test (#455) * fix: removing --keep-terminated-pod-volumes from kubelet flags * fix: test updating case to properly get the --keep-terminated-pod-volumes on older k8s versions * fix: removing print statement * test: upgrading controller-runtime/tools/envtest to leverage the new envtest 1.31 binaries --- .github/workflows/ci-test.yml | 2 +- hack/toolchain.sh | 2 +- .../imagefamily/bootstrap/aksbootstrap.go | 7 +++- pkg/providers/instancetype/suite_test.go | 41 ++++++++++++++----- 4 files changed, 38 insertions(+), 14 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/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 diff --git a/pkg/providers/imagefamily/bootstrap/aksbootstrap.go b/pkg/providers/imagefamily/bootstrap/aksbootstrap.go index c064a8f7b..83014232b 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..05570b78e 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" @@ -1092,7 +1093,13 @@ var _ = Describe("InstanceType Provider", func() { }) Context("Bootstrap", func() { - It("should gate kubelet flags that are dependent on kubelet version", 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) @@ -1104,28 +1111,40 @@ var _ = Describe("InstanceType Provider", func() { 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="):] + decodedString = string(decodedBytes[:]) + kubeletFlags = decodedString[strings.Index(decodedString, "KUBELET_FLAGS=")+len("KUBELET_FLAGS="):] - // 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 != "" { + minorVersion = semver.MustParse(k8sVersion).Minor + credentialProviderURL = bootstrap.CredentialProviderURL(k8sVersion, "amd64") + }) + + 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")) + } + }) + + 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(crendetialProviderURL)) - } else { + 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"