kubeadm: Inherit `dry-run` flags for each sub-phases #112945

chendave · 2022-10-10T03:45:18Z

The sub-phases like kubeadm reset phase cleanup-node which could be run independently would be able to support the dry-run mode as well.
Consistent with the sub-phases which support the dry-run mode already, such as kubeadm init phase control-plane apiserver --dry-run.
Prepare for the day when each of those sub-phases could be run independently.

Signed-off-by: Dave Chen dave.chen@arm.com

What type of PR is this?

/kind feature
/sig cluster-lifecycle

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubeadm: sub-phases are now able to support the dry-run mode, e.g. kubeadm reset phase cleanup-node --dry-run

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2022-10-10T03:45:26Z

@chendave: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

chendave · 2022-10-10T03:46:20Z

I went through all those command and sub-phases and inherit the flag for all those phases that dry-run is applicable.

chendave · 2022-10-10T03:47:19Z

/cc @neolit123 @SataQiu @pacoxu

neolit123

i think this makes sense. does it enable it for all sub phases that we have? do normal phases already have it?

one request, please start the release note with "kubeadm: ...."

/approve

k8s-ci-robot · 2022-10-10T06:56:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chendave, neolit123

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/kubeadm/OWNERS~~ [neolit123]

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

chendave · 2022-10-10T07:27:48Z

does it enable it for all sub phases that we have?

enabled all sub phases where the dry-run is applicable.

do normal phases already have it?

yes,

releasenote: kubeadm: kubeadm sub-phases now able to support the dry-run mode, e.g. kubeadm reset phase cleanup-node --dry-run

chendave · 2022-10-10T07:28:06Z

/release-note-edit: kubeadm: kubeadm sub-phases now able to support the dry-run mode, e.g. kubeadm reset phase cleanup-node --dry-run

neolit123 · 2022-10-10T07:32:16Z

just edit it manually in the op like this

kubeadm: sub-phases are now able to support the dry-run mode, e.g. kubeadm reset phase cleanup-node --dry-run

chendave · 2022-10-10T07:37:17Z

just edit it manually in the op like this

Done

pacoxu · 2022-10-10T08:07:45Z

does it enable it for all sub phases that we have?

If I understand correctly, dry-run should be a valid flag for all phase of init\ join \ reset. Can we add them all in somewhere like

kubernetes/cmd/kubeadm/app/cmd/init.go

Lines 126 to 133 in 575031b

    
           // defines additional flag that are not used by the init command but that could be eventually used 
        
           // by the sub-commands automatically generated for phases 
        
           initRunner.SetAdditionalFlags(func(flags *flag.FlagSet) { 
        
           	options.AddKubeConfigFlag(flags, &initOptions.kubeconfigPath) 
        
           	options.AddKubeConfigDirFlag(flags, &initOptions.kubeconfigDir) 
        
           	options.AddControlPlanExtraArgsFlags(flags, &initOptions.externalClusterCfg.APIServer.ExtraArgs, &initOptions.externalClusterCfg.ControllerManager.ExtraArgs, &initOptions.externalClusterCfg.Scheduler.ExtraArgs) 
        
           })

chendave · 2022-10-10T08:38:48Z

If I understand correctly, dry-run should be a valid flag for all phase of init\ join \ reset

mostly true, but I don't want the dry-run to be a flag where it's not applicable,

e.g.

kubernetes/cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go

Lines 85 to 96 in 99360e5

    
           func newUpdateStatusSubphase() workflow.Phase { 
        
           	return workflow.Phase{ 
        
           		Name: "update-status", 
        
           		Short: fmt.Sprintf( 
        
           			"Register the new control-plane node into the ClusterStatus maintained in the %s ConfigMap (DEPRECATED)", 
        
           			kubeadmconstants.KubeadmConfigConfigMap, 
        
           		), 
        
           		Run:           runUpdateStatusPhase, 
        
           		InheritFlags:  getControlPlaneJoinPhaseFlags("update-status"), 
        
           		ArgsValidator: cobra.NoArgs, 
        
           	} 
        
           }

or

kubernetes/cmd/kubeadm/app/cmd/phases/init/showjoincommand.go

Lines 71 to 78 in 99360e5

    
           func NewShowJoinCommandPhase() workflow.Phase { 
        
           	return workflow.Phase{ 
        
           		Name:         "show-join-command", 
        
           		Short:        "Show the join command for control-plane and worker node", 
        
           		Run:          showJoinCommand, 
        
           		Dependencies: []string{"bootstrap-token", "upload-certs"}, 
        
           	} 
        
           }

show the flag for those phase where it is not consumed is confusing.

/hold
I am thinking is it possible to apply the flag as a whole at least for reset.

pacoxu · 2022-10-10T10:22:39Z

I prefer to make all phase consistent. However, your concern makes sense.

I'd like to leave this to other owners to decide. Only +0.5 😄 for the consistent way.

