Use CAAPH to install cloud-provider-azure in tests #4413

CecileRobertMichon · 2023-12-19T23:50:16Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it: Switch to using https://github.com/kubernetes-sigs/cluster-api-addon-provider-helm to install cloud-provider-azure components in test templates.

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 #3736

Special notes for your reviewer:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Use CAAPH to install cloud-provider-azure in tests

CecileRobertMichon · 2023-12-19T23:51:01Z

Need to run more tests on this after the holidays but opening for a first round of review

cc @Jont828 @lzhecheng

codecov · 2023-12-20T00:00:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1c7c802) 61.96% compared to head (27ec5ec) 61.96%.
Report is 14 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4413   +/-   ##
=======================================
  Coverage   61.96%   61.96%           
=======================================
  Files         188      188           
  Lines       18768    18768           
=======================================
  Hits        11630    11630           
  Misses       6500     6500           
  Partials      638      638

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Jont828

Looking good so far!

Btw if you decide to set default placeholder for the CI artifact environment variables at the BeforeEach, I added it to azure_test.go, capi_test.go, conformance_test.go, and azure_selfhosted.go. If not, disregard the previous sentence.

Jont828 · 2023-12-20T03:30:29Z

scripts/ci-entrypoint.sh

+            export CLOUD_CONFIG=""
+            export CONFIG_SECRET_NAME="azure-cloud-provider"
+            export ENABLE_DYNAMIC_RELOADING=true
+            copy_secret || return 1


Q: What is the return 1 part for?

It was in the original install_cloud_provider_azure function to allow retrying the operation for transient failures:

# Any retryable operation in this function must return a non-zero exit code on failure so that we can # retry it using a `until install_cloud_provider_azure; do sleep 5; done` pattern; # and any statement must be idempotent so that subsequent retry attempts can make forward progress.

Since we're no longer retrying the function (it's part of setup now), I changed it to use until directly (the copy_secret function itself returns 1 on failure).

Jont828 · 2023-12-20T03:32:45Z

scripts/ci-entrypoint.sh

-        --set cloudNodeManager.imageName="${CNM_IMAGE_NAME}"
-        --set-string cloudControllerManager.imageTag="${IMAGE_TAG_CCM}"
-        --set-string cloudNodeManager.imageTag="${IMAGE_TAG_CNM}")
+        if [[ -n "${LOAD_CLOUD_CONFIG_FROM_SECRET:-}" ]]; then


Do we still want to use E2E_ARGS="-kubetest.use-ci-artifacts" as the variable configuring the CI artifacts?

should_build_ccm already encompasses E2E_ARGS="-kubetest.use-ci-artifacts" and other cases where we want to use a CI version of CCM in the context of ci-entrypoint.sh

https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/hack/util.sh#L44

Jont828 · 2023-12-20T03:34:06Z

templates/addons/cluster-api-helm/cloud-provider-azure-ci.yaml

+      imageName: ${CCM_IMAGE_NAME:-""}
+      imageRepository: ${IMAGE_REGISTRY:-""}
+      imageTag: ${IMAGE_TAG_CCM:-""}
+      logVerbosity: ${CCM_LOG_VERBOSITY:-4}


I'd need to verify this, but I'm not sure that envsubst supports setting default values when the variable isn't set (like in bash/zsh).

it does https://github.com/drone/envsubst

Jont828 · 2023-12-20T03:36:17Z

templates/addons/cluster-api-helm/cloud-provider-azure-ci.yaml

+      logVerbosity: ${CCM_LOG_VERBOSITY:-4}
+      replicas: ${CCM_COUNT:-1}
+      enableDynamicReloading: ${ENABLE_DYNAMIC_RELOADING:-false}
+    cloudNodeManager:


If it's possible to use bash if logic, an alternative would be to move the CI artifacts field behind an if statement and leave it blank if it's false.

It might be possible to use if blocks but I'm not sure we'd gain in simplicity, having both addon versions as separate HelmChartProxies would also allow testing both versions side by side in the future.

lzhecheng · 2023-12-21T07:44:50Z

So far no comments from my side. Please ping me if there's anything wrong in CI.

CecileRobertMichon · 2024-01-03T17:34:08Z

Squashed commits and removed [WIP], this is ready for review!

nojnhuh · 2024-01-03T18:36:42Z

/assign @Jont828

CecileRobertMichon · 2024-01-03T19:48:42Z

/retest

Jont828

/lgtm

Glad to see this is at the finish line!

k8s-ci-robot · 2024-01-04T01:47:37Z

LGTM label has been added.

Git tree hash: 4151c263e6d72809ab59c931999dec5c52eac3c2

nojnhuh · 2024-01-04T05:04:44Z

/test pull-cluster-api-provider-azure-e2e-optional
^ primarily for the in-tree test

templates/test/ci/cluster-template-prow-intree-cloud-provider.yaml

test/e2e/data/infrastructure-azure/v1beta1/bases/cloud-provider-azure-ci.yaml

nojnhuh · 2024-01-04T07:07:30Z

@CecileRobertMichon Could you also please fill out the rest of the PR description? Mostly for posterity.

CecileRobertMichon · 2024-01-04T16:30:00Z

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

nojnhuh · 2024-01-09T22:30:53Z

/approve
/hold for @CecileRobertMichon's question: #4413 (comment)

