Migrate to kubebuilder #494

pwittrock · 2018-09-07T23:28:05Z

This PR is a WIP for migrating from apiserver-builder (aggregation based) to kubebuilder (CRD based). As a consequence of differences in the design philosophies a number of structural changes are made:

Switch from code-generation clients to a dynamic client
Remove manually working with informers / listers - replace with first class support for watch-reconcile
Decompose the Machine-Node bi-Controller into 2 separate Controllers: Node and Machine
Introduction of Manager as mechanism for injecting dependencies instead of "Init" functions called from generated code
Deletion of lost of generated code. Replace with small amount of scaffolded code (doesn't need to be regenerated).

roberthbailey · 2018-09-27T22:51:03Z

I've removing WIP from this PR. The tests all pass locally for me and I am able to stand up a cluster on gce using clusterctl with this change. I need to figure out how to get prow configured to run the kubebuilder tests (e.g. have etcd, apiserver, etc. in the path).

It would also be nice to update to go 1.10+ while doing so to re-enable coverage, which works on multiple packages in 1.10 but not in earlier versions. Coverage has temporarily been disabled in the test target.

pwittrock · 2018-09-28T00:58:56Z

Tests are passing :)

scruplelesswizard

cmd/clusterctl/clusterctl binary shouldn't be committed to the repo

This is a quick first pass. Wanted to free up for @xmudrii to continue

scruplelesswizard · 2018-09-28T09:33:30Z

CONTRIBUTING.md

@@ -1,116 +0,0 @@
-# Contributing Guidelines


Was CONTRIBUTING.md intentionally removed, or was this a drive-by in 46b895e?

I think it was an artifact of the commits. I can probably clean that up if you like. It looks like there aren't any changes to that file in the PR as a whole. https://github.com/kubernetes-sigs/cluster-api/pull/494/files

scruplelesswizard · 2018-09-28T09:40:11Z

Dockerfile

+RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -a -o manager sigs.k8s.io/cluster-api/cmd/manager
+
+# Copy the controller-manager into a thin image
+FROM ubuntu:latest


We should use debian-base or scratch as the base image

ref: kubernetes/kubernetes#40248 (comment)

I switched to debian:latest. I didn't see debian-base here: https://hub.docker.com/_/debian/. Is this correct?

scruplelesswizard · 2018-09-28T09:43:03Z

Dockerfile

+FROM golang:1.10.3 as builder
+
+# Copy in the go src
+WORKDIR /go/src/sigs.k8s.io/cluster-api


~~/go/~~ $GOPATH/

scruplelesswizard · 2018-09-28T09:49:55Z

LICENSE

@@ -1,201 +0,0 @@
-                                 Apache License


Was LICENSE intentionally removed, or was this a drive-by?

I think that is an artifact of the commits. It should be back.

scruplelesswizard · 2018-09-28T10:21:29Z

cmd/clusterctl/clusterdeployer/clusterclient/clusterclient.go

+	closeFn         func() error
+}
+
+// New creates and returns the address of a Client, the kubeconfig argument is expected to be the string represenattion


typo
/represenattion/representation/

scruplelesswizard · 2018-09-28T10:38:06Z

cmd/clusterctl/clusterdeployer/clusterclient/clusterclient.go

+	return nil
+}
+
+// NewFromDefaultSearchPath creates and returns the address of a Client, the kubeconfigFile argument is expected to be the path to a


/Client, the kubeconfigFile/Client. The kubeconfigFile/

scruplelesswizard · 2018-09-28T10:39:35Z

cmd/clusterctl/clusterdeployer/clusterclient/clusterclient.go

+	}, nil
+}
+
+// Frees resources associated with the cluster client


/Frees/Close frees/

scruplelesswizard · 2018-09-28T10:44:21Z

cmd/clusterctl/clusterdeployer/clusterclient/clusterclient.go

+}
+
+// Deprecated API. Please do not extend or use.
+func (c *client) GetClusterObjects() ([]*clusterv1.Cluster, error) {


If the client isn't backwards compatible, maybe this is a good time to drop the deprecated APIs?

xmudrii

Review in progress, but here are initial findings.

Some other nits:

cmd/clusterctl/clusterctl - binary file, should probably be deleted
cmd/clusterctl/CONTRIBUTING.md - do we need this CONTRIBUTING.md here?
cmd/clusterctl/README.md - to be checked should links be updated

xmudrii · 2018-09-28T16:18:20Z

Makefile

-	$$GOPATH/bin/conversion-gen -i ./pkg/apis/cluster/v1alpha1/ -O zz_generated.conversion --go-header-file boilerplate.go.txt
+# Deploy controller in the configured Kubernetes cluster in ~/.kube/config
+deploy: manifests
+	kubectl apply -f config/crds


Nit: this line could be replaced by deploy: manifests install.

I am not sure the CRDs need to be installed separately since they are referenced by config/default/kustomization.yaml. Am I missing something?

No. That can probably just be deleted.

xmudrii · 2018-09-28T16:22:45Z

cmd/clusterctl/clientcmd/configutil.go

+package clientcmd
+
+import (
+	"fmt"


Nit(goimports): probably a new line here

xmudrii · 2018-09-28T16:23:59Z

cmd/clusterctl/clusterdeployer/clusterclient/clientfactory.go

+	"sigs.k8s.io/cluster-api/cmd/clusterctl/clientcmd"
+)
+
+// Can create cluster clients


Nit(lint): could be Factory can...

xmudrii · 2018-09-28T16:33:41Z

pkg/apis/cluster/v1alpha1/cluster_types.go

 	"sigs.k8s.io/cluster-api/pkg/apis/cluster/common"
 )

