e2e: use helm to install out-of-tree cloud-provider-azure #2209

jackfrancis · 2022-03-31T01:04:50Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR updates the "external" (or out-of-tree) cloud-provider-azure templates and test implementation so that we use the official helm chart (see this PR: kubernetes-sigs/cloud-provider-azure#1306).

This approach has the following advantages:

rather than define static versions of cloud-provider-azure to install, we rely upon the helm chart to choose the right version for the version of Kubernetes running in the cluster
no longer rely upon ClusterResourceSet CRD, which is alpha and may not be enabled on your cluster-api mgmt cluster

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

replace ClusterResourceSet with helm for external cloud-provider-azure templates

jackfrancis · 2022-03-31T02:09:41Z

/test pull-cluster-api-provider-azure-e2e-optional

jsturtevant

My concern with this is we now have two ways to install cluster addons on until the new proposal lands and goes forward. This just adds some complexity from maintenace and possible confusion. We are also suggesting using helm for most out of the box clusters. Is that what we desire? I don't have any strong objections just pointing it out.

We also probably need to add docs on how to do this post cluster creation.

jsturtevant · 2022-03-31T15:48:51Z

test/e2e/azure_test.go

@@ -450,6 +450,10 @@ var _ = Describe("Workload cluster creation", func() {
 				WaitForClusterIntervals:      e2eConfig.GetIntervals(specName, "wait-cluster"),
 				WaitForControlPlaneIntervals: e2eConfig.GetIntervals(specName, "wait-control-plane"),
 				WaitForMachinePools:          e2eConfig.GetIntervals(specName, "wait-machine-pool-nodes"),
+				ControlPlaneWaiters: clusterctl.ControlPlaneWaiters{


cool, didn't know about this!

test/e2e/cloud-provider-azure.go

templates/cluster-template-external-cloud-provider.yaml

test/e2e/cloud-provider-azure.go

test/e2e/azure_test.go

jackfrancis · 2022-04-01T20:14:36Z

/test pull-cluster-api-provider-azure-e2e-optional

docs/book/src/topics/cloud-provider-config.md

jsturtevant · 2022-04-01T20:25:48Z

test/e2e/cloud-provider-azure.go

+		if err != nil {
+			return err
+		}
+		if len(n.Items) == (int(*input.ConfigCluster.WorkerMachineCount) + int(*input.ConfigCluster.ControlPlaneMachineCount)) {


This test doesn't run for windows currently, but if it did we would need to include WINDOWS_WORKER_MACHINE_COUNT here.

Actually this brings up a good question. On this test, for example, this WorkerMachineCount value seems to apply to each node pool (not total number of expected worker nodes):

https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/v1.2.1/test/e2e/azure_test.go#L216

And indeed, this value seems to be used to populate the named env var that kustomize uses to configure a single value which by convention is shared across all MachinePools/MachineDeployments:

https://github.com/kubernetes-sigs/cluster-api/blob/v1.1.3/cmd/clusterctl/client/config.go#L436

This check is really here to know that we're ready to reliably perform a helm install against the cluster. We can simply check for 1 Running node to do that.

This will work for a Windows-enabled cluster.

Do we want to just convert the reference "external-cloud-provider" template to have a Windows MachineDeployment as well? @CecileRobertMichon for thoughts as well

IMO we should not change the existing external cloud provider template in this PR but instead change all the reference templates to use external cloud provider as we had started doing in #1889 in a follow PR. That will include various windows flavors.

Makes sense

templates/cluster-template-external-cloud-provider.yaml

jackfrancis · 2022-04-02T01:42:19Z

/test pull-cluster-api-provider-azure-e2e-optional

jackfrancis · 2022-04-04T17:27:20Z

test/e2e/cloud-provider-azure.go

+		p := helmGetter.All(settings)
+		valueOpts := &helmVals.Options{}
+		valueOpts.Values = []string{fmt.Sprintf("infra.clusterName=%s", input.ClusterProxy.GetName())}
+		if imageRegistryFromEnv := os.Getenv("IMAGE_REGISTRY"); imageRegistryFromEnv != "" {


@CecileRobertMichon @jsturtevant the below is a proposed solution to ensuring these changes are back-compat with upstream cloud-provider-azure test-infra jobs that rely upon capz to test out-of-tree. This is how the current tests currently work:

TEST_CCM=true is set when calling scripts/ci-entrypoint.sh:

https://github.com/kubernetes/test-infra/blob/f74bc266fbbff04f30769fbfcd736634c90d3b5f/config/jobs/kubernetes-sigs/cloud-provider-azure/cloud-provider-azure-config.yaml#L185

capz maintains a script to set the CI-built image data here:

cluster-api-provider-azure/scripts/ci-entrypoint.sh

Line 91 in 1352fa2

source "${REPO_ROOT}/scripts/ci-build-azure-ccm.sh"

cluster-api-provider-azure/scripts/ci-build-azure-ccm.sh

Line 43 in 1352fa2

export IMAGE_REGISTRY=${REGISTRY}

cluster-api-provider-azure/scripts/ci-build-azure-ccm.sh

Lines 34 to 36 in 1352fa2

declare CCM_IMAGE_NAME=azure-cloud-controller-manager

# cloud node manager image

declare CNM_IMAGE_NAME=azure-cloud-node-manager

cluster-api-provider-azure/scripts/ci-build-azure-ccm.sh

Line 44 in 1352fa2

pushd "${AZURE_CLOUD_PROVIDER_ROOT}" && IMAGE_TAG=$(git rev-parse --short=7 HEAD) && export IMAGE_TAG && popd

These env vars aren't as specifically named as we'd ideally want, but re-using them as-is allows us to reduce the change surface area (don't have to update capz CI scripts and/or test-infra job defs).

the out of tree cloud provider PR tests and conformance tests don't run CAPZ e2e so they won't call InstallCloudProviderAzureHelmChart as far as I know. We may need to 1) modify ci-entrypoint.sh directly to do the helm install for out of tree cases, and 2) change conformance_test.go to use this custom control plane waiter too when testing out of tree

/cc @lzhecheng

I don't think we have to worry about the conformance stuff yet (in this PR), it seems the only template flavors that it currently uses are default and windows (not the external templates).

When we eventually get to the part of this effort where we're testing out-of-tree by default, then yeah, the conformance ControlPlaneWaiters will need to get updated as well.

jackfrancis · 2022-04-04T17:27:53Z

/test pull-cluster-api-provider-azure-e2e-optional

jackfrancis · 2022-04-04T21:28:06Z

/test pull-cluster-api-provider-azure-e2e-optional

jackfrancis · 2022-04-04T23:08:45Z

scripts/ci-entrypoint.sh

@@ -152,6 +154,11 @@ create_cluster
 # export the target cluster KUBECONFIG if not already set
 export KUBECONFIG="${KUBECONFIG:-${PWD}/kubeconfig}"

+# install cloud-provider-azure components, if using out-of-tree


@CecileRobertMichon @lzhecheng incorporated the helm stuff into the ci-entrypoint.sh script itself. The way this will work is basically the same way I implemented this in the Tiltfile (wait for kubeconfig secret, wait for successful get nodes). Reference:

cluster-api-provider-azure/Makefile

Lines 283 to 287 in 1352fa2

# Wait for the kubeconfig to become available.

timeout --foreground 300 bash -c "while ! kubectl get secrets | grep $(CLUSTER_NAME)-kubeconfig; do sleep 1; done"

# Get kubeconfig and store it locally.

kubectl get secrets $(CLUSTER_NAME)-kubeconfig -o json | jq -r .data.value | base64 --decode > ./kubeconfig

timeout --foreground 600 bash -c "while ! kubectl --kubeconfig=./kubeconfig get nodes | grep control-plane; do sleep 1; done"

jackfrancis · 2022-04-04T23:09:05Z

/test pull-cluster-api-provider-azure-e2e-optional

CecileRobertMichon · 2022-04-08T21:57:01Z

test/e2e/cloud-provider-azure.go

+			Namespace: input.ConfigCluster.Namespace,
+		}
+		return client.Get(ctx, key, secret)
+	}, 20*time.Minute, 5*time.Second).Should(Succeed())


should this be using a config interval variable here instead of hardcoding to 20 minutes? same question for intervals below

reusing the WaitForControlPlaneIntervals var everywhere here

arguably you can tune these down per retryable operations, but it's debatable whether or not it's worth the effort

test/e2e/cloud-provider-azure.go

jackfrancis · 2022-04-08T22:42:05Z

/retest

jackfrancis · 2022-04-08T22:42:56Z

/test pull-cluster-api-provider-azure-e2e-optional

jackfrancis · 2022-04-10T17:21:27Z

/retest

CecileRobertMichon · 2022-04-11T17:10:32Z

/lgtm
/assign @jsturtevant

CecileRobertMichon · 2022-04-11T17:10:44Z

/hold cancel

CecileRobertMichon · 2022-04-14T21:24:11Z

/assign @mboersma

James is OOF this week

mboersma

Just a few questions, but this looks good.

mboersma · 2022-04-14T22:41:49Z

Tiltfile

@@ -331,9 +332,13 @@ def deploy_worker_templates(template, substitutions):
        yaml = yaml.replace("${" + substitution + "}", value)

    yaml = yaml.replace('"', '\\"')  # add escape character to double quotes in yaml
+    flavor_name = os.path.basename(flavor)
+    flavor_cmd = "RANDOM=$(bash -c 'echo $RANDOM'); CLUSTER_NAME=" + flavor.replace("windows", "win") + "-$RANDOM; make generate-flavors; echo \"" + yaml + "\" > ./.tiltbuild/" + flavor + "; cat ./.tiltbuild/" + flavor + " | " + envsubst_cmd + " | " + kubectl_cmd + " apply -f - && echo \"Cluster \'$CLUSTER_NAME\' created, don't forget to delete\""


I wanted to suggest this would be better as an f-string, but then I remembered this is Starlark.

mboersma · 2022-04-14T22:42:04Z

Makefile

@@ -236,6 +241,9 @@ verify-tiltfile: ## Verify Tiltfile format.

 ##@ Development:

+.PHONY: install-tools # populate hack/tools/bin
+install-tools: $(ENVSUBST) $(KUSTOMIZE) $(KUBECTL) $(HELM) $(GINKGO)


I'm also mildly opposed to this change if it adds any noticeable time to testing targets. Could we maybe have the new target install just the tools that are actually shared, or is this still a maintenance headache?

install-tools: $(ENVSUBST) $(KUSTOMIZE) $(KUBECTL) test-e2e-run: generate-e2e-templates install-tools $(GINKGO)

docs/book/src/developers/kubernetes-developers.md

jackfrancis · 2022-04-14T23:26:37Z

/test pull-cluster-api-provider-azure-e2e-optional

mboersma

/lgtm

CecileRobertMichon · 2022-04-15T16:27:19Z

/lgtm
/approve

k8s-ci-robot · 2022-04-15T16:27:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [CecileRobertMichon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from alexeldeib and shysank March 31, 2022 01:05

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 31, 2022

jsturtevant reviewed Mar 31, 2022

View reviewed changes

CecileRobertMichon reviewed Mar 31, 2022

View reviewed changes

test/e2e/azure_test.go Outdated Show resolved Hide resolved

jackfrancis force-pushed the e2e-external-cloud-provider-helm branch 2 times, most recently from fa8fd21 to 7313554 Compare April 1, 2022 20:05

jsturtevant reviewed Apr 1, 2022

View reviewed changes

jsturtevant mentioned this pull request Apr 1, 2022

Templates and E2E tests use external cloud-provider-azure by default #1994

Closed

3 tasks

jackfrancis force-pushed the e2e-external-cloud-provider-helm branch 3 times, most recently from ca7138b to 1fc368f Compare April 2, 2022 00:34

jackfrancis force-pushed the e2e-external-cloud-provider-helm branch from 1fc368f to 60d5a04 Compare April 4, 2022 17:17

jackfrancis commented Apr 4, 2022

View reviewed changes

jackfrancis force-pushed the e2e-external-cloud-provider-helm branch from 60d5a04 to 20b2855 Compare April 4, 2022 21:18

k8s-ci-robot requested a review from lzhecheng April 4, 2022 22:40

jackfrancis force-pushed the e2e-external-cloud-provider-helm branch from 20b2855 to fb2b32e Compare April 4, 2022 23:02

jackfrancis commented Apr 4, 2022

View reviewed changes

jackfrancis force-pushed the e2e-external-cloud-provider-helm branch from fb2b32e to bb81770 Compare April 4, 2022 23:19

CecileRobertMichon reviewed Apr 8, 2022

View reviewed changes

test/e2e/cloud-provider-azure.go Outdated Show resolved Hide resolved

CecileRobertMichon reviewed Apr 8, 2022

View reviewed changes

test/e2e/cloud-provider-azure.go Outdated Show resolved Hide resolved

jackfrancis force-pushed the e2e-external-cloud-provider-helm branch from 90026ff to 2527091 Compare April 8, 2022 22:30

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 11, 2022

CecileRobertMichon mentioned this pull request Apr 12, 2022

[release-1.2] Update make target when ccm test #2225

Merged

k8s-ci-robot assigned mboersma Apr 14, 2022

mboersma reviewed Apr 14, 2022

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 14, 2022

e2e: use helm to install out-of-tree cloud-provider-azure

f8c166d

jackfrancis force-pushed the e2e-external-cloud-provider-helm branch from 7251cc7 to f8c166d Compare April 14, 2022 23:15

mboersma approved these changes Apr 15, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 15, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2022

k8s-ci-robot merged commit b06af49 into kubernetes-sigs:main Apr 15, 2022

k8s-ci-robot added this to the v1.3 milestone Apr 15, 2022

jackfrancis deleted the e2e-external-cloud-provider-helm branch April 15, 2022 17:19

CecileRobertMichon mentioned this pull request Apr 22, 2022

e2e test for k8s v1.22 --> v1.23 upgrade to use out of tree csi driver #2242

Closed

jackfrancis mentioned this pull request Apr 25, 2022

generalize helm install during E2E testing #2264

Merged

3 tasks

CecileRobertMichon mentioned this pull request Jul 7, 2022

Fix Cluster labels in OOT cloud provider templates #2462

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e: use helm to install out-of-tree cloud-provider-azure #2209

e2e: use helm to install out-of-tree cloud-provider-azure #2209

jackfrancis commented Mar 31, 2022 •

edited

Loading

jackfrancis commented Mar 31, 2022

jsturtevant left a comment

jsturtevant Mar 31, 2022

jackfrancis commented Apr 1, 2022

jsturtevant Apr 1, 2022

jackfrancis Apr 1, 2022

CecileRobertMichon Apr 4, 2022

jackfrancis Apr 4, 2022

jackfrancis commented Apr 2, 2022

jackfrancis Apr 4, 2022

CecileRobertMichon Apr 4, 2022

CecileRobertMichon Apr 4, 2022

jackfrancis Apr 4, 2022

jackfrancis commented Apr 4, 2022

jackfrancis commented Apr 4, 2022

jackfrancis Apr 4, 2022

jackfrancis commented Apr 4, 2022

CecileRobertMichon Apr 8, 2022

jackfrancis Apr 8, 2022

jackfrancis commented Apr 8, 2022

jackfrancis commented Apr 8, 2022

jackfrancis commented Apr 10, 2022

CecileRobertMichon commented Apr 11, 2022

CecileRobertMichon commented Apr 11, 2022

CecileRobertMichon commented Apr 14, 2022

mboersma left a comment

mboersma Apr 14, 2022

mboersma Apr 14, 2022

jackfrancis commented Apr 14, 2022

mboersma left a comment

CecileRobertMichon commented Apr 15, 2022

k8s-ci-robot commented Apr 15, 2022

	declare CCM_IMAGE_NAME=azure-cloud-controller-manager
	# cloud node manager image
	declare CNM_IMAGE_NAME=azure-cloud-node-manager

	# Wait for the kubeconfig to become available.
	timeout --foreground 300 bash -c "while ! kubectl get secrets \| grep $(CLUSTER_NAME)-kubeconfig; do sleep 1; done"
	# Get kubeconfig and store it locally.
	kubectl get secrets $(CLUSTER_NAME)-kubeconfig -o json \| jq -r .data.value \| base64 --decode > ./kubeconfig
	timeout --foreground 600 bash -c "while ! kubectl --kubeconfig=./kubeconfig get nodes \| grep control-plane; do sleep 1; done"

e2e: use helm to install out-of-tree cloud-provider-azure #2209

e2e: use helm to install out-of-tree cloud-provider-azure #2209

Conversation

jackfrancis commented Mar 31, 2022 • edited Loading

jackfrancis commented Mar 31, 2022

jsturtevant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Apr 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Apr 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Apr 4, 2022

jackfrancis commented Apr 4, 2022

Choose a reason for hiding this comment

jackfrancis commented Apr 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Apr 8, 2022

jackfrancis commented Apr 8, 2022

jackfrancis commented Apr 10, 2022

CecileRobertMichon commented Apr 11, 2022

CecileRobertMichon commented Apr 11, 2022

CecileRobertMichon commented Apr 14, 2022

mboersma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Apr 14, 2022

mboersma left a comment

Choose a reason for hiding this comment

CecileRobertMichon commented Apr 15, 2022

k8s-ci-robot commented Apr 15, 2022

jackfrancis commented Mar 31, 2022 •

edited

Loading