Migrate to CRDs (and kubebuilder) #37

roberthbailey · 2018-09-27T22:57:29Z

What this PR does / why we need it: Picks up kubernetes-sigs/cluster-api#494 and migrates the GCP provider repository to use CRDs and kubebuilder to stay in line with the cluster-api repository.

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 #

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:

The GCP Cluster API provider now uses CRDs instead of an extension apiserver.

k8s-ci-robot · 2018-09-27T22:58:11Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

I've verified that this works following the (updated) instructions in the README:

$ ./bin/clusterctl create cluster --provider google -c cmd/clusterctl/examples/google/out/cluster.yaml -m cmd/clusterctl/examples/google/out/machines.yaml -p cmd/clusterctl/examples/google/out/provider-components.yaml -a cmd/clusterctl/examples/google/out/addons.yaml
I0927 15:35:45.038086    3802 clusterdeployer.go:94] Creating bootstrap cluster
I0927 15:38:03.991785    3802 clusterdeployer.go:111] Applying Cluster API stack to bootstrap cluster
I0927 15:38:03.991805    3802 clusterdeployer.go:300] Applying Cluster API Provider Components
I0927 15:38:04.554027    3802 clusterdeployer.go:116] Provisioning target cluster via bootstrap cluster
I0927 15:38:04.554048    3802 clusterdeployer.go:118] Creating cluster object test1-r5dcu on bootstrap cluster in namespace "default"
I0927 15:38:04.559822    3802 clusterdeployer.go:123] Creating master  in namespace "default"
I0927 15:39:34.595833    3802 clusterdeployer.go:128] Updating bootstrap cluster object for cluster test1-r5dcu in namespace "default" with master () endpoint
I0927 15:39:35.289560    3802 clusterdeployer.go:133] Creating target cluster
I0927 15:40:31.664412    3802 clusterdeployer.go:140] Applying Cluster API stack to target cluster
I0927 15:40:31.664430    3802 clusterdeployer.go:309] Applying Cluster API Provider Components
I0927 15:41:14.033701    3802 clusterdeployer.go:314] Pivoting Cluster API objects from bootstrap to target cluster.
I0927 15:41:14.323743    3802 clusterdeployer.go:368] Moved Cluster 'test1-r5dcu'
I0927 15:41:14.423718    3802 clusterdeployer.go:408] Moved Machine 'gce-master-5pps7'
I0927 15:41:14.423743    3802 clusterdeployer.go:145] Saving provider components to the target cluster
I0927 15:41:14.723028    3802 clusterdeployer.go:158] Updating target cluster object with master () endpoint
I0927 15:41:15.139619    3802 clusterdeployer.go:163] Creating node machines in target cluster.
I0927 15:42:45.278062    3802 clusterdeployer.go:169] Creating addons in target cluster.
I0927 15:42:46.684638    3802 clusterdeployer.go:175] Done provisioning cluster. You can now access your cluster with kubectl --kubeconfig kubeconfig
I0927 15:42:46.686093    3802 clusterdeployer.go:223] Cleaning up bootstrap cluster.

Tests pass locally, but we need to update prow to use go 1.10+ and add some of the kubebuilder binaries before CI tests will pass.

roberthbailey · 2018-09-28T04:26:53Z

/test pull-cluster-api-provider-gcp-test

- Parse flags - Add cluster-api Scheme - Initialize deps

- Issue with serializer / deserializer - Issue with registering actuator multiple times

…ock/cluster-api.

removed and be sourced from the vendored kubebuilder code.

timothysc · 2018-10-01T18:50:02Z

@roberthbailey - I'm just trying to figure out how we consume downstream :-/

roberthbailey · 2018-10-05T20:05:15Z

@timothysc and I chatted on slack about his question. Unfortunately, I don't see any easy way for other providers to pick up this change. Each one will need to update deps and migrate to kubebuilder following the same steps. The good news is that the steps aren't too complicated.

justinsb

