✨ Adding CAPI Provisioner to vc-manager #136

christopherhein · 2021-06-16T00:25:55Z

What this PR does / why we need it:
This adds integration points between VC and CAPN but more so CAPI as it doesn't use any CAPN components, solely reliant on CAPI's v1alpha4.Cluster{} resource. The templates/cluster-template-virtualcluster.yaml is a clusterctl --flavor for auto configuring the VirtualCluster CR with the cluster.

Testing:

Clone CAPI master
build master clusterctl
Clone this branch
Run local build https://github.com/kubernetes-sigs/cluster-api-provider-nested/tree/main/docs#create-docker-images-manifests-and-load-images
cd virtualcluster/
make build-images
kind load docker-image virtualcluster/vn-agent-amd64 && kind load docker-image virtualcluster/syncer-amd64 && kind load docker-image virtualcluster/manager-amd64
kubectl apply -f config/crd/
kubectl apply -f config/setup/all_in_one_capi.yaml
From this branch run: ../cluster-api/bin/clusterctl generate cluster ${CLUSTER_NAME} --from templates/cluster-template-virtualcluster.yaml | k apply -f -

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 #81
Fixes #135

/milestone v0.1.x

christopherhein · 2021-06-16T00:27:10Z

controllers/nestedcluster_controller.go

 	}

-	return ctrl.Result{}, nil
+	return ctrl.Result{RequeueAfter: 5 * time.Second}, nil


For now this will just constant poll for changes until status provisioned, this should in the long run be triggered by the NCP status updating instead.

christopherhein · 2021-06-16T00:28:39Z

virtualcluster/config/rbac/role.yaml

@@ -0,0 +1,128 @@
+


Apparently the role.yaml wasn't working cause the comments were nested under a function. Moving them allows controller-gen to regenerate these, I'm not sure if they are 100% but this work can be punted to #130

christopherhein · 2021-06-16T00:29:44Z

virtualcluster/config/setup/all_in_one_capi.yaml

        image: virtualcluster/manager-amd64
-        imagePullPolicy: Always
+        imagePullPolicy: Never


This will be handled properly with #130 using kustomize to set them differently based on the env/stage, for now Never allows us to build images locally and load them into kind for testing.

christopherhein · 2021-06-16T00:30:21Z

virtualcluster/go.mod

+	github.com/moby/spdystream => github.com/moby/spdystream v0.2.0
+	k8s.io/api => k8s.io/api v0.21.1
+	k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.21.1
+	k8s.io/apimachinery => k8s.io/apimachinery v0.21.1


You'll notice this also updates us to v0.21 for the lates Controller Runtime version that CAPI v1alpha4 shipped with.

christopherhein · 2021-06-16T00:38:18Z

/assign @Fei-Guo @charleszheng44 @weiling61

Fei-Guo

Awesome work! Nits.

virtualcluster/pkg/syncer/constants/constants.go

virtualcluster/pkg/controller/controllers/capi_virtualcluster_controller.go

Signed-off-by: Chris Hein <[email protected]>

christopherhein · 2021-06-16T18:04:03Z

thanks @Fei-Guo updated based on your feedback.

Fei-Guo · 2021-06-16T19:02:20Z

virtualcluster/pkg/controller/controllers/capi_virtualcluster_controller.go

+	return ctrl.NewControllerManagedBy(mgr).
+		WithOptions(opts).
+		For(&tenancyv1alpha1.VirtualCluster{}).
+		Owns(&clusterv1.Cluster{}).


I think setting owner reference for the Cluster object is a must now, otherwise, the vcmanager will miss the event of Cluster state update?

sorta, right now if the cluster isn't up we blindly retry until successful, which accounts for this case, the code I removed wasn't being used at all since I didn't set and ownerref.

my concern is I'm not sure I fully agree that a VC should be the owner of the cluster, like there could be a case where you want to move this can not delete all the resources and having an ownerref would setup GC to delete it.

