✨ clusterctl alpha rollout pause/resume for MachineDeployments #4054

Arvinderpal · 2021-01-06T22:33:07Z

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

Tracking Issue:
#3439

k8s-ci-robot · 2021-01-06T22:33:15Z

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-06T22:36:13Z

@wfernandes @vincepri PTAL.
I also have the undo command ready to go but will wait until this is merged.
Thank you

wfernandes · 2021-01-08T23:24:23Z

@Arvinderpal Thanks for this PR! I'll take a look at this first thing next week 🙂

wfernandes · 2021-01-08T23:24:35Z

/ok-to-test

wfernandes

Thanks for the PR @Arvinderpal. Just some minor comments 🙂

cmd/clusterctl/client/alpha/rollout_pauser.go

cmd/clusterctl/client/alpha/rollout_pauser_test.go

cmd/clusterctl/client/alpha/rollout_resumer.go

cmd/clusterctl/client/alpha/rollout_resumer_test.go

wfernandes · 2021-01-11T18:01:47Z

cmd/clusterctl/client/rollout.go

+	clusterClient, err := c.clusterClientFactory(ClusterClientFactoryInput{Kubeconfig: options.Kubeconfig})
+	if err != nil {
+		return err
+	}
+
+	// If the option specifying the Namespace is empty, try to detect it.
+	if options.Namespace == "" {
+		currentNamespace, err := clusterClient.Proxy().CurrentNamespace()
+		if err != nil {
+			return err
+		}
+		options.Namespace = currentNamespace
+	}
+
+	if len(options.Resources) == 0 {
+		return fmt.Errorf("required resource not specified")
+	}
+	normalized := normalizeResources(options.Resources)
+	tuples, err := util.ResourceTypeAndNameArgs(normalized...)
+	if err != nil {
+		return err
+	}


This code seems duplicated in every method. I suggest refactoring it to consolidate this.

Refactored it.

wfernandes · 2021-01-11T18:05:29Z

cmd/clusterctl/client/rollout_test.go

