✨ Clusterctl alpha rollout undo for MachineDeployments #4098

Arvinderpal · 2021-01-21T02:16:44Z

What this PR does / why we need it:
Adds command and client for the clusterctl alpha rollout undo command.

Example Usage: Rollback to earlier K version after upgrade.

Do an K version upgrade so that a new MS is created:

kubectl get md -n default test-md-0 -o json | jq '.spec.template.spec.version="v1.19.3"' | kubectl apply -f-

You should see a new MS and an old MS. MD will have revision annotation to point to new MS.
Perform undo:

clusterctl alpha rollout undo machinedeployment/test-md-0 --to-revision=XXX

Another example is to rollback to an earlier MachineTemplate. See CAPI book on how to change MachineTemplates.

Tracking Issue:
#3439

k8s-ci-robot · 2021-01-21T02:16:52Z

Hi @Arvinderpal. 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.

Arvinderpal · 2021-01-21T02:17:29Z

@wfernandes Here it is. 😄

vincepri

Seems this is building on top of #4054

Arvinderpal · 2021-01-22T14:42:13Z

Seems this is building on top of #4054

Yes, it is. @wfernandes suggested it to get the review process started. Once #4054 merges, we can rebase.

wfernandes

I haven't had the chance to actually use these features yet. But this is a first pass review. Thanks for putting in the work.

cmd/clusterctl/client/alpha/rollout_rollbacker.go

cmd/clusterctl/client/rollout_test.go

wfernandes · 2021-01-22T20:22:52Z

cmd/clusterctl/client/rollout.go

