handle clusterctl block-move annotation #4213

nojnhuh · 2023-11-01T03:46:07Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

This PR makes CAPZ aware of the clusterctl clusterctl.cluster.x-k8s.io/block-move annotation added by kubernetes-sigs/cluster-api#8690 and uses it to block clusterctl move when ASO resources have not yet been paused. These changes modify the necessary controllers to add the annotation at the beginning of a "normal" reconciliation and remove it at the end of a "pause" reconciliation.

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

Special notes for your reviewer:

These changes are not expected to build until #4182 or any other CAPI bump to v1.6.0+.

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

CAPZ now adds the `clusterctl.cluster.x-k8s.io/block-move` annotation during `clusterctl move` to better synchronize pause operations

k8s-ci-robot · 2023-11-01T03:46:09Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2023-12-12T04:48:40Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (703a98b) 60.88% compared to head (27d8eff) 60.97%.
Report is 39 commits behind head on main.

Files	Patch %	Lines
exp/controllers/azuremachinepool_controller.go	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4213      +/-   ##
==========================================
+ Coverage   60.88%   60.97%   +0.08%     
==========================================
  Files         190      190              
  Lines       18983    19029      +46     
==========================================
+ Hits        11557    11602      +45     
- Misses       6789     6790       +1     
  Partials      637      637

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

nojnhuh · 2023-12-12T04:55:16Z

/test ?

k8s-ci-robot · 2023-12-12T04:55:19Z

@nojnhuh: The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-azure-build
/test pull-cluster-api-provider-azure-ci-entrypoint
/test pull-cluster-api-provider-azure-e2e
/test pull-cluster-api-provider-azure-e2e-aks
/test pull-cluster-api-provider-azure-test
/test pull-cluster-api-provider-azure-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-azure-apidiff
/test pull-cluster-api-provider-azure-apiversion-upgrade
/test pull-cluster-api-provider-azure-capi-e2e
/test pull-cluster-api-provider-azure-conformance
/test pull-cluster-api-provider-azure-conformance-custom-builds
/test pull-cluster-api-provider-azure-conformance-dual-stack-with-ci-artifacts
/test pull-cluster-api-provider-azure-conformance-ipv6-with-ci-artifacts
/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
/test pull-cluster-api-provider-azure-e2e-optional
/test pull-cluster-api-provider-azure-e2e-workload-upgrade
/test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts-serial-slow
/test pull-cluster-api-provider-azure-windows-custom-builds
/test pull-cluster-api-provider-azure-windows-with-ci-artifacts

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-azure-apidiff
pull-cluster-api-provider-azure-build
pull-cluster-api-provider-azure-ci-entrypoint
pull-cluster-api-provider-azure-e2e
pull-cluster-api-provider-azure-e2e-aks
pull-cluster-api-provider-azure-test
pull-cluster-api-provider-azure-verify

In response to this:

/test ?

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.

nojnhuh · 2023-12-12T04:55:42Z

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

nojnhuh · 2023-12-12T06:56:49Z

/test pull-cluster-api-provider-azure-ci-entrypoint
/test pull-cluster-api-provider-azure-capi-e2e

nojnhuh · 2023-12-13T19:56:48Z

/assign @Jont828

willie-yao

Looks good pending one nit!

willie-yao · 2023-12-14T00:26:13Z

controllers/helpers.go

@@ -1120,3 +1121,29 @@ func GetClusterScoper(ctx context.Context, logger logr.Logger, c client.Client,

 	return nil, errors.Errorf("unsupported infrastructure type %q, should be AzureCluster or AzureManagedCluster", cluster.Spec.InfrastructureRef.Kind)
 }
+
+// AddBlockMoveAnnotation adds CAPI's block-move annotation returns whether or not the change had an effect.


Suggested change

// AddBlockMoveAnnotation adds CAPI's block-move annotation returns whether or not the change had an effect.

// AddBlockMoveAnnotation adds CAPI's block-move annotation and returns whether or not the annotation was added.

nit for more clarity

Jont828

LGTM pending Willie's comment

Jont828 · 2023-12-14T03:14:21Z

controllers/azurecluster_controller.go

+	// Register our finalizer immediately to avoid orphaning Azure resources on delete
+	needsPatch := controllerutil.AddFinalizer(azureCluster, infrav1.ClusterFinalizer)
+	// Register the block-move annotation immediately to avoid moving un-paused ASO resources
+	needsPatch = AddBlockMoveAnnotation(azureCluster) || needsPatch