-const ClusterFinalizer string = "cluster.cluster.k8s.io"
+const ClusterFinalizer = "cluster.cluster.k8s.io"


Question: should this be cluster.cluster.k8s.io or clusters.cluster.k8s.io? I've seen that CRDs are named plural, but should plural name be used here as well?

@xmudrii Good question. Since this wasn't something changed by this PR and this PR has a lot of changes, perhaps it can be opened in a separate thread or issue.

xmudrii · 2018-09-28T16:34:48Z

pkg/apis/cluster/v1alpha1/machine_types.go

 )

 // Finalizer is set on PreareForCreate callback
-const MachineFinalizer string = "machine.cluster.k8s.io"
+const MachineFinalizer = "machine.cluster.k8s.io"


Question: Same as for cluster/clusters, should this be machines.cluster.k8s.io?

pwittrock · 2018-09-28T17:00:07Z

@xmudrii Thanks for the comments.

pwittrock · 2018-09-28T17:00:30Z

FYI: I restructured the commits and re-pushed so that it is easier to review.

tamalsaha · 2018-09-28T17:01:53Z

Any plans to use 1.12 instead of ancient 1.10 version at this point?

pwittrock · 2018-10-01T16:23:58Z

pkg/apis/cluster/v1alpha1/machine_types.go

