clusterctl - Allow for disabling pivoting of cluster #463

detiber · 2018-07-27T19:38:10Z

What this PR does / why we need it:

Add --no-pivot-cluster flag to disable pivoting of the cluster

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

Release note:

- clusterctl adds the --no-pivot-cluster flag to 'clusterctl create cluster' that allows for disabling pivoting of cluster-api components.

ingvagabund

One blocker wrt. cleanupExternalCluster set to false when pivotCluster is false. Otherwise, just questions to more clarify semantics of the code.

ingvagabund · 2018-08-01T11:00:32Z

clusterctl/clusterdeployer/clusterdeployer.go

@@ -131,7 +134,7 @@ func (d *ClusterDeployer) Create(cluster *clusterv1.Cluster, machines []*cluster
 		return fmt.Errorf("unable to apply cluster api stack to external cluster: %v", err)
 	}

-	glog.Info("Provisioning internal cluster via external cluster")
+	glog.Info("Provisioning cluster via external cluster")


Would it make more sense to say "Provisioning cluster via external (bootstrapping) cluster"? It may be unclear to actually distinguish between what is external cluster (in this case cluster responsible for bootstrapping process) and what is internal one (the product of the bootstrapping process).

And in general to find better terms for the external/internal clusters.

ingvagabund · 2018-08-01T11:21:16Z

clusterctl/clusterdeployer/clusterdeployer.go