@@ -32,7 +32,7 @@ func Test_clusterctlClient_RolloutRestart(t *testing.T) {
 		client *fakeClient
 	}
 	type args struct {
-		options RolloutRestartOptions
+		options RolloutOptions


Let's refactor this test so that we can assert on each of the rollout behaviors - pause, resume, restart.

cmd/clusterctl/cmd/rollout/pause.go

cmd/clusterctl/cmd/rollout/resume.go

cmd/clusterctl/cmd/rollout/pause.go

Arvinderpal

@wfernandes Thank you for the review. I think I have addressed all your comments.

cmd/clusterctl/client/alpha/rollout_pauser.go

cmd/clusterctl/client/alpha/rollout_pauser_test.go

Arvinderpal · 2021-01-13T18:08:34Z

cmd/clusterctl/client/rollout.go

+	clusterClient, err := c.clusterClientFactory(ClusterClientFactoryInput{Kubeconfig: options.Kubeconfig})
+	if err != nil {
+		return err
+	}
+
+	// If the option specifying the Namespace is empty, try to detect it.
+	if options.Namespace == "" {
+		currentNamespace, err := clusterClient.Proxy().CurrentNamespace()
+		if err != nil {
+			return err
+		}
+		options.Namespace = currentNamespace
+	}
+
+	if len(options.Resources) == 0 {
+		return fmt.Errorf("required resource not specified")
+	}
+	normalized := normalizeResources(options.Resources)
+	tuples, err := util.ResourceTypeAndNameArgs(normalized...)
+	if err != nil {
+		return err
+	}


Refactored it.

Arvinderpal · 2021-01-13T18:26:23Z

cmd/clusterctl/client/rollout_test.go

@@ -32,7 +32,7 @@ func Test_clusterctlClient_RolloutRestart(t *testing.T) {
 		client *fakeClient
 	}
 	type args struct {
-		options RolloutRestartOptions
+		options RolloutOptions


cmd/clusterctl/cmd/rollout/resume.go

wfernandes

Just a clarification. Thanks.

wfernandes · 2021-01-13T20:19:51Z

cmd/clusterctl/client/rollout.go

 	clusterClient, err := c.clusterClientFactory(ClusterClientFactoryInput{Kubeconfig: options.Kubeconfig})
 	if err != nil {
 		return err
 	}
+	tuples, err := getResourceTuples(clusterClient, options)
+	if err != nil {
+		return nil


Umm...is this correct? Or should this be return err? It also looks like the tests all passed having this logic in so if this is indeed meant to be return err then we should have a unit test that asserts this. Thanks!

Thanks for pointing that out! The fakeClient was not calling the individual funcs for the subcommands.
I also had to separate these tests -- for example, the RolloutResume will throw an error if the md is not paused, while the RolloutRestart will do the opposite if it is paused, so the same test cases cannot be passed to all of them.
There are additional tests in the lower level funcs that check for functional correctness. Is it okay if we keep these tests relatively simple?

yeah...that's fine. I just wanted to ensure that if I changed this to return nil and run the tests in the client pkg, then I would see a failing test for the right reason. 🙂

cmd/clusterctl/client/rollout.go

wfernandes · 2021-01-13T23:09:57Z

/lgtm
Over to @fabriziopandini or @CecileRobertMichon for approval.

Arvinderpal · 2021-01-15T13:25:48Z

Thanks @wfernandes
@fabriziopandini @CecileRobertMichon PTAL.
I have the undo command PR ready to go after this one as well.

fabriziopandini · 2021-01-19T13:33:09Z

/milestone v0.4.0

wfernandes · 2021-01-19T21:14:53Z

@Arvinderpal If you'd like to submit the PR for the undo rollout command we can temporarily put a WIP or a /hold until this gets merged and then we can rebase onto it.
Just a suggestion so you can get eyes on the other PR immediately instead of having to wait for this to get merged. 🙂

Arvinderpal · 2021-01-21T01:39:49Z

@Arvinderpal If you'd like to submit the PR for the undo rollout command we can temporarily put a WIP or a /hold until this gets merged and then we can rebase onto it.
Just a suggestion so you can get eyes on the other PR immediately instead of having to wait for this to get merged.

@wfernandes That's a good idea. I'll open an undo PR with a wip on it.

cmd/clusterctl/client/alpha/rollout_resumer.go

cmd/clusterctl/client/alpha/rollout_pauser.go

Arvinderpal

@vincepri Thanks for the review. I have incorporated your suggestions.

for MachineDeployments.

wfernandes · 2021-01-22T17:33:58Z

/lgtm

Arvinderpal · 2021-01-26T16:29:17Z

@vincepri Can we merge this?

CecileRobertMichon · 2021-01-29T00:04:25Z

cmd/clusterctl/cmd/rollout/pause.go

+)
+
+// NewCmdRolloutPause returns a Command instance for 'rollout pause' sub command
+func NewCmdRolloutPause(cfgFile string) *cobra.Command {


are you planning to add docs for the pause/resume command in a separate PR?

I actually don't see any docs for the rollout command in https://github.com/kubernetes-sigs/cluster-api/tree/master/docs/book/src/clusterctl/commands, are we tracking that in an issue?

@CecileRobertMichon There is a tracking issue and docs is in the list: #3439
If it's okay with you, I want to wait for a few more rollout subcommands to merge before opening a PR for the docs.

vincepri

/approve
/hold
/assign @CecileRobertMichon
to remove the hold if everything looks 👍

k8s-ci-robot · 2021-02-03T21:33:02Z

[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

CecileRobertMichon

/hold cancel

Arvinderpal · 2021-02-04T01:37:00Z

/retest

k8s-ci-robot · 2021-02-04T01:38:25Z

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

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff-main	`b769eab`	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 the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 6, 2021

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

Arvinderpal mentioned this pull request Jan 6, 2021

clusterctl rollout #3439

Closed

9 tasks

k8s-ci-robot requested review from fabriziopandini and JoelSpeed January 6, 2021 22:33

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 8, 2021

wfernandes reviewed Jan 11, 2021

View reviewed changes

Arvinderpal commented Jan 13, 2021

View reviewed changes

Arvinderpal force-pushed the clusterctl-alpha-rollout-pause branch from a829333 to 67d64f1 Compare January 13, 2021 18:35

wfernandes reviewed Jan 13, 2021

View reviewed changes

Arvinderpal force-pushed the clusterctl-alpha-rollout-pause branch 2 times, most recently from f0b137d to 43afe6c Compare January 13, 2021 21:30

k8s-ci-robot assigned wfernandes Jan 13, 2021

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

k8s-ci-robot added this to the v0.4.0 milestone Jan 19, 2021

vincepri reviewed Jan 21, 2021

View reviewed changes

cmd/clusterctl/client/alpha/rollout_resumer.go Outdated Show resolved Hide resolved

cmd/clusterctl/client/alpha/rollout_resumer.go Outdated Show resolved Hide resolved

cmd/clusterctl/client/alpha/rollout_pauser.go Outdated Show resolved Hide resolved

Arvinderpal mentioned this pull request Jan 22, 2021

✨ Clusterctl alpha rollout undo for MachineDeployments #4098

Merged

Arvinderpal commented Jan 22, 2021

View reviewed changes

Adds command and client for the clusterctl alpha rollout pause/resume

b769eab

for MachineDeployments.

Arvinderpal force-pushed the clusterctl-alpha-rollout-pause branch from 43afe6c to b769eab Compare January 22, 2021 14:49

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

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

CecileRobertMichon reviewed Jan 29, 2021

View reviewed changes

vincepri approved these changes Feb 3, 2021

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon Feb 3, 2021

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

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

CecileRobertMichon reviewed Feb 3, 2021

View reviewed changes

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

k8s-ci-robot merged commit 1acb1b0 into kubernetes-sigs:master Feb 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ clusterctl alpha rollout pause/resume for MachineDeployments #4054

✨ clusterctl alpha rollout pause/resume for MachineDeployments #4054

Arvinderpal commented Jan 6, 2021

k8s-ci-robot commented Jan 6, 2021

Arvinderpal commented Jan 6, 2021

wfernandes commented Jan 8, 2021

wfernandes commented Jan 8, 2021

wfernandes left a comment

wfernandes Jan 11, 2021

Arvinderpal Jan 13, 2021

wfernandes Jan 11, 2021

Arvinderpal Jan 13, 2021

Arvinderpal left a comment

Arvinderpal Jan 13, 2021

Arvinderpal Jan 13, 2021

wfernandes left a comment

wfernandes Jan 13, 2021

Arvinderpal Jan 13, 2021

wfernandes Jan 13, 2021

wfernandes commented Jan 13, 2021

Arvinderpal commented Jan 15, 2021

fabriziopandini commented Jan 19, 2021

wfernandes commented Jan 19, 2021

Arvinderpal commented Jan 21, 2021

Arvinderpal left a comment

wfernandes commented Jan 22, 2021

Arvinderpal commented Jan 26, 2021

CecileRobertMichon Jan 29, 2021

CecileRobertMichon Jan 29, 2021

Arvinderpal Jan 29, 2021

vincepri left a comment

k8s-ci-robot commented Feb 3, 2021

CecileRobertMichon left a comment

Arvinderpal commented Feb 4, 2021

k8s-ci-robot commented Feb 4, 2021 •

edited

Loading

✨ clusterctl alpha rollout pause/resume for MachineDeployments #4054

✨ clusterctl alpha rollout pause/resume for MachineDeployments #4054

Conversation

Arvinderpal commented Jan 6, 2021

k8s-ci-robot commented Jan 6, 2021

Arvinderpal commented Jan 6, 2021

wfernandes commented Jan 8, 2021

wfernandes commented Jan 8, 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

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

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

wfernandes commented Jan 13, 2021

Arvinderpal commented Jan 15, 2021

fabriziopandini commented Jan 19, 2021

wfernandes commented Jan 19, 2021

Arvinderpal commented Jan 21, 2021

Arvinderpal left a comment

Choose a reason for hiding this comment

wfernandes commented Jan 22, 2021

Arvinderpal commented Jan 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 3, 2021

CecileRobertMichon left a comment

Choose a reason for hiding this comment

Arvinderpal commented Feb 4, 2021

k8s-ci-robot commented Feb 4, 2021 • edited Loading

k8s-ci-robot commented Feb 4, 2021 •

edited

Loading