@@ -128,15 +122,16 @@ type MachineStatus struct {
 	// can be added as events to the Machine object and/or logged in the


scruplelesswizard · 2018-09-28T13:27:58Z

pkg/apis/cluster/v1alpha1/machineset_types.go

@@ -128,9 +124,7 @@ type MachineSetStatus struct {
 	ErrorMessage *string `json:"errorMessage,omitempty"`


typo
/reconcilation/reconciliation/

scruplelesswizard · 2018-09-28T13:31:40Z

cmd/clusterctl/clientcmd/configutil.go

+}
+
+// newRestConfigForKubeconfig creates a rest.Config for a given kubeconfig string.
+func newRestConfigForKubeconfig(kubeconfig string) (*rest.Config, error) {


lint: deadcode
It appears this isn't used anywhere in the package. Is this intentional?

scruplelesswizard · 2018-09-28T13:37:30Z

pkg/controller/machinedeployment/controller.go

+	"sigs.k8s.io/controller-runtime/pkg/handler"
+	"sigs.k8s.io/controller-runtime/pkg/manager"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+	"sigs.k8s.io/controller-runtime/pkg/source"
 )

 const requeueAfterWhenQueueAbsent = 1 * time.Second


lint: deadcode

Looks like the patch from #504 was dropped (as intended) without removing the const

scruplelesswizard · 2018-09-28T18:27:53Z

Sorry for the late comments, wifi was out on the train. If these have already been fixed please ignore.

pwittrock · 2018-09-28T18:33:42Z

Any plans to use 1.12 instead of ancient 1.10 version at this point?

Are you asking about the cluster that gets created, or the codebase the apis are built on? This is using the 1.11.0 : version = "kubernetes-1.11.0" version of the kubernetes libs.

pwittrock · 2018-09-28T18:34:58Z

@chaosaffe Thanks for the first pass. I've addressed a few of the comments, but not all.

pwittrock · 2018-09-28T18:45:15Z

cmd/clusterctl/clusterdeployer/clusterdeployer.go

@@ -298,59 +297,29 @@ func (d *ClusterDeployer) saveProviderComponentsToCluster(factory ProviderCompon
 }

 func (d *ClusterDeployer) applyClusterAPIStack(client clusterclient.Client, namespace string) error {
-	glog.Info("Applying Cluster API APIServer")


Deleted because we no longer setup an aggregated apiserver

liztio · 2018-09-28T20:06:25Z

What's the thinking behind removing the Bazel build infrastructure?

xmudrii

I've went through some commits again and everything seems reasonable. Looks very well done, no more comments from my side 👍

roberthbailey · 2018-10-04T23:49:17Z

/lgtm
/approve

k8s-ci-robot · 2018-10-04T23:49:24Z

[APPROVALNOTIFIER] This PR is APPROVED

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

pwittrock · 2018-10-05T00:13:27Z

Woot!

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 7, 2018

k8s-ci-robot requested review from karan and rsdcastro September 7, 2018 23:28

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 7, 2018

roberthbailey assigned roberthbailey and detiber Sep 8, 2018

enxebre mentioned this pull request Sep 11, 2018

Machine api mvp openshift/installer#119

Merged

alvaroaleman mentioned this pull request Sep 14, 2018

Import and enable upstream machineDeployment controller in the machine-controller kubermatic/kubermatic#1843

Closed

detiber mentioned this pull request Sep 25, 2018

Update apiserver-builder from 1.9, or switch to CRDs? #409

Closed

pwittrock force-pushed the master branch from 1246c3f to 69d8dfe Compare September 27, 2018 21:43

roberthbailey changed the title ~~WIP: DO NOT MERGE: Migrate to kubebuilder~~ Migrate to kubebuilder Sep 27, 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 27, 2018

roberthbailey mentioned this pull request Sep 27, 2018

Migrate to CRDs (and kubebuilder) kubernetes-sigs/cluster-api-provider-gcp#37

Merged

scruplelesswizard reviewed Sep 28, 2018

View reviewed changes

enxebre mentioned this pull request Sep 28, 2018

update apisever image openshift/machine-api-operator#82

Merged

pwittrock force-pushed the master branch from 55c5a2e to b233080 Compare September 28, 2018 16:37

xmudrii reviewed Sep 28, 2018

View reviewed changes

pwittrock force-pushed the master branch from b233080 to aac691b Compare September 28, 2018 17:06

scruplelesswizard reviewed Sep 28, 2018

View reviewed changes

pwittrock commented Sep 28, 2018

View reviewed changes

xmudrii approved these changes Oct 4, 2018

View reviewed changes

roberthbailey mentioned this pull request Oct 4, 2018

Remove references to kubeadm specific locations for kubeconfig. #492

Closed

pwittrock added 15 commits October 4, 2018 16:13

Change vendored deps to use controller-runtime libs.

0698f96

Migrate API definitions to CRDs.

1d2e1d2

Migrate controllers to controller-runtime libs.

a6e0d25

Rename clusterctl and move to dynamic client.

7e83691

delete obselete files

92ade58

Add manager binary for running the controllers.

b980277

Migrate pkg/util to dynamic client

551e692

Provide kustomization.yaml for installing the APIs.

c60af0d

Update scripts for testing on prow.

adeee5b

Update project related files for kubebuilder.

7324888

Address PR comments

899d3b5

Address PR comments

33ff478

delete install target from Makefile

72cc558

Delete unused logging code

67f0e08

Make verify boilerplate work.

1ade72a

pwittrock force-pushed the master branch from d4a3d43 to 1ade72a Compare October 4, 2018 23:18

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

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

k8s-ci-robot merged commit 80129c0 into kubernetes-sigs:master Oct 4, 2018

scruplelesswizard mentioned this pull request Oct 8, 2018

Migrate to CRD's kubernetes-sigs/cluster-api-provider-openstack#32

Closed

frodenas mentioned this pull request Oct 8, 2018

Migrate to Kubebuilder kubernetes-sigs/cluster-api-provider-vsphere#66

Closed

xmudrii mentioned this pull request Jan 3, 2019

Migrate to CRDs, Kubebuilder and Kustomize kubernetes-sigs/cluster-api-provider-digitalocean#126

Merged

7 tasks

roberthbailey mentioned this pull request Jan 24, 2019

Improve help text for 'kubeconfig', 'namespace', and 'cluster' in client-go #397

Closed

yuwenma mentioned this pull request Mar 21, 2019

KEP: Rebase k8s images to distroless kubernetes/enhancements#900

Merged

chingchiaw mentioned this pull request May 30, 2019

MD controller sync getNewMachineSet should not return nil as newMS after updating existingNewMS #970

Closed

		@@ -128,15 +122,16 @@ type MachineStatus struct {
		// can be added as events to the Machine object and/or logged in the

		@@ -128,9 +124,7 @@ type MachineSetStatus struct {
		ErrorMessage *string `json:"errorMessage,omitempty"`

Migrate to kubebuilder #494

Migrate to kubebuilder #494

Conversation

pwittrock commented Sep 7, 2018

roberthbailey commented Sep 27, 2018

pwittrock commented Sep 28, 2018

scruplelesswizard left a comment • edited Loading

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

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

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

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

xmudrii left a comment

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

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

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

pwittrock commented Sep 28, 2018

pwittrock commented Sep 28, 2018

tamalsaha commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scruplelesswizard Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scruplelesswizard Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scruplelesswizard commented Sep 28, 2018

pwittrock commented Sep 28, 2018

pwittrock commented Sep 28, 2018

Choose a reason for hiding this comment

liztio commented Sep 28, 2018

xmudrii left a comment

Choose a reason for hiding this comment

roberthbailey commented Oct 4, 2018

k8s-ci-robot commented Oct 4, 2018

pwittrock commented Oct 5, 2018

scruplelesswizard left a comment •

edited

Loading

scruplelesswizard Sep 28, 2018 •

edited

Loading

scruplelesswizard Sep 28, 2018 •

edited

Loading