Just to clarify, this just means we don't want to allow clusterctl move to happen before we're done reconciling, right?

Right, this makes sure there isn't any interval of time where both an ASO resource has reconcile-policy: manage and the AzureCluster doesn't have the block-move label, which is the danger zone.

willie-yao · 2023-12-14T17:28:16Z

/lgtm

k8s-ci-robot · 2023-12-14T17:28:23Z

LGTM label has been added.

Git tree hash: 52d8be664b66eb331ab38b7905783f2d869fa304

CecileRobertMichon

/lgtm
/approve

k8s-ci-robot · 2023-12-15T00:56:22Z

[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 added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Nov 1, 2023

k8s-ci-robot requested a review from mboersma November 1, 2023 03:46

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 1, 2023

k8s-ci-robot requested a review from willie-yao November 1, 2023 03:46

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 1, 2023

nojnhuh force-pushed the aso-block-move branch from 98cd69c to 2266f9f Compare December 12, 2023 04:35

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2023

nojnhuh marked this pull request as ready for review December 12, 2023 04:35

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 12, 2023

k8s-ci-robot requested a review from jsturtevant December 12, 2023 04:35

nojnhuh force-pushed the aso-block-move branch from 2266f9f to a224eb7 Compare December 12, 2023 06:30

k8s-ci-robot assigned Jont828 Dec 13, 2023

willie-yao reviewed Dec 14, 2023

View reviewed changes

Jont828 reviewed Dec 14, 2023

View reviewed changes

handle clusterctl block-move annotation

27d8eff

nojnhuh force-pushed the aso-block-move branch from a224eb7 to 27d8eff Compare December 14, 2023 16:34

k8s-ci-robot assigned willie-yao Dec 14, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 14, 2023

dtzar added this to the v1.13 milestone Dec 14, 2023

mboersma assigned mboersma and unassigned mboersma Dec 14, 2023

CecileRobertMichon reviewed Dec 15, 2023

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon Dec 15, 2023

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

k8s-ci-robot merged commit ea84b59 into kubernetes-sigs:main Dec 15, 2023
20 checks passed

nojnhuh deleted the aso-block-move branch December 19, 2023 16:34

nojnhuh mentioned this pull request May 3, 2024

ASOAPI: add pause handling #4803

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handle clusterctl block-move annotation #4213

handle clusterctl block-move annotation #4213

nojnhuh commented Nov 1, 2023 •

edited

Loading

k8s-ci-robot commented Nov 1, 2023

codecov bot commented Dec 12, 2023 •

edited

Loading

nojnhuh commented Dec 12, 2023

k8s-ci-robot commented Dec 12, 2023

nojnhuh commented Dec 12, 2023

nojnhuh commented Dec 12, 2023

nojnhuh commented Dec 13, 2023

willie-yao left a comment

willie-yao Dec 14, 2023

Jont828 left a comment

Jont828 Dec 14, 2023

nojnhuh Dec 14, 2023

willie-yao commented Dec 14, 2023

k8s-ci-robot commented Dec 14, 2023

CecileRobertMichon left a comment

k8s-ci-robot commented Dec 15, 2023

	// AddBlockMoveAnnotation adds CAPI's block-move annotation returns whether or not the change had an effect.
	// AddBlockMoveAnnotation adds CAPI's block-move annotation and returns whether or not the annotation was added.

handle clusterctl block-move annotation #4213

handle clusterctl block-move annotation #4213

Conversation

nojnhuh commented Nov 1, 2023 • edited Loading

k8s-ci-robot commented Nov 1, 2023

codecov bot commented Dec 12, 2023 • edited Loading

Codecov Report

nojnhuh commented Dec 12, 2023

k8s-ci-robot commented Dec 12, 2023

nojnhuh commented Dec 12, 2023

nojnhuh commented Dec 12, 2023

nojnhuh commented Dec 13, 2023

willie-yao left a comment

Choose a reason for hiding this comment

willie-yao Dec 14, 2023

Choose a reason for hiding this comment

Jont828 left a comment

Choose a reason for hiding this comment

Jont828 Dec 14, 2023

Choose a reason for hiding this comment

nojnhuh Dec 14, 2023

Choose a reason for hiding this comment

willie-yao commented Dec 14, 2023

k8s-ci-robot commented Dec 14, 2023

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 15, 2023

nojnhuh commented Nov 1, 2023 •

edited

Loading

codecov bot commented Dec 12, 2023 •

edited

Loading