-	}
+	if d.pivotCluster {
+		glog.Info("Creating internal cluster")
+		internalClient, err := d.createInternalCluster(externalClient, provider, kubeconfigOutput)


createInternalCluster actually does not create the internal cluster, right? It just builds the internal cluster client. So maybe s/createInternalCluster/createInternalClusterClient/? The createInternalCluster pulls the cluster objects, waits until the internal cluster can provide its kubeconfig and then builds the client (please prove me wrong).

ingvagabund · 2018-08-01T12:02:25Z

clusterctl/clusterdeployer/clusterdeployer.go

-	glog.Info("Creating node machines in internal cluster.")
-	if err := internalClient.CreateMachineObjects(nodes); err != nil {
+	glog.Infof("Creating node machines in %s cluster.", targetCluster)
+	if err := targetClient.CreateMachineObjects(nodes); err != nil {
 		return fmt.Errorf("unable to create node machines: %v", err)
 	}

 	if d.addonComponents != "" {


What is purpose of the addon components? Do they extend the cluster API stack? Checking:

https://github.com/kubernetes-sigs/cluster-api-provider-gcp/blob/master/clusterctl/examples/google/addons.yaml.template

https://github.com/roberthbailey/cluster-api-provider-vsphere/blob/master/clusterctl/examples/vsphere/addons.yaml.template

I can't decide if they are meant for the external of the internal cluster.

They are meant for the internal cluster. The provider components are the cluster api stack that is applied to the internal and external cluster. The addon components are things we "add on" to the internal cluster in addition to the core k8s control plane as part of the baseline installation.

ingvagabund · 2018-08-01T12:05:45Z

clusterctl/clusterdeployer/clusterdeployer.go

+		if err := d.updateClusterEndpoint(internalClient, provider); err != nil {
+			return fmt.Errorf("unable to update internal cluster endpoint: %v", err)
+		}
+	} else {


If the pivotCluster is false, the cleanupExternalCluster needs to be set to false. Otherwise, we will provision the cluster API stack and then remove it.

This has been addressed here:

cluster-api/clusterctl/clusterdeployer/clusterdeployer.go

Line 247 in b546f32

if d.cleanupExternalCluster && d.pivotCluster {

roberthbailey · 2018-08-10T14:36:24Z

/assign @ingvagabund

k8s-ci-robot · 2018-08-10T14:36:25Z

@roberthbailey: GitHub didn't allow me to assign the following users: ingvagabund.

Note that only kubernetes-sigs members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @ingvagabund

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.

ingvagabund · 2018-08-13T11:45:31Z

@roberthbailey unfortunately I am not in the kubernetes-sigs org. What's the typical trial to get to the org? Just request for it?

roberthbailey · 2018-08-13T18:26:12Z

Yes. Since you are a member of the kubernetes org, you have implicit membership in the kubernetes-sigs org as well but you need to request it.

From https://github.com/kubernetes/community/blob/master/community-membership.md#kubernetes-ecosystem

We are currently working on automation that would transfer membership in the Kubernetes organization to any related orgs automatically, but such is not the case currently.

detiber · 2018-08-14T14:36:04Z

@ingvagabund I believe I've addressed all of the comments not already answered. I agree about the unfortunate naming of external/internal, but believe that would be better handled through a separate issue/PR.

ingvagabund · 2018-08-15T07:45:17Z

@roberthbailey I am in the sig now, thanks for the hints
/assign ingvagabund

ingvagabund · 2018-08-16T15:00:07Z

@detiber thanks for addressing the comments. Looks good now. Lemme just build and run the clusterctl command locally.

ingvagabund · 2018-08-17T13:09:58Z

@detiber lgtm. Can you squash the commits?

detiber · 2018-08-17T14:17:30Z

@ingvagabund squashed. I left the version bump for the api server image as a separate commit since it's not directly related to the change, thanks for the review!

ingvagabund · 2018-08-17T14:58:01Z

/lgtm

ingvagabund · 2018-08-22T07:52:19Z

/approve

ingvagabund · 2018-08-22T07:52:43Z

@medinatiger can you please approve?

roberthbailey · 2018-08-22T07:59:31Z

I was looking at this today and I had a couple of questions that I wanted to discuss in the wg meeting.

roberthbailey · 2018-08-22T20:37:37Z

@detiber - please ping this PR once you've updated it to reflect the discussion during the WG meeting today.

roberthbailey · 2018-08-31T19:21:07Z

Apparently the bot doesn't auto-tag PRs in this repo that need to be rebased, but this PR now needs to be rebased.

detiber · 2018-09-04T14:42:03Z

Rebased, but still need to address the default behavior as discussed in the cluster-api meeting.

detiber · 2018-09-05T19:21:43Z

Updated to switch the default back to pivoting of the cluster and specifying the command line flag to override.

It's not overly clear if we can have configurable defaults based on the values of other provided flags with cobra. If someone can point me to examples or docs on how to accomplish this, I will gladly update this PR accordingly.

- Add --no-pivot-cluster flag to disable pivoting of the cluster

roberthbailey · 2018-09-05T20:10:40Z

/lgtm
/approve
/hold

Holding for a bit in case other folks want to comment before the merge bot takes over.

k8s-ci-robot · 2018-09-05T20:10:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: detiber, ingvagabund, roberthbailey

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

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

frodenas · 2018-10-04T02:46:49Z

@roberthbailey There haven't been any comments in the last 28 days, can this be merged now?

roberthbailey · 2018-10-04T06:19:08Z

Yes, although it now needs to be rebased.

detiber · 2018-10-04T13:36:21Z

@frodenas @roberthbailey I can rebase this next week (and after #494) is merged

roberthbailey · 2018-10-12T08:35:50Z

@detiber - let me know when this has been rebased and is ready for review.

detiber · 2019-02-01T16:51:49Z

Closing in favor of the phases support.

Use base images for bootstrap and manifests images

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 27, 2018

k8s-ci-robot requested review from mkjelland and roberthbailey July 27, 2018 19:38

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 27, 2018

detiber force-pushed the noPivot branch from 7fc4c56 to 1c9b7b5 Compare July 27, 2018 19:55

detiber changed the title ~~[WIP] clusterctl - Do not pivot cluster by default~~ clusterctl - Do not pivot cluster by default Jul 31, 2018

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 Jul 31, 2018

ingvagabund reviewed Aug 1, 2018

View reviewed changes

detiber force-pushed the noPivot branch from a98fb0b to b546f32 Compare August 14, 2018 14:31

k8s-ci-robot assigned ingvagabund Aug 15, 2018

detiber force-pushed the noPivot branch from b546f32 to d7eff27 Compare August 17, 2018 14:16

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2018

roberthbailey added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2018

detiber force-pushed the noPivot branch from d7eff27 to 631690e Compare September 4, 2018 14:41

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2018

detiber changed the title ~~clusterctl - Do not pivot cluster by default~~ [WIP] clusterctl - Do not pivot cluster by default Sep 4, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2018

Bump the apiServerImage in clusterctl

9864549

detiber force-pushed the noPivot branch from 631690e to d6b3387 Compare September 5, 2018 19:17

detiber changed the title ~~[WIP] clusterctl - Do not pivot cluster by default~~ clusterctl - Allow for disabling pivoting of cluster Sep 5, 2018

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 Sep 5, 2018

clusterctl - Allow for disabling pivoting of cluster

eaddd5c

- Add --no-pivot-cluster flag to disable pivoting of the cluster

detiber force-pushed the noPivot branch from d6b3387 to eaddd5c Compare September 5, 2018 19:24

roberthbailey removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2018

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 5, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2018

roberthbailey added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2018

detiber closed this Feb 1, 2019

detiber deleted the noPivot branch April 2, 2019 17:44

jayunit100 pushed a commit to jayunit100/cluster-api that referenced this pull request Jan 31, 2020

Merge pull request kubernetes-sigs#463 from codenrhoden/use-base-images

0e25314

Use base images for bootstrap and manifests images

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clusterctl - Allow for disabling pivoting of cluster #463

clusterctl - Allow for disabling pivoting of cluster #463

detiber commented Jul 27, 2018 •

edited

Loading

ingvagabund left a comment

ingvagabund Aug 1, 2018

ingvagabund Aug 1, 2018

ingvagabund Aug 1, 2018

roberthbailey Aug 10, 2018

ingvagabund Aug 1, 2018

detiber Aug 14, 2018

roberthbailey commented Aug 10, 2018

k8s-ci-robot commented Aug 10, 2018

ingvagabund commented Aug 13, 2018

roberthbailey commented Aug 13, 2018

detiber commented Aug 14, 2018

ingvagabund commented Aug 15, 2018

ingvagabund commented Aug 16, 2018

ingvagabund commented Aug 17, 2018

detiber commented Aug 17, 2018

ingvagabund commented Aug 17, 2018

ingvagabund commented Aug 22, 2018

ingvagabund commented Aug 22, 2018

roberthbailey commented Aug 22, 2018

roberthbailey commented Aug 22, 2018

roberthbailey commented Aug 31, 2018

detiber commented Sep 4, 2018

detiber commented Sep 5, 2018

roberthbailey commented Sep 5, 2018

k8s-ci-robot commented Sep 5, 2018

frodenas commented Oct 4, 2018

roberthbailey commented Oct 4, 2018

detiber commented Oct 4, 2018

roberthbailey commented Oct 12, 2018

detiber commented Feb 1, 2019

clusterctl - Allow for disabling pivoting of cluster #463

clusterctl - Allow for disabling pivoting of cluster #463

Conversation

detiber commented Jul 27, 2018 • edited Loading

ingvagabund left a comment

Choose a reason for hiding this comment

ingvagabund Aug 1, 2018

Choose a reason for hiding this comment

ingvagabund Aug 1, 2018

Choose a reason for hiding this comment

ingvagabund Aug 1, 2018

Choose a reason for hiding this comment

roberthbailey Aug 10, 2018

Choose a reason for hiding this comment

ingvagabund Aug 1, 2018

Choose a reason for hiding this comment

detiber Aug 14, 2018

Choose a reason for hiding this comment

roberthbailey commented Aug 10, 2018

k8s-ci-robot commented Aug 10, 2018

ingvagabund commented Aug 13, 2018

roberthbailey commented Aug 13, 2018

detiber commented Aug 14, 2018

ingvagabund commented Aug 15, 2018

ingvagabund commented Aug 16, 2018

ingvagabund commented Aug 17, 2018

detiber commented Aug 17, 2018

ingvagabund commented Aug 17, 2018

ingvagabund commented Aug 22, 2018

ingvagabund commented Aug 22, 2018

roberthbailey commented Aug 22, 2018

roberthbailey commented Aug 22, 2018

roberthbailey commented Aug 31, 2018

detiber commented Sep 4, 2018

detiber commented Sep 5, 2018

roberthbailey commented Sep 5, 2018

k8s-ci-robot commented Sep 5, 2018

frodenas commented Oct 4, 2018

roberthbailey commented Oct 4, 2018

detiber commented Oct 4, 2018

roberthbailey commented Oct 12, 2018

detiber commented Feb 1, 2019

detiber commented Jul 27, 2018 •

edited

Loading