This all happens right - https://github.com/kubernetes-sigs/cluster-api-provider-nested/pull/136/files#diff-607056cc549543fc88ebc9695705629f15915ccaac3161e854845ca5a2f0b548R149-R159

We can definitely improve that too trying to figure out the right model for how this wires together.

My concern is that if vc is not the cluster owner, once cluster become ready, the event will not trigger vc reconciler because the cluster does not have owner. Then the vc status cannot be updated to ready unless we add periodic check. Do I miss anything?

Yeah, check the link in that last message, if the cluster is in Provisioning which is auto set if blank then I return ctrl.Result{RequeueAfter: 1*time.Second}, nil

I see. Since vc lifecycle and cluster lifecycle are completely decoupled now, polling in VC controller may not be ideal since it may take longtime until the cluster is ready. It is ok for now, moving forward we can set up cluster Watch manually and implement a custom enqueue struct so that we can add the matching VC in the queue by checking the annotation.

Yeah, let me file an issue to track on improving this, cause I 100% agree.

Fei-Guo · 2021-06-16T20:51:49Z

LGTM. I will let Chao give a final approve.

charleszheng44 · 2021-06-17T16:21:24Z

@christopherhein Everything works as expected until I created a pod on the tenant cluster. The pod hangs in the pending state. Looks like the syncer can not connect to the tenant controlplane. I checked the syncer log and saw the following error message

I0617 16:08:03.453969       1 syncer.go:383] cluster default shutdown: Get "https://cluster-sample-apiserver:6443/api?timeout=30s": dial tcp 127.0.0.1:6443: connect: connection refused

That's wired. Because I was able to do port forwarding on svc/cluster-sample-apiserver 6443:6443

christopherhein · 2021-06-17T16:25:07Z

@christopherhein Everything works as expected until I created a pod on the tenant cluster. The pod hangs in the pending state. Looks like the syncer can not connect to the tenant controlplane. I checked the syncer log and saw the following error message
I0617 16:08:03.453969       1 syncer.go:383] cluster default shutdown: Get "https://cluster-sample-apiserver:6443/api?timeout=30s": dial tcp 127.0.0.1:6443: connect: connection refused
That's wired. Because I was able to do port forwarding on svc/cluster-sample-apiserver 6443:6443

@charleszheng44 It looks like you are using the wrong specs, can you delete that cluster then redo it making sure that the Cluster object includes the namespace in the controlPlaneEndpoint.host you'll see that commented about in - templates/cluster-template-virtualcluster.yaml - https://github.com/kubernetes-sigs/cluster-api-provider-nested/pull/136/files#diff-ec9ceefbdef73c7fb4f5a8ef98f5342b423b770ce83d208b185267d54ed2a10eR7-R12

zhuangqh · 2021-06-17T16:51:29Z

virtualcluster/pkg/controller/controller.go