@@ -62,7 +139,7 @@ func (c *clusterctlClient) RolloutRestart(options RolloutRestartOptions) error {
 	}

 	for _, t := range tuples {
-		if err := c.alphaClient.Rollout().ObjectRestarter(clusterClient.Proxy(), t, options.Namespace); err != nil {
+		if err := c.alphaClient.Rollout().ObjectRollbacker(clusterClient.Proxy(), t, options.Namespace, options.ToRevision); err != nil {


Is there a reason we are calling this behavior "Rollback" and not "Undo" within the Rollout client? Just curious.

The only reason is that kubectl itself has this naming convention -- the command is undo but underlying implementation refers to rollback. I kept it consistent with kubectl, even though it's annoying.

Thanks for clarifying @Arvinderpal!

wfernandes · 2021-01-22T21:49:21Z

cmd/clusterctl/client/alpha/rollout_rollbacker.go

+
+		selector, err := metav1.LabelSelectorAsSelector(&d.Spec.Selector)
+		if err != nil {
+			log.V(5).Info("Skipping MachineSet, failed to get label selector from spec selector", "machineset", ms.Name)


Since this is getting the label selector from the deployment, I'm assuming that the error is meant to reflect just that.

Suggested change

log.V(5).Info("Skipping MachineSet, failed to get label selector from spec selector", "machineset", ms.Name)

log.V(5).Info("Skipping MachineSet, failed to get label selector from MachineDeployment.Spec.Selector", "MachineDeployment", d.Name)

On second thought, can this and the following selector.Empty() checks be moved outside the loop? The MachineDeployment is fixed, so should we iterate over the MachineSet items only if we have a non-empty selector.

WDYT?

I actually leveraged existing code here:

cluster-api/controllers/machinedeployment_controller.go

Line 218 in 2dd66d6

selector, err := metav1.LabelSelectorAsSelector(&d.Spec.Selector)

But I see your point, I don't see why the selector should be in the loop. I'll change it unless you see any reason otherwise.

I don't see any reason TBH. Maybe someone else may know why.
@vincepri Any thoughts?

wfernandes · 2021-02-04T21:05:58Z

@Arvinderpal Since #4054 has been merged we can rebase this and remove WIP. 🙂

Arvinderpal · 2021-02-04T22:00:08Z

@Arvinderpal Since #4054 has been merged we can rebase this and remove WIP.

@wfernandes Done. PTAL.

wfernandes · 2021-02-04T22:39:43Z

@Arvinderpal Thanks! I'll take a look tomorrow asap 🙂

wfernandes

Wasn't able to manually test this because of yak-shaving issues. 😄
But apart from the MachineDeployment labelselector being within the MachineSet loop, it lgtm

wfernandes · 2021-02-11T17:37:25Z

cmd/clusterctl/client/alpha/rollout_rollbacker.go

+	if toRevision > 0 {
+		return nil, errors.Errorf("unable to find specified revision: %v", toRevision)
+	}


I'm guessing this is added to provide a better error msg because I see that the next check would also result in returning an error.

Yes, the specific revision desired was not found. It's different from the next error which is that there is no previous MS associated with this deployment.

Arvinderpal · 2021-02-11T20:16:39Z

Wasn't able to manually test this because of yak-shaving issues.
But apart from the MachineDeployment labelselector being within the MachineSet loop, it lgtm

@wfernandes Thanks for the final review!
@vincepri Do you have time to review this or delegate to someone else?

wfernandes · 2021-02-11T20:22:04Z

/ok-to-test

vincepri · 2021-03-02T17:55:07Z

/assign @fabriziopandini

for final review

fabriziopandini

First pass, I still have to take a look at the tests

cmd/clusterctl/client/alpha/rollout_rollbacker.go

fabriziopandini · 2021-03-03T14:01:10Z

cmd/clusterctl/client/alpha/rollout_rollbacker.go

+// ObjectRollbacker will issue a rollback on the specified cluster-api resource.
+func (r *rollout) ObjectRollbacker(proxy cluster.Proxy, tuple util.ResourceTuple, namespace string, toRevision int64) error {
+	switch tuple.Resource {
+	case machineDeployment:


looking at the code base in some places we are using machineDeployment (const), in some others "machinedeployment" (string).
What about getting rid of lowercase version of Kind and use Kind instead?

Again, just followed kubectl conventions with the long-term goal to put this functionality in kubectl.
I should be using the const everywhere including tests. I will do that.

cmd/clusterctl/client/alpha/rollout_rollbacker.go

fabriziopandini · 2021-03-03T14:21:08Z

cmd/clusterctl/client/alpha/rollout_rollbacker.go

+	for idx := range machineSets.Items {
+		ms := &machineSets.Items[idx]
+
+		selector, err := metav1.LabelSelectorAsSelector(&d.Spec.Selector)


Move out of the for loop?

See discussion with Warren below. :)

cmd/clusterctl/client/alpha/rollout_rollbacker.go

fabriziopandini · 2021-03-03T14:30:44Z

cmd/clusterctl/cmd/rollout/undo.go

+	cmd := &cobra.Command{
+		Use:                   "undo RESOURCE",
+		DisableFlagsInUseLine: true,
+		Short:                 "Undo a cluster-api resource",


show we be a little bit more specific given that we are supporting only machine deployments?

I can add this to the cleanup PR since all commands will need updating.

fabriziopandini · 2021-03-03T14:38:18Z

cmd/clusterctl/client/alpha/rollout.go

@@ -30,6 +30,7 @@ type Rollout interface {
 	ObjectRestarter(cluster.Proxy, util.ResourceTuple, string) error
 	ObjectPauser(cluster.Proxy, util.ResourceTuple, string) error
 	ObjectResumer(cluster.Proxy, util.ResourceTuple, string) error
+	ObjectRollbacker(cluster.Proxy, util.ResourceTuple, string, int64) error


is it required to use int64?

Yes, the underlying mdutil.Revision() func returns a int64.

vincepri · 2021-03-03T15:25:10Z

/milestone v0.4.0

@Arvinderpal Please rebase as well, given the changes for Go 1.16 that merged yesterday

Arvinderpal

@fabriziopandini Thank you for the feedback. PTAL again.

Arvinderpal · 2021-03-05T14:14:16Z

cmd/clusterctl/client/alpha/rollout.go

@@ -30,6 +30,7 @@ type Rollout interface {
 	ObjectRestarter(cluster.Proxy, util.ResourceTuple, string) error
 	ObjectPauser(cluster.Proxy, util.ResourceTuple, string) error
 	ObjectResumer(cluster.Proxy, util.ResourceTuple, string) error
+	ObjectRollbacker(cluster.Proxy, util.ResourceTuple, string, int64) error


Yes, the underlying mdutil.Revision() func returns a int64.

cmd/clusterctl/client/alpha/rollout_rollbacker.go

Arvinderpal · 2021-03-05T14:39:46Z

cmd/clusterctl/client/alpha/rollout_rollbacker.go

+// ObjectRollbacker will issue a rollback on the specified cluster-api resource.
+func (r *rollout) ObjectRollbacker(proxy cluster.Proxy, tuple util.ResourceTuple, namespace string, toRevision int64) error {
+	switch tuple.Resource {
+	case machineDeployment:


Again, just followed kubectl conventions with the long-term goal to put this functionality in kubectl.
I should be using the const everywhere including tests. I will do that.

cmd/clusterctl/client/alpha/rollout_rollbacker.go

Arvinderpal · 2021-03-05T14:57:33Z

cmd/clusterctl/cmd/rollout/undo.go

+	cmd := &cobra.Command{
+		Use:                   "undo RESOURCE",
+		DisableFlagsInUseLine: true,
+		Short:                 "Undo a cluster-api resource",


I can add this to the cleanup PR since all commands will need updating.

Arvinderpal · 2021-03-05T14:58:55Z

cmd/clusterctl/client/alpha/rollout_rollbacker.go

+	for idx := range machineSets.Items {
+		ms := &machineSets.Items[idx]
+
+		selector, err := metav1.LabelSelectorAsSelector(&d.Spec.Selector)


See discussion with Warren below. :)

for MachineDeployment.

Arvinderpal · 2021-03-08T14:26:02Z

@fabriziopandini Thanks again for the follow up. If it's okay with you, we can merge this and address the remaining issues in: #4266

See tracking issue as well: #3439

fabriziopandini · 2021-03-09T11:08:39Z

/lgtm
considering some cleanup/refactor is going to land in follow up PRs.

Arvinderpal · 2021-03-12T14:18:31Z

@vincepri @fabriziopandini Considering there are two LGTMs, can we approve and merge this? Thank you.

vincepri · 2021-03-12T15:49:46Z

@Arvinderpal Could we open an issue for the follow-up PRs?

vincepri

/approve

k8s-ci-robot · 2021-03-12T15:50:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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:

~~cmd/clusterctl/OWNERS~~ [vincepri]

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

Arvinderpal · 2021-03-12T16:37:00Z

@Arvinderpal Could we open an issue for the follow-up PRs?

@vincepri #4266

Arvinderpal · 2021-03-12T16:37:18Z

/retest

Arvinderpal · 2021-03-12T16:43:59Z

/retest

k8s-ci-robot · 2021-03-12T16:45:31Z

@Arvinderpal: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff-main	`3cd6fa1`	link	`/test pull-cluster-api-apidiff-main`

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 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 21, 2021

k8s-ci-robot requested review from JoelSpeed and wfernandes January 21, 2021 02:16

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 21, 2021

Arvinderpal mentioned this pull request Jan 21, 2021

clusterctl rollout #3439

Closed

9 tasks

vincepri reviewed Jan 21, 2021

View reviewed changes

wfernandes reviewed Jan 22, 2021

View reviewed changes

Arvinderpal force-pushed the clusterctl-alpha-rollout-rollback branch from f3a2143 to 2a25c68 Compare January 24, 2021 02:11

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

Arvinderpal force-pushed the clusterctl-alpha-rollout-rollback branch from 2a25c68 to db1abdc Compare February 4, 2021 21:57

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 4, 2021

Arvinderpal changed the title ~~WIP: ✨ Clusterctl alpha rollout undo for MachineDeployments~~ ✨ Clusterctl alpha rollout undo for MachineDeployments Feb 4, 2021

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 Feb 4, 2021

Arvinderpal force-pushed the clusterctl-alpha-rollout-rollback branch 2 times, most recently from 00d4d4b to e99d62e Compare February 4, 2021 22:08

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

wfernandes reviewed Feb 11, 2021

View reviewed changes

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 Feb 11, 2021

k8s-ci-robot assigned fabriziopandini Mar 2, 2021

fabriziopandini reviewed Mar 3, 2021

View reviewed changes

k8s-ci-robot added this to the v0.4.0 milestone Mar 3, 2021

Arvinderpal commented Mar 5, 2021

View reviewed changes

Arvinderpal force-pushed the clusterctl-alpha-rollout-rollback branch 3 times, most recently from 3f347d2 to 9dbfb4f Compare March 6, 2021 22:54

Adds command and client for the clusterctl alpha rollout undo

3cd6fa1

for MachineDeployment.

Arvinderpal force-pushed the clusterctl-alpha-rollout-rollback branch from 9dbfb4f to 3cd6fa1 Compare March 6, 2021 23:07

Arvinderpal mentioned this pull request Mar 8, 2021

clusterctl rollout clean up #4266

Closed

3 tasks

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

vincepri approved these changes Mar 12, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2021

k8s-ci-robot merged commit abba4c9 into kubernetes-sigs:master Mar 12, 2021

	log.V(5).Info("Skipping MachineSet, failed to get label selector from spec selector", "machineset", ms.Name)
	log.V(5).Info("Skipping MachineSet, failed to get label selector from MachineDeployment.Spec.Selector", "MachineDeployment", d.Name)

✨ Clusterctl alpha rollout undo for MachineDeployments #4098

✨ Clusterctl alpha rollout undo for MachineDeployments #4098

Conversation

Arvinderpal commented Jan 21, 2021

k8s-ci-robot commented Jan 21, 2021

Arvinderpal commented Jan 21, 2021

vincepri left a comment

Choose a reason for hiding this comment

Arvinderpal commented Jan 22, 2021

wfernandes 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

wfernandes commented Feb 4, 2021

Arvinderpal commented Feb 4, 2021

wfernandes commented Feb 4, 2021

wfernandes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arvinderpal commented Feb 11, 2021

wfernandes commented Feb 11, 2021

vincepri commented Mar 2, 2021

fabriziopandini 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

vincepri commented Mar 3, 2021

Arvinderpal 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

Arvinderpal commented Mar 8, 2021

fabriziopandini commented Mar 9, 2021

Arvinderpal commented Mar 12, 2021

vincepri commented Mar 12, 2021

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 12, 2021

Arvinderpal commented Mar 12, 2021 • edited Loading

Arvinderpal commented Mar 12, 2021

Arvinderpal commented Mar 12, 2021

k8s-ci-robot commented Mar 12, 2021 • edited Loading

Arvinderpal commented Mar 12, 2021 •

edited

Loading

k8s-ci-robot commented Mar 12, 2021 •

edited

Loading