Add NodeIPAM Controller to CCM #247

cici37 · 2021-07-13T21:52:50Z

This PR is to add NodeIPAM controller into CCM.

Following work:

A following PR for NodeIPAM controller configuration switch.
Further customize the code regarding with legacycloud

cici37 · 2021-07-15T18:57:58Z

/test cloud-provider-gcp-e2e-full

cici37 · 2021-07-21T19:30:04Z

/assign @jpbetz

jpbetz

Generally looks good. A couple details:

Can we split the vendor changes into their commit? Ideally we would have one commit for vendor and another for the code copy, and another for any hand written changes.

Do we need to copy over https://github.com/kubernetes/kubernetes/blob/master/test/integration/ipamperf/ipam_test.go to somewhere appropriate? I don't see any e2e tests that run for nodeIPAM, but let me know if I missed anything. It would be good ensure we're running them in our presubmits for cloud-provider-gcp if there are any.

I suspect we don't need both legacyprovider.go and nolegacyprovider.go. Can we simplify this down? Ideally we'd drop "legacy" from anything we're going to keep long term.

go.mod

pkg/controller/nodeipam/config/types.go

jpbetz · 2021-07-21T19:27:20Z

pkg/controller/nodeipam/config/v1alpha1/register.go

+limitations under the License.
+*/
+
+package v1alpha1


Since we're out-of-tree, I'm a bit unclear on what the stability graduation criteria is going to be. But I think keeping this as v1alpha1 makes sense.

@cheftako @bowei feel free to weigh in if you have any thoughts about stability level for the out-of-tree IPAM controller.

I think its probably appropriate for now. Lets get it moved. Then clean it up for our use (If possible), then move it to Beta.

pkg/controller/nodeipam/nolegacyprovider.go

tools/.golint_failures

cici37 · 2021-07-21T20:27:01Z

@jpbetz Hi Joe, thank you for your review. Regarding with your suggestion:

I have split the commits based on your suggestion. Please feel free to take a look now.
Currently we don't have the mechanism in cloud-provider-gcp to run integration tests. For the current e2e test, it will run all the available e2e test from k/k so if there is any e2e tests related with NodeIPAM controller, it will not be missed(there might not be e2e test highly related with nodeipam controller).
For current nodeIPAM controller in //pkg/controller/nodeipam, it would be a directly copy over from k/k which will make the review process easier. We sure could customize based on what we need. It could go with the next pr and I will update the following step to reflect.

jpbetz · 2021-07-21T20:31:44Z

/lgtm
Code migration looks good mechanically. I'd like to get a review from someone on networking (@bowei?) to make sure we get everything we need from a networking perspective.

cici37 · 2021-07-21T20:37:42Z

cc @sdmodi

sdmodi

It looks like you have copied all of the code under https://github.com/kubernetes/kubernetes/tree/master/pkg/controller/nodeipam except the legacyprovider.go file. I think this is fine. I think it would have been ok to not port over ipam/controller_legacyprovider.go as well.

I am no expert but this looks good to me and it has the code that IPv6 feature needs.

cluster/gce/manifests/cloud-controller-manager.manifest

sdmodi · 2021-07-23T23:01:57Z

/assign @bowei

cici37 · 2021-07-26T23:02:37Z

/assign @cheftako

cici37 · 2021-08-17T17:55:47Z

@bowei Thanks for reviewing. I have updated commit message with source file info(No changes except commit message update). Please have a look when you have time. Thank you

sdmodi · 2021-08-19T23:51:38Z

Cici, it looks like my patch caused merge conflicts for you. Would you please update this patch. Thanks!

cmd/cloud-controller-manager/main.go

cheftako · 2021-08-22T05:28:33Z

cmd/cloud-controller-manager/main.go

+	// e.g. remove the cloud-node-lifecycle controller which current cloud provider does not need.
+	//delete(controllerInitializers, "cloud-node-lifecycle")
+
+	// Here is an example to add an controller(NodeIpamController) which will be used by cloud provider


Ditto this is no longer an example, this is actually using said controller.

cmd/cloud-controller-manager/nodeipamcontroller.go

cheftako · 2021-08-22T05:37:50Z

cmd/cloud-controller-manager/nodeipamcontroller.go