+	if c.ProvisionerName == "capi" {
+		if err := (&controllers.ReconcileCAPIVirtualCluster{
+			Client: mgr.GetClient(),
+			Log:    c.Log.WithName("virtualcluster"),


should we change the log name here to distinguish between different controller?

We're still reconciling virtualcluster resources so I don't think so and they can't be turned on with other reconcilers since it's done via the --master-prov flag. Also we don't do this with native vs aliyun:

https://github.com/kubernetes-sigs/cluster-api-provider-nested/blob/main/virtualcluster/pkg/controller/controller.go#L52

So I would say no, but I'm open to it.

k8s-ci-robot · 2021-06-17T18:53:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: charleszheng44, christopherhein

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~~ [charleszheng44,christopherhein]

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

charleszheng44 · 2021-06-17T18:53:59Z

/lgtm

k8s-ci-robot added this to the v0.1.x milestone Jun 16, 2021

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 16, 2021

k8s-ci-robot requested review from charleszheng44 and Fei-Guo June 16, 2021 00:26

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 16, 2021

christopherhein commented Jun 16, 2021

View reviewed changes

christopherhein force-pushed the feature/vc-capi-provisioner branch from 25ff104 to b25255e Compare June 16, 2021 00:35

k8s-ci-robot assigned charleszheng44, Fei-Guo and weiling61 Jun 16, 2021

christopherhein changed the title ~~✨ Adding CAPI Provisioner tro vc-manager~~ ✨ Adding CAPI Provisioner to vc-manager Jun 16, 2021

christopherhein mentioned this pull request Jun 16, 2021

Update README to reflect latest code and way for init #127

Merged

Fei-Guo reviewed Jun 16, 2021

View reviewed changes

virtualcluster/pkg/syncer/constants/constants.go Outdated Show resolved Hide resolved

virtualcluster/pkg/controller/controllers/capi_virtualcluster_controller.go Outdated Show resolved Hide resolved

christopherhein added 2 commits June 16, 2021 11:02

✨ Adding CAPI VC Provisioner to vc-manager

885f24e

Signed-off-by: Chris Hein <[email protected]>

adding go vendor files

3799f8b

Signed-off-by: Chris Hein <[email protected]>

christopherhein force-pushed the feature/vc-capi-provisioner branch from b25255e to 3799f8b Compare June 16, 2021 18:02

Fei-Guo reviewed Jun 16, 2021

View reviewed changes

christopherhein mentioned this pull request Jun 16, 2021

✨ Update CAPI VC Controller to Use a Cluster watch. #137

Closed

zhuangqh reviewed Jun 17, 2021

View reviewed changes

christopherhein mentioned this pull request Jun 17, 2021

📖 Update docs/README.md with virtualcluster flavor #141

Open

charleszheng44 approved these changes Jun 17, 2021

View reviewed changes

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

k8s-ci-robot merged commit ce11f3e into kubernetes-retired:main Jun 17, 2021

christopherhein mentioned this pull request Jun 21, 2021

🐛 Adding back original all_in_one.yaml for VC #144

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Adding CAPI Provisioner to vc-manager #136

✨ Adding CAPI Provisioner to vc-manager #136

christopherhein commented Jun 16, 2021

christopherhein Jun 16, 2021

christopherhein Jun 16, 2021

christopherhein Jun 16, 2021

christopherhein Jun 16, 2021

christopherhein commented Jun 16, 2021

Fei-Guo left a comment

christopherhein commented Jun 16, 2021

Fei-Guo Jun 16, 2021

christopherhein Jun 16, 2021

christopherhein Jun 16, 2021

christopherhein Jun 16, 2021

Fei-Guo Jun 16, 2021 •

edited

Loading

christopherhein Jun 16, 2021

Fei-Guo Jun 16, 2021 •

edited

Loading

christopherhein Jun 16, 2021

Fei-Guo commented Jun 16, 2021

charleszheng44 commented Jun 17, 2021

christopherhein commented Jun 17, 2021

zhuangqh Jun 17, 2021 •

edited

Loading

christopherhein Jun 17, 2021 •

edited

Loading

k8s-ci-robot commented Jun 17, 2021

charleszheng44 commented Jun 17, 2021

✨ Adding CAPI Provisioner to vc-manager #136

✨ Adding CAPI Provisioner to vc-manager #136

Conversation

christopherhein commented Jun 16, 2021

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

christopherhein commented Jun 16, 2021

Fei-Guo left a comment

Choose a reason for hiding this comment

christopherhein commented Jun 16, 2021

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

Fei-Guo Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fei-Guo Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fei-Guo commented Jun 16, 2021

charleszheng44 commented Jun 17, 2021

christopherhein commented Jun 17, 2021

zhuangqh Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

christopherhein Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 17, 2021

charleszheng44 commented Jun 17, 2021

Fei-Guo Jun 16, 2021 •

edited

Loading

Fei-Guo Jun 16, 2021 •

edited

Loading

zhuangqh Jun 17, 2021 •

edited

Loading

christopherhein Jun 17, 2021 •

edited

Loading