k8s-ci-robot · 2024-01-09T22:31:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nojnhuh

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~~ [nojnhuh]

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

Jont828 · 2024-01-09T22:53:12Z

/lgtm

nojnhuh · 2024-01-09T22:54:49Z

/hold cancel

CecileRobertMichon · 2024-01-09T22:55:11Z

/retest

apiversion test should be fixed now

nojnhuh · 2024-01-09T22:55:28Z

/hold for the upgrade test

CecileRobertMichon · 2024-01-09T23:35:06Z

the upgrade test failed 🤔

/retest

in case it's a flake, will look into it in parallel...

CecileRobertMichon · 2024-01-09T23:56:26Z

Update: it was not a flake. The release name was different than the one we were using previously which caused issues with the upgrade test. Renamed to match the previous naming in the latest commit which should fix it.

nojnhuh

/lgtm
/hold for squash in addition to the upgrade test

k8s-ci-robot · 2024-01-09T23:58:17Z

LGTM label has been added.

Git tree hash: 9a205db17a3d7f5b53ff6a67cb6696a01070b76b

Jont828 · 2024-01-09T23:59:19Z

/lgtm
/hold

nojnhuh · 2024-01-10T01:29:30Z

/retest

nojnhuh · 2024-01-10T03:12:54Z

@CecileRobertMichon tests passed, PSAYEC

(please squash at your earliest convenience)

CecileRobertMichon · 2024-01-10T18:31:12Z

/hold cancel

squashed!

nojnhuh · 2024-01-10T18:52:56Z

/lgtm

k8s-ci-robot · 2024-01-10T19:46:27Z

@CecileRobertMichon: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-azure-windows-custom-builds	`27ec5ec`	link	false	`/test pull-cluster-api-provider-azure-windows-custom-builds`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot requested review from marosset and mboersma December 19, 2023 23:50

CecileRobertMichon force-pushed the ccm-caaph branch from 862a0be to 44c4e3e Compare December 19, 2023 23:51

Jont828 reviewed Dec 20, 2023

View reviewed changes

CecileRobertMichon force-pushed the ccm-caaph branch 2 times, most recently from ad6f9d6 to 37fc796 Compare January 3, 2024 17:31

CecileRobertMichon changed the title ~~[WIP] Use CAAPH to install cloud-provider-azure in tests~~ Use CAAPH to install cloud-provider-azure in tests Jan 3, 2024

k8s-ci-robot assigned Jont828 Jan 3, 2024

Jont828 approved these changes Jan 4, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 4, 2024

nojnhuh reviewed Jan 4, 2024

View reviewed changes

templates/test/ci/cluster-template-prow-intree-cloud-provider.yaml Outdated Show resolved Hide resolved

test/e2e/data/infrastructure-azure/v1beta1/bases/cloud-provider-azure-ci.yaml Outdated Show resolved Hide resolved

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 4, 2024

k8s-ci-robot requested a review from Jont828 January 4, 2024 15:52

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 9, 2024

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

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2024

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 9, 2024

k8s-ci-robot requested review from Jont828 and nojnhuh January 9, 2024 23:55

nojnhuh reviewed Jan 9, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 9, 2024

Install Azure cloud provider Helm chart with CAAPH

27ec5ec

CecileRobertMichon force-pushed the ccm-caaph branch from 1e00fa0 to 27ec5ec Compare January 10, 2024 18:30

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

k8s-ci-robot merged commit 1ce44f1 into kubernetes-sigs:main Jan 10, 2024
25 of 29 checks passed

k8s-ci-robot added this to the v1.13 milestone Jan 10, 2024

jackfrancis mentioned this pull request Jan 31, 2024

Add Jonathan Tong to maintainers #4526

Merged

4 tasks

Use CAAPH to install cloud-provider-azure in tests #4413

Use CAAPH to install cloud-provider-azure in tests #4413

Conversation

CecileRobertMichon commented Dec 19, 2023 • edited Loading

CecileRobertMichon commented Dec 19, 2023

codecov bot commented Dec 20, 2023 • edited Loading

Codecov Report

Jont828 left a comment

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

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

Choose a reason for hiding this comment

lzhecheng commented Dec 21, 2023

CecileRobertMichon commented Jan 3, 2024

nojnhuh commented Jan 3, 2024

CecileRobertMichon commented Jan 3, 2024

Jont828 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 4, 2024

nojnhuh commented Jan 4, 2024

nojnhuh commented Jan 4, 2024

CecileRobertMichon commented Jan 4, 2024

nojnhuh commented Jan 9, 2024

k8s-ci-robot commented Jan 9, 2024

Jont828 commented Jan 9, 2024

nojnhuh commented Jan 9, 2024

CecileRobertMichon commented Jan 9, 2024

nojnhuh commented Jan 9, 2024

CecileRobertMichon commented Jan 9, 2024

CecileRobertMichon commented Jan 9, 2024

nojnhuh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 9, 2024

Jont828 commented Jan 9, 2024

nojnhuh commented Jan 10, 2024

nojnhuh commented Jan 10, 2024

CecileRobertMichon commented Jan 10, 2024

nojnhuh commented Jan 10, 2024

k8s-ci-robot commented Jan 10, 2024

CecileRobertMichon commented Dec 19, 2023 •

edited

Loading

codecov bot commented Dec 20, 2023 •

edited

Loading