Skip system pool validation while cluster is paused/moved #2774

pkbhowmick · 2022-11-03T14:33:01Z

Signed-off-by: Pulak Kanti Bhowmick [email protected]

What type of PR is this?
/kind bug

What this PR does / why we need it:
Fix bug for cluster move operation
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 #2656

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:

Fixed bug for cluster move operation.

k8s-ci-robot · 2022-11-03T14:33:09Z

Welcome @pkbhowmick!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-azure 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-azure has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-11-03T14:33:10Z

Hi @pkbhowmick. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

jackfrancis · 2022-11-04T19:34:03Z

exp/api/v1beta1/azuremanagedmachinepool_webhook.go

@@ -267,6 +267,10 @@ func (m *AzureManagedMachinePool) validateLastSystemNodePool(cli client.Client)
 		return nil
 	}

+	if ownerCluster.Spec.Paused {


I think I'd prefer some other "clusterctl move"-specific indication that we're O.K. to delete the system node pool. Does a move operation result in an annotation or other metadata that we can detect?

Also, we should probably just implement this in the (m *AzureManagedMachinePool) ValidateDelete method itself.

Currently, I see there is no annotation is added in the move operation.
Move client library ref: https://github.com/kubernetes-sigs/cluster-api/blob/5eaf9e3c0e93d93b75e57675a7e9fb560d15ae37/cmd/clusterctl/client/cluster/mover.go#L334

If we want to implement this in ValidateDelete method, we need to get the cluster object which is already done in func (m *AzureManagedMachinePool) validateLastSystemNodePool method. I think, some code will be repeated then.

@Jont828 @fabriziopandini does either of you know if there is any marker that indicates a delete is initiated as part of a move operation more specific than the pause annotation?

currently, we don't add any markers to resources before the move.
cluster being paused is the only proxy information, but it is not 100% reliable because a cluster can be paused for any reason.

note: if required, adding a "move" marker on one or more move-hierarchies root (cluster, cluster class, crs) is something that makes sense; adding a "move" marker on all resources is a little bit more complex...

I think I agree with @CecileRobertMichon

There are going to be edge cases with this change but I think they are sufficiently "weird" that preventing them isn't more important than addressing this broken core capi functionality for AKS.

My only feedback at this point would be a unit test for this (I think TestAzureManagedMachinePool_ValidateCreate is a good copy/paste candidate for scaffolding a new UT for this func.

Thank you @pkbhowmick!

So, should I only add a new UT for this func?

Correct, in order to UT this new func, you'll need a new func (for example a new UT func TestValidateLastSystemNodePool based on existing pattern in TestAzureManagedMachinePool_ValidateCreate).

You will be doing the project an extra credit favor by adding coverage for an existing func that we don't have coverage for, thank you!

Got it! Thank you!

I think TestAzureManagedMachinePool_ValidateCreate does this already

nawazkh · 2022-12-14T00:11:50Z

Hey @pkbhowmick, how is this PR coming along?

pkbhowmick · 2022-12-18T04:53:48Z

Hi @nawazkh ,
Sorry for replying late. The current fix is working fine. @jackfrancis wants some specific indicator to identify move operation.

While deleting objects for move operation, child objects are deleted first. So, we can say that, while deleting last system pool if there is a paused Cluster object without delation time stamp set, this is clear indication for cluster move operation.

If we agree with this, then we can proceed with current fix. Moreover, I am using this fix, which works fine so far.
ref: https://github.com/kubernetes-sigs/cluster-api/blob/main/cmd/clusterctl/client/cluster/mover.go#L311

CecileRobertMichon · 2023-01-24T17:30:18Z

/assign @jackfrancis

To decide how we want to move this forward

api/v1beta1/azuremanagedmachinepool_webhook_test.go

Signed-off-by: Pulak Kanti Bhowmick <[email protected]> Add unit test for validateLastSystemNodePool method Signed-off-by: Pulak Kanti Bhowmick <[email protected]>

CecileRobertMichon · 2023-01-31T22:26:31Z

/ok-to-test

CecileRobertMichon · 2023-01-31T22:27:01Z

@jackfrancis is this ready to go (pending tests)?

jackfrancis · 2023-01-31T22:28:29Z

/lgtm
/approve

k8s-ci-robot · 2023-01-31T22:28:34Z

LGTM label has been added.

Git tree hash: 0e82085bda3f02bde656234cf981fb0e800be069

k8s-ci-robot · 2023-01-31T22:28:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis

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

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

pkbhowmick · 2023-02-01T03:00:22Z

/retest-required

pkbhowmick · 2023-02-01T03:32:08Z

/retest

CecileRobertMichon · 2023-02-01T21:25:35Z

/retest

k8s-ci-robot requested review from CecileRobertMichon and mboersma November 3, 2022 14:33

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 3, 2022

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 3, 2022

jackfrancis reviewed Nov 4, 2022

View reviewed changes

jackfrancis added this to the next milestone Dec 15, 2022

jackfrancis modified the milestones: next, v1.8 Jan 19, 2023

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

k8s-ci-robot assigned jackfrancis Jan 24, 2023

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 25, 2023

pkbhowmick force-pushed the fix-2656 branch from 58b0aaf to 8d1e3a9 Compare January 25, 2023 19:26

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

pkbhowmick changed the title ~~WIP: Skip system pool validation while cluster is paused/moved~~ Skip system pool validation while cluster is paused/moved Jan 25, 2023

pkbhowmick requested review from fabriziopandini and removed request for mboersma, CecileRobertMichon and fabriziopandini January 25, 2023 19:34

pkbhowmick requested review from jackfrancis and CecileRobertMichon and removed request for jackfrancis and CecileRobertMichon January 25, 2023 19:35

jackfrancis requested changes Jan 25, 2023

View reviewed changes

api/v1beta1/azuremanagedmachinepool_webhook_test.go Show resolved Hide resolved

jackfrancis approved these changes Jan 26, 2023

View reviewed changes

Skip system pool validation while cluster is paused/moved

e7a1c38

Signed-off-by: Pulak Kanti Bhowmick <[email protected]> Add unit test for validateLastSystemNodePool method Signed-off-by: Pulak Kanti Bhowmick <[email protected]>

pkbhowmick force-pushed the fix-2656 branch from 8d1e3a9 to e7a1c38 Compare January 27, 2023 07:13

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 31, 2023

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

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

k8s-ci-robot merged commit d253509 into kubernetes-sigs:main Feb 1, 2023

pkbhowmick mentioned this pull request Apr 14, 2023

Use clusterctlv1.DeleteForMoveAnnotation on last systempool validation #3426

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip system pool validation while cluster is paused/moved #2774

Skip system pool validation while cluster is paused/moved #2774

pkbhowmick commented Nov 3, 2022 •

edited

Loading

k8s-ci-robot commented Nov 3, 2022

k8s-ci-robot commented Nov 3, 2022

jackfrancis Nov 4, 2022

pkbhowmick Nov 26, 2022

pkbhowmick Nov 26, 2022

CecileRobertMichon Jan 14, 2023

fabriziopandini Jan 20, 2023

jackfrancis Jan 24, 2023

pkbhowmick Jan 24, 2023

jackfrancis Jan 24, 2023

pkbhowmick Jan 24, 2023

jackfrancis Jan 25, 2023

nawazkh commented Dec 14, 2022

pkbhowmick commented Dec 18, 2022 •

edited

Loading

CecileRobertMichon commented Jan 24, 2023

CecileRobertMichon commented Jan 31, 2023

CecileRobertMichon commented Jan 31, 2023

jackfrancis commented Jan 31, 2023

k8s-ci-robot commented Jan 31, 2023

k8s-ci-robot commented Jan 31, 2023

pkbhowmick commented Feb 1, 2023

pkbhowmick commented Feb 1, 2023

CecileRobertMichon commented Feb 1, 2023

Skip system pool validation while cluster is paused/moved #2774

Skip system pool validation while cluster is paused/moved #2774

Conversation

pkbhowmick commented Nov 3, 2022 • edited Loading

k8s-ci-robot commented Nov 3, 2022

k8s-ci-robot commented Nov 3, 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

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

nawazkh commented Dec 14, 2022

pkbhowmick commented Dec 18, 2022 • edited Loading

CecileRobertMichon commented Jan 24, 2023

CecileRobertMichon commented Jan 31, 2023

CecileRobertMichon commented Jan 31, 2023

jackfrancis commented Jan 31, 2023

k8s-ci-robot commented Jan 31, 2023

k8s-ci-robot commented Jan 31, 2023

pkbhowmick commented Feb 1, 2023

pkbhowmick commented Feb 1, 2023

CecileRobertMichon commented Feb 1, 2023

pkbhowmick commented Nov 3, 2022 •

edited

Loading

pkbhowmick commented Dec 18, 2022 •

edited

Loading