pacoxu · 2022-10-10T10:48:27Z

I checked the code again.
If it is not included in InheritFlags, the subcommand will not show the flag. So the current change is correct.

/lgtm

neolit123 · 2022-10-10T10:52:50Z

I prefer to make all phase consistent. However, your concern makes sense.

agreed, even if a phase's dryrun is a no-op we can have the flag everywhere.

chendave · 2022-10-10T13:29:49Z

If it is not included in InheritFlags, the subcommand will not show the flag.

+1, current workflow doesn't allow us to set the flag for the sub-phases as a whole without listing all of them in the slice of InheritFlags, but I still need to update some sub-phases to inherit the flag, such as cert or upload-certs.

this is a potential chance to optimize the workflow a little bit, but this is a different thing.

I will update this change in my next day.

- The sub-phases like `kubeadm reset phase cleanup-node` which could be run independently would be able to support the `dry-run` mode as well. - Consistent with the sub-phases which support the `dry-run` mode already, such as `kubeadm init phase control-plane apiserver`. - Prepare for the day when each of those sub-phases could be run independently. Signed-off-by: Dave Chen <dave.chen@arm.com>

chendave · 2022-10-11T08:07:41Z

some update here,

inherit the dry-run flags for the phases where the fake client could be built with the dry-run mode.
inherit the dry-run flags for the phases which named as "all", in this case, the flag is also needed in order to enable the dry-run mode for all the siblings.

I think this is good to go now.

/hold cancel

@pacoxu @neolit123 looking for a LGTM label.

pacoxu · 2022-10-11T10:01:06Z

/lgtm

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubeadm labels Oct 10, 2022

k8s-ci-robot requested review from fabriziopandini and RA489 October 10, 2022 03:46

k8s-ci-robot requested review from neolit123, pacoxu and SataQiu October 10, 2022 03:47

neolit123 reviewed Oct 10, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2022

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

k8s-ci-robot assigned pacoxu Oct 10, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2022

chendave force-pushed the dry-run branch from 21ac80c to 183a26f Compare October 11, 2022 08:03

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2022

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

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2022

k8s-ci-robot merged commit 5301d92 into kubernetes:master Oct 11, 2022

k8s-ci-robot added this to the v1.26 milestone Oct 11, 2022

chendave deleted the dry-run branch October 12, 2022 01:44

This was referenced Oct 13, 2022

kubeadm: Enable dry-run mode for phase of control-plane-prepare certs #113005

Merged

Decouple kubeadm phases and make each phase idempotent kubernetes/kubeadm#2769

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Inherit `dry-run` flags for each sub-phases #112945

kubeadm: Inherit `dry-run` flags for each sub-phases #112945

chendave commented Oct 10, 2022 •

edited

k8s-ci-robot commented Oct 10, 2022

chendave commented Oct 10, 2022

chendave commented Oct 10, 2022

neolit123 left a comment •

edited

k8s-ci-robot commented Oct 10, 2022

chendave commented Oct 10, 2022

chendave commented Oct 10, 2022 •

edited

neolit123 commented Oct 10, 2022

chendave commented Oct 10, 2022

pacoxu commented Oct 10, 2022 •

edited

chendave commented Oct 10, 2022 •

edited

pacoxu commented Oct 10, 2022

pacoxu commented Oct 10, 2022 •

edited

neolit123 commented Oct 10, 2022

chendave commented Oct 10, 2022 •

edited

chendave commented Oct 11, 2022

pacoxu commented Oct 11, 2022

kubeadm: Inherit dry-run flags for each sub-phases #112945

kubeadm: Inherit dry-run flags for each sub-phases #112945

Conversation

chendave commented Oct 10, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Oct 10, 2022

chendave commented Oct 10, 2022

chendave commented Oct 10, 2022

neolit123 left a comment • edited

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 10, 2022

chendave commented Oct 10, 2022

chendave commented Oct 10, 2022 • edited

neolit123 commented Oct 10, 2022

chendave commented Oct 10, 2022

pacoxu commented Oct 10, 2022 • edited

chendave commented Oct 10, 2022 • edited

pacoxu commented Oct 10, 2022

pacoxu commented Oct 10, 2022 • edited

neolit123 commented Oct 10, 2022

chendave commented Oct 10, 2022 • edited

chendave commented Oct 11, 2022

pacoxu commented Oct 11, 2022

kubeadm: Inherit `dry-run` flags for each sub-phases #112945

kubeadm: Inherit `dry-run` flags for each sub-phases #112945

chendave commented Oct 10, 2022 •

edited

neolit123 left a comment •

edited

chendave commented Oct 10, 2022 •

edited

pacoxu commented Oct 10, 2022 •

edited

chendave commented Oct 10, 2022 •

edited

pacoxu commented Oct 10, 2022 •

edited

chendave commented Oct 10, 2022 •

edited