A few suggestions, but nothing that should delay this PR further..

justinsb · 2018-10-05T18:54:25Z

README.md

   ./generate-yaml.sh
-   cd ../..
+   cd ../../../..
+   kustomize build config/default/ > cmd/clusterctl/examples/google/out/provider-components.yaml


Yay for more kustomize and less bash :-)

justinsb · 2018-10-05T18:59:36Z

cmd/manager/main.go

+)
+
+func main() {
+	flag.Parse()


I always like to do flag.Set("logtostderr", "true"), so don't be surprised to see a PR for that soon!

This file was generated by kubebuilder - so send it upstream (and then everyone will get the change).

justinsb · 2018-10-05T19:13:41Z

pkg/apis/gceproviderconfig/v1alpha1/gcemachineproviderconfig_types.go

+	Zone        string `json:"zone"`
+	MachineType string `json:"machineType"`
+
+	// The name of the OS to be installed on the machine.


This is matching the yaml file, rather than actually installing the OS... so I'm wondering if we should just make it key value pairs. That might actually be generic enough for the Machine type itself. Probably best discussed at the next cluster API meeting!

We can discuss these types separately; for this PR they were just copied from the existing types.

justinsb · 2018-10-05T19:24:06Z

pkg/cloud/google/clusteractuator.go

+		SourceTags: []string{cluster.Name + "-worker"},
+	})
+	if err != nil {
+		glog.Warningf("Error creating firewall rule for internal cluster traffic: %v", err)


Just warning feels like we will bury bad news in the logs, and users won't understand why their cluster didn't launch. return error instead?

justinsb · 2018-10-05T19:24:39Z

pkg/cloud/google/clusteractuator.go

+		SourceRanges: []string{"0.0.0.0/0"},
+	})
+	if err != nil {
+		glog.Warningf("Error creating firewall rule for core api server traffic: %v", err)


Same concern here, but will stop commenting on this class of problem for brevity :-)

justinsb · 2018-10-05T21:07:40Z

pkg/cloud/google/instancestatus.go

