Support cluster API objects in multiple namespaces #481

ashish-amarnath · 2018-08-20T18:07:24Z

What this PR does / why we need it:
Supporting multiple clusters requires, cluster and machine to be stored in multiple namespaces, clusterctl create currently assumes default namespace which will not work in a multi-cluster setup.

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

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

ashish-amarnath · 2018-08-20T18:09:03Z

@davidewatson This is still a work in progress but just wanted to share what I have so far.
The unit tests don't pass atm there is that and other work that needs to go into this PR

davidewatson

Thanks, Ashish, this is looking good!

Currently, the Delete() functions assume a single cluster per namespace. This is no worse than the status quo, though since we are exporting this interface, I wonder if we should add a cluster name to the arguments (and require labels). I'll add a comment to one of #41, #145, #177. ...

davidewatson · 2018-08-20T19:32:16Z

clusterctl/clusterdeployer/clusterclient.go

 	clusters := []*clusterv1.Cluster{}
-	// TODO: Iterate over all namespaces where we could have Cluster API Objects https://github.com/kubernetes-sigs/cluster-api/issues/252
-	clusterlist, err := c.clientSet.ClusterV1alpha1().Clusters(apiv1.NamespaceDefault).List(metav1.ListOptions{})
+	clusterlist, err := c.clientSet.ClusterV1alpha1().Clusters(ns).List(metav1.ListOptions{})
 	if err != nil {
 		return nil, fmt.Errorf("error listing cluster objects: %v", err)


All of the log and error messages should probably reference the namespace to uniquely identify the cluster.

Yes, agree. Will do that.

I took a pass for this. I will do another pass before calling this PR done.

davidewatson · 2018-08-20T19:34:49Z

clusterctl/clusterdeployer/clusterdeployer.go

@@ -110,12 +117,11 @@ const (
 	timeoutKubeconfigReady = 20 * time.Minute
 )

-// Creates the a cluster from the provided cluster definition and machine list.
-
+// Create cerates the a cluster from the provided cluster definition and machine list.


s/cerates//g

TIL: cerate comes from the Latin cera, meaning wax.

clusterctl/clusterdeployer/clusterdeployer.go

vendor/k8s.io/kube-openapi/example/openapi-gen/BUILD.bazel

philips · 2018-08-20T20:51:22Z

/unassign philips

roberthbailey · 2018-08-23T06:13:58Z

/ok-to-test

clusterctl/clusterdeployer/clusterclient.go

add unit tests for multi namespace scenarios

clusterctl/clusterdeployer/clusterclient.go

clusterctl/clusterdeployer/clusterdeployer.go

clusterctl/clusterdeployer/clusterclient.go

clusterctl/clusterdeployer/clusterdeployer_test.go

ashish-amarnath · 2018-08-29T01:35:36Z

/assign @detiber

davidewatson · 2018-08-29T19:53:53Z

/lgtm

k8s-ci-robot · 2018-08-29T19:54:00Z

@davidewatson: changing LGTM is restricted to assignees, and only kubernetes-sigs/cluster-api repo collaborators may be assigned issues.

In response to this:

/lgtm

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.

detiber

Overall these changes look good to me, but should clusterctl/cmd/create_cluster.go and clusterctl/cmd/validate_cluster.go also be updated to add support for setting the namespace?

detiber · 2018-08-30T13:29:15Z

clusterctl/clusterdeployer/clusterclient.go

@@ -118,11 +171,15 @@ func (c *clusterClient) GetClusterObjects() ([]*clusterv1.Cluster, error) {
 	return clusters, nil
 }

-func (c *clusterClient) GetMachineDeploymentObjects() ([]*clusterv1.MachineDeployment, error) {
+func (c *clusterClient) GetClusterObjects() ([]*clusterv1.Cluster, error) {


Could you add comments to these preserved methods to indicate that they are deprecated and to be removed in the future? It might be good to also log the deprecation as well.

Good suggestion. Added comments and log messages.

clusterctl/clusterdeployer/clusterdeployer.go

detiber · 2018-08-30T19:24:13Z

/lgtm

ashish-amarnath · 2018-08-30T22:32:24Z

/approve

ashish-amarnath · 2018-08-30T22:35:42Z

/assign @roberthbailey

roberthbailey

I didn't review the test code too closely.

clusterctl/clusterdeployer/clusterclient.go

clusterctl/clusterdeployer/clusterdeployer.go

clusterctl/clusterdeployer/clusterclient.go

clusterctl/clusterdeployer/clusterdeployer.go

vendor/k8s.io/kube-openapi/example/openapi-gen/BUILD.bazel

Fix: cluster client should not attempt deletion of the default namespace Make logs and error messages consistent when referring to cluster.Namespace Update unit tests

roberthbailey · 2018-09-06T23:15:05Z

/lgtm
/approve

k8s-ci-robot · 2018-09-06T23:15:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashish-amarnath, 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

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 Aug 20, 2018

k8s-ci-robot requested review from philips and spew August 20, 2018 18:07

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

davidewatson reviewed Aug 20, 2018

View reviewed changes

ashish-amarnath force-pushed the iss-252-muli-ns branch from bdb245c to a406536 Compare August 23, 2018 00:21

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

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 23, 2018

detiber reviewed Aug 23, 2018

View reviewed changes

clusterctl/clusterdeployer/clusterclient.go Outdated Show resolved Hide resolved

ashish-amarnath force-pushed the iss-252-muli-ns branch 2 times, most recently from 09de334 to 37b01d8 Compare August 23, 2018 22:29

implement APIs for multi namespace support for Cluster API objects

da0e454

add unit tests for multi namespace scenarios

ashish-amarnath force-pushed the iss-252-muli-ns branch from 37b01d8 to da0e454 Compare August 23, 2018 22:40

ashish-amarnath commented Aug 24, 2018

View reviewed changes

clusterctl/clusterdeployer/clusterclient.go Outdated Show resolved Hide resolved

ashish-amarnath commented Aug 24, 2018

View reviewed changes

clusterctl/clusterdeployer/clusterdeployer.go Outdated Show resolved Hide resolved

ashish-amarnath commented Aug 24, 2018

View reviewed changes

clusterctl/clusterdeployer/clusterclient.go Outdated Show resolved Hide resolved

ashish-amarnath commented Aug 24, 2018

View reviewed changes

clusterctl/clusterdeployer/clusterdeployer_test.go Outdated Show resolved Hide resolved

ashish-amarnath commented Aug 24, 2018

View reviewed changes

clusterctl/clusterdeployer/clusterdeployer_test.go Outdated Show resolved Hide resolved

ashish-amarnath commented Aug 24, 2018

View reviewed changes

clusterctl/clusterdeployer/clusterdeployer_test.go Outdated Show resolved Hide resolved

implement methods to handle namespace create and delete

6f3b124

ashish-amarnath force-pushed the iss-252-muli-ns branch from ec9081e to 6f3b124 Compare August 24, 2018 19:58

ashish-amarnath changed the title ~~[WIP] Support cluster API objects in multiple namespaces~~ Support cluster API objects in multiple namespaces Aug 24, 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 Aug 24, 2018

ashish-amarnath mentioned this pull request Aug 27, 2018

REQUEST: New membership for ashish-amarnath kubernetes/org#45

Closed

6 tasks

k8s-ci-robot assigned detiber Aug 29, 2018

detiber reviewed Aug 30, 2018

View reviewed changes

add deprecation comments and logs and remove unused methods

da42f35

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

k8s-ci-robot assigned roberthbailey Aug 30, 2018

davidewatson mentioned this pull request Sep 4, 2018

REQUEST: New membership for davidewaton kubernetes/org#59

Closed

6 tasks

roberthbailey reviewed Sep 5, 2018

View reviewed changes

addressing code review comments

31c617e

Fix: cluster client should not attempt deletion of the default namespace Make logs and error messages consistent when referring to cluster.Namespace Update unit tests

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

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

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

k8s-ci-robot merged commit 3b6365b into kubernetes-sigs:master Sep 6, 2018

ashish-amarnath deleted the iss-252-muli-ns branch September 7, 2018 05:13

This was referenced Sep 13, 2018

clusterctl: match kubeconfig by path instead of content #500

Merged

clusterctl: put resources in the kube-system namespace instead of default kubernetes-sigs/cluster-api-provider-digitalocean#47

Closed

ashish-amarnath mentioned this pull request Sep 21, 2018

Cluster-API apiserver should be namespace-ed #509

Merged

This was referenced Jan 9, 2019

Support multiple clusters in a single namespace #667

Closed

Establish a stronger link between Machines, MachineSets, and MachineDeployments with their Cluster. #41

Closed

Support cluster API objects in multiple namespaces #481

Support cluster API objects in multiple namespaces #481

Uh oh!

Conversation

ashish-amarnath commented Aug 20, 2018

Uh oh!

ashish-amarnath commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidewatson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidewatson Aug 20, 2018

Choose a reason for hiding this comment

Uh oh!

ashish-amarnath Aug 20, 2018

Choose a reason for hiding this comment

Uh oh!

ashish-amarnath Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

davidewatson Aug 20, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

philips commented Aug 20, 2018

Uh oh!

roberthbailey commented Aug 23, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ashish-amarnath commented Aug 29, 2018

Uh oh!

davidewatson commented Aug 29, 2018

Uh oh!

k8s-ci-robot commented Aug 29, 2018

Uh oh!

detiber left a comment

Choose a reason for hiding this comment

Uh oh!

detiber Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

ashish-amarnath Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

detiber commented Aug 30, 2018

Uh oh!

ashish-amarnath commented Aug 30, 2018

Uh oh!

ashish-amarnath commented Aug 30, 2018

Uh oh!

roberthbailey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roberthbailey commented Sep 6, 2018

Uh oh!

k8s-ci-robot commented Sep 6, 2018

Uh oh!

Uh oh!

ashish-amarnath commented Aug 20, 2018 •

edited

Loading

davidewatson left a comment •

edited

Loading