+	if len(strings.TrimSpace(nodeIPAMConfig.ServiceCIDR)) != 0 {
+		_, serviceCIDR, err = net.ParseCIDR(nodeIPAMConfig.ServiceCIDR)
+		if err != nil {
+			klog.Warningf("Unsuccessful parsing of service CIDR %v: %v", nodeIPAMConfig.ServiceCIDR, err)


If we can't parse our configuration is starting the right thing to do?

It is the current logic KCM starts nodeipam controller. I am not sure if some sort of default values will be used when met parsing issue?

In the interest of getting this in with minimal changes lets go ahead merge with this. However we should probably have at least a tracking bug. My guess is the controller does not do what we want in this case but that having it "start" means that its failing quietly, which is bad.

cmd/cloud-controller-manager/nodeipamcontroller.go

go.sum

cheftako · 2021-08-22T05:47:42Z

pkg/controller/nodeipam/OWNERS

+# See the OWNERS docs at https://go.k8s.io/owners
+
+approvers:
+- bowei


Can we get a second approver? Dislike having a SPF.

Would you have any suggestions on who could be added here? cc @bowei

cheftako · 2021-08-22T05:48:49Z

pkg/controller/nodeipam/config/v1alpha1/BUILD

+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/conversion:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//vendor/k8s.io/kube-controller-manager/config/v1alpha1:go_default_library",


Need to work out how to break this dependency.

cheftako · 2021-08-22T05:55:01Z

pkg/controller/nodeipam/ipam/cloud_cidr_allocator.go

+	klog.V(0).Infof("Sending events to api server.")
+	eventBroadcaster.StartRecordingToSink(&v1core.EventSinkImpl{Interface: client.CoreV1().Events("")})
+
+	gceCloud, ok := cloud.(*gce.Cloud)


Good for this checkin. Might be interesting to see if we need to keep this in a follow on.

cheftako · 2021-08-22T06:02:12Z

pkg/util/taints/taints.go

+	"k8s.io/api/core/v1"
+)
+
+// TaintExists checks if the given taint exists in list of taints. Returns true if exists false otherwise.


Is this really a cloud-provider-gcp utility?

It is used when creates a new cloud CIDR allocator here

Not complaining about it being a utility or our using it. My through was this seems like a generic K/K utility for taint detection. There seems to be nothing GCP specific to this code.

cheftako · 2021-08-22T06:06:15Z

/lgtm
Couple of minor nits before approval and of course resolve the merge conflict.

Copy source code files from k8s.io/kubernetes Githash:a013c6a2db54c59b78de974b181586723e088246 Repo: https://github.com/kubernetes/kubernetes Paths: //pkg/controller/nodeipam

…ion, etc.

sdmodi · 2021-08-25T16:31:41Z

Thanks Cici for the quick turnaround on the comments. Walter, would you please take a look at this. This patch is blocking IPv6 support in GKE. Thanks!

bowei · 2021-08-26T22:37:19Z

/approve

cheftako · 2021-08-27T17:16:55Z

/lgtm
/approve

k8s-ci-robot · 2021-08-27T17:17:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, cheftako, cici37, sdmodi

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~~ [bowei,cheftako]

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 13, 2021

k8s-ci-robot requested review from jpbetz and saad-ali July 13, 2021 21:53

k8s-ci-robot added the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Jul 13, 2021

cici37 force-pushed the nodeipam branch 2 times, most recently from 18e8ff0 to 52566ce Compare July 14, 2021 22:29

cici37 changed the title ~~[WIP]Add NodeIPAM Controller to CCM~~ Add NodeIPAM Controller to CCM Jul 15, 2021

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

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2021

cici37 force-pushed the nodeipam branch from 52566ce to cc061d7 Compare July 21, 2021 19:23

k8s-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. labels Jul 21, 2021

cici37 force-pushed the nodeipam branch from cc061d7 to 817bd3d Compare July 21, 2021 19:24

k8s-ci-robot assigned jpbetz Jul 21, 2021

jpbetz requested changes Jul 21, 2021

View reviewed changes

cici37 force-pushed the nodeipam branch from 7cc965a to ed3c3e0 Compare July 21, 2021 20:19

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

sdmodi approved these changes Jul 21, 2021

View reviewed changes

cluster/gce/manifests/cloud-controller-manager.manifest Show resolved Hide resolved

k8s-ci-robot assigned bowei Jul 23, 2021

sdmodi mentioned this pull request Jul 26, 2021

Start the sharedInformers for cloud controller manager controllers #257

Closed

k8s-ci-robot assigned cheftako Jul 26, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2021