+// Sets the status of the instance identified by the given machine to the given machine
+func (gce *GCEClient) updateInstanceStatus(machine *clusterv1.Machine) error {
+	if gce.client == nil {
+		return nil


I feel this should be an error(?)

justinsb · 2018-10-05T21:08:18Z

pkg/cloud/google/instancestatus.go

+
+// Gets the state of the instance stored on the given machine CRD
+func (gce *GCEClient) machineInstanceStatus(machine *clusterv1.Machine) (instanceStatus, error) {
+	if machine.ObjectMeta.Annotations == nil {


Nit: I think this check could be ommitted, because a nil map is treated the same as an empty map for reading

justinsb · 2018-10-05T21:12:18Z

pkg/cloud/google/machineactuator.go

+
+// The two machines differ in a way that requires an update
+func (gce *GCEClient) requiresUpdate(a *clusterv1.Machine, b *clusterv1.Machine) bool {
+	// Do not want status changes. Do want changes that impact machine provisioning


Nit: I would zero-out the fields you don't care about (after copying!); my fear is that someone adds a field that we do care about and we get hard-to-figure out bugs.

justinsb · 2018-10-05T21:15:58Z

pkg/cloud/google/machineactuator.go

+
+	// Otherwise, fall back to the base image.
+	glog.Infof("Could not find image at %s. Defaulting to %s.", img, defaultImg)
+	return defaultImg


Should this be an error?

justinsb · 2018-10-05T21:18:54Z

pkg/cloud/google/metadata.go

+
+const nodeEnvironmentVars = `
+#!/bin/bash
+KUBELET_VERSION={{ .Machine.Spec.Versions.Kubelet }}


Idea: we should move this to building in code from a map (no template), then we can have some sanity checks around quoting or invalid characters

I agree that this should be tidied up.

justinsb · 2018-10-05T21:22:39Z

Looks great @roberthbailey - a few suggestions, but it sounds like the community needs an existence proof :-)

/lgtm

roberthbailey · 2018-10-06T07:24:26Z

Can you send a PR to fix the issues that you found?

k8s-ci-robot added 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 Sep 27, 2018

roberthbailey assigned justinsb Sep 27, 2018

k8s-ci-robot requested review from justinsb and timothysc September 27, 2018 22:58

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

roberthbailey mentioned this pull request Sep 29, 2018

Be explicit that minikube <= 0.28.0 is required for now. #39

Merged

xmudrii mentioned this pull request Oct 1, 2018

Migrate to CRDs kubernetes-sigs/cluster-api-provider-digitalocean#83

Closed

5 tasks

roberthbailey and others added 16 commits October 1, 2018 04:56

New kubebuilder project.

a440c90

Move packages around so that code is all under pkg or cmd.

96d35a6

Vendor updates.

d901db7

Getting code to compile; one test commented out.

aabbc91

Update with latest cluster-api fork

9168328

Fix compile errors; remove dependency on typed generated client.

2a906d5

Update scripts for running CI automation.

4c93286

Ignore the bin directory.

b882abf

Update vendored cluster-api version

f30e7be

Update kustomization.yaml to work with output from clusterctl

ea8c9c1

Fixes to manager main

5c6170c

- Parse flags - Add cluster-api Scheme - Initialize deps

update package name for clusterctl

c2a4c34

Fix minor issues in actuators

ae59c8d

- Issue with serializer / deserializer - Issue with registering actuator multiple times

clusterctl now works with machines / clusters defined as CRDs.

aec5b36

Update cluster-api dependency to the tip of https://github.com/pwittr…

25ae0ae

…ock/cluster-api.

Add a script to fetch k8s binaries. In the future this should be

1b9dced

removed and be sourced from the vendored kubebuilder code.

roberthbailey force-pushed the kubebuilder branch from d48e0c5 to 1b9dced Compare October 1, 2018 11:57

Depend on master branch of cluster api instead of pwittrock fork.

19b99cc

justinsb approved these changes Oct 5, 2018

View reviewed changes

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

k8s-ci-robot merged commit f3145d8 into kubernetes-sigs:master Oct 5, 2018

Lion-Wei mentioned this pull request Oct 8, 2018

Reorganize code to conform to what is in skeleton repo kubernetes-sigs/cluster-api-provider-openstack#8

Closed

dlipovetsky mentioned this pull request Oct 27, 2018

Update k8s.sigs.io/cluster-api source to upstream #48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to CRDs (and kubebuilder) #37

Migrate to CRDs (and kubebuilder) #37

roberthbailey commented Sep 27, 2018

k8s-ci-robot commented Sep 27, 2018

roberthbailey commented Sep 27, 2018

roberthbailey commented Sep 28, 2018

timothysc commented Oct 1, 2018

roberthbailey commented Oct 5, 2018

justinsb left a comment

justinsb Oct 5, 2018

justinsb Oct 5, 2018

roberthbailey Oct 6, 2018

justinsb Oct 5, 2018

roberthbailey Oct 6, 2018

justinsb Oct 5, 2018

justinsb Oct 5, 2018

justinsb Oct 5, 2018

justinsb Oct 5, 2018

justinsb Oct 5, 2018

justinsb Oct 5, 2018

justinsb Oct 5, 2018

roberthbailey Oct 6, 2018

justinsb commented Oct 5, 2018

roberthbailey commented Oct 6, 2018

Migrate to CRDs (and kubebuilder) #37

Migrate to CRDs (and kubebuilder) #37

Conversation

roberthbailey commented Sep 27, 2018

k8s-ci-robot commented Sep 27, 2018

roberthbailey commented Sep 27, 2018

roberthbailey commented Sep 28, 2018

timothysc commented Oct 1, 2018

roberthbailey commented Oct 5, 2018

justinsb 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Oct 5, 2018

roberthbailey commented Oct 6, 2018