Initial version of clusterctl create #219

jessicaochen · 2018-05-28T00:24:48Z

What this PR does / why we need it:
First version of clusterctl create. There is plenty room to iterate and improve (as indicated by the TODOs). However, I think it is beneficial to get a functional initial version down so iteration and improvement can happen in parallel.
#157

Note that apiserver podspec is captured from commit a1ea634 (#153).
Special notes for your reviewer:
The code for generating the gcp example files for this cluster create command is coming in the next PR.

Release note:

@kubernetes/kube-deploy-reviewers

k8s-ci-robot · 2018-05-28T00:24:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jessicaochen

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~~ [jessicaochen]

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

jessicaochen · 2018-05-28T00:25:25Z

/assign @k4leung4 @mkjelland

k4leung4 · 2018-05-29T15:23:48Z

clusterctl/cmd/create_cluster.go

-	VmDriver string
+	VmDriver               string
+	Provider               string
+	KubeonfigOutput        string


KubeConfigOutput?

k4leung4 · 2018-05-29T15:27:53Z

clusterctl/clusterdeployer/clusterapiserver.go

@@ -0,0 +1,334 @@
+/*
+Copyright 2017 The Kubernetes Authors.


k4leung4 · 2018-05-29T15:32:17Z

clusterctl/clusterdeployer/clusterapiserver.go

+	return string(tmplBuf.Bytes()), nil
+}
+
+const ClusterAPIAPIServerConfigTemplate = `


consider moving the template into a file by itself?
will make it obvious when someone is tweaking the template vs changing code

k4leung4 · 2018-05-29T15:33:18Z

clusterctl/clusterdeployer/clusterclient.go

@@ -0,0 +1,211 @@
+package clusterdeployer


missing license

k4leung4 · 2018-05-29T15:36:44Z

clusterctl/clusterdeployer/clusterclient.go

+func (c *clusterClient) GetClusterObjects() ([]*clusterv1.Cluster, error) {
+	clusters := []*clusterv1.Cluster{}
+	// TODO: Iterate over all namespaces where we could have Cluster API Objects
+	clusterlist, err := c.clientSet.ClusterV1alpha1().Clusters(apiv1.NamespaceDefault).List(metav1.ListOptions{})


there is a apiv1.NamespaceAll if you do want to list all clusters in all namespaces.

*Acutally punted with issue labeled.

k4leung4 · 2018-05-29T15:39:39Z

clusterctl/clusterdeployer/clusterclient.go

+	cluster.Status.APIEndpoints = append(cluster.Status.APIEndpoints,
+		clusterv1.APIEndpoint{
+			Host: masterIP,
+			Port: 443,


consider making 443 const

k4leung4 · 2018-05-29T15:40:48Z

clusterctl/clusterdeployer/clusterclient_test.go

@@ -0,0 +1,4 @@
+package clusterdeployer


check license for all files

i meant copyright

k4leung4 · 2018-05-29T15:51:58Z

clusterctl/clusterdeployer/clusterdeployer.go

+		}
+	}()
+
+	glog.V(2).Info("Applying Cluster API APIServer to external cluster.")


while the code in this method is relatively straightforward, there is a lot to keep track of.
does it make sense to add helper methods for creating objects with the external client one for the internal client?

Broke it out as much as I could while preserving flow.

k4leung4 · 2018-05-29T15:53:47Z

clusterctl/clusterdeployer/clusterdeployer.go

+	}
+
+	for _, cluster := range clusters {
+		cluster.SetResourceVersion("")


can you comment on why it is necessary to set the resource version to an empty string

k4leung4 · 2018-05-29T15:54:36Z

clusterctl/clusterdeployer/clusterdeployer_test.go

@@ -4,12 +4,18 @@ import (
 	"fmt"


run gofmt to order imports

spew · 2018-05-29T17:01:09Z

clusterctl/clusterdeployer/clusterclient.go

+		return nil, err
+	}
+	defer f.Close()
+	f.WriteString(kubeconfig)


Missing the error check for writing the string to the file on line 31.

Recommend switching from WriteString and using ioutil.WriteFile(...)

There is a subtle bug here where it is possible that the Write on line 31 was not flushed to disk and then on line 32 the contents of the file are being used. If ioutil.WriteFile(...) is used then file handle it opens is guaranteed to be closed.

spew · 2018-05-29T17:10:19Z

clusterctl/clusterdeployer/clusterapiserver.go

+	return certParams, nil
+}
+
+func GetApiServerYaml() (string, error) {


This public method should have associated test(s).

spew · 2018-05-29T17:14:57Z

clusterctl/clusterdeployer/clusterdeployer.go

+	glog.V(1).Info("Getting internal cluster kubeconfig.")
+	internalKubeconfig, err := waitForKubeconfigReady(d.provider, master)
+	if err != nil {
+		return fmt.Errorf("unable to get internal cluster kbueconfig: %v", err)


typo: kbueconfig -> kubeconfig

spew · 2018-05-29T17:16:52Z

clusterctl/clusterdeployer/clusterdeployer.go

+	glog.V(2).Infof("Creating master %v", master.Name)
+	err = externalClient.CreateMachineObjects([]*clusterv1.Machine{master})
+	if err != nil {
+		return fmt.Errorf("unable to create master machine:%v", err)


typo: missing space after 'machine:'

spew · 2018-05-29T17:17:09Z

clusterctl/clusterdeployer/clusterdeployer.go

+	// Fetch freshly created master.
+	updatedMachines, err := externalClient.GetMachineObjects()
+	if err != nil {
+		return fmt.Errorf("unable to fetch machines:%v", err)


typo: missing space after 'machine:'

spew · 2018-05-29T17:17:20Z

clusterctl/clusterdeployer/clusterdeployer.go

+	}
+	master, _, err = splitMachineRoles(updatedMachines)
+	if err != nil {
+		return fmt.Errorf("unable to fetch master machine:%v", err)


typo: missing space after 'machine:'

spew · 2018-05-29T17:23:15Z

clusterctl/clusterdeployer/clusterdeployer.go

+
+func (d *ClusterDeployer) writeKubeconfig(kubeconfig string) error {
+	os.Remove(d.kubeconfigOutput)
+	f, err := os.Create(d.kubeconfigOutput)


This function, lines 223 -> 229, can be replaced / simplified with:

return ioutil.WriteFile(d.kubeconfigOutput, []byte(kubeconfig), 0666

which is functionally equivalent (os.Create / ioutil.WriteFile both truncuate).

spew · 2018-05-29T17:31:22Z

clusterctl/clusterdeployer/clusterclient.go

+		if err != nil {
+			return err
+		}
+		err = waitForMachineReady(c.clientSet, createdMachine)


This could be parallelized and It would be faster, if the it were to first create all machine objects. After the creationg loop completed it could then call waitForMachineReady(...) on each object.

Punted as a post v0 optimization.

Can you add a TODO into the code?

mkjelland · 2018-05-30T22:44:56Z

clusterctl/clusterdeployer/clusterdeployer.go

+	defer func() {
+		err := externalClient.Close()
+		if err != nil {
+			glog.Error("Could not close external client")


Add err value to error message

mkjelland · 2018-05-30T22:48:47Z

clusterctl/clusterdeployer/clusterdeployer.go

+	defer func() {
+		err := internalClient.Close()
+		if err != nil {
+			glog.Error("Could not close internal client")


Add err value to error message

mkjelland · 2018-05-30T23:00:56Z

clusterctl/clusterdeployer/clusterdeployer.go

+}
+
+func (d *ClusterDeployer) applyClusterAPIApiserver(client ClusterClient) error {
+	yaml, err := getApiServerYaml()


Nit: I would prefer if this was called getClusterApiServerYaml since it could mean the core api server.

I think whether it uses yaml under the covers or something else is a implementation detail. I was hoping the clusterapi + apiserver in the method name would make it clear it is the cluster api apiserver as opposed to the main apiserver. Is this ok or do you have other ideas on how do distinguish between the main apiserver and the clusterapi apiserver for the method name?

Ok, that makes sense!

mkjelland · 2018-05-30T23:03:47Z

clusterctl/clusterdeployer/clusterdeployer.go

+	CreateClusterObject(*clusterv1.Cluster) error
+	CreateMachineObjects([]*clusterv1.Machine) error
+	UpdateClusterObjectEndpoint(string) error
+	Close() error


Eventually, will there be Delete methods for Machine and Cluster? Is that out of scope for clusterctl?

Deletion is in scope for clusterctl.

If that ability is added to the ClusterDeployer, it would make sense to extend the ClusterClient interface to handle the needed methods at that point.

roberthbailey · 2018-05-31T15:05:52Z

clusterctl/README.md

@@ -22,13 +22,10 @@ $ go build



Can you add a TBD here so that it's clear that the limitations section isn't filled out? Right now it is easy to think that Creating a cluster falls in the limitations section.

roberthbailey · 2018-05-31T15:07:20Z

clusterctl/clusterdeployer/clusterapiserver.go

+	"k8s.io/client-go/util/cert/triple"
+)
+
+var apiServerImage = "gcr.io/k8s-cluster-api/cluster-apiserver:0.0.3"


This is out of date compared to head. I think the current version is 0.0.5 or so.

I know. This is snapshot-ed to the version called out in the PR description. My plan is to move roll forward after landing this v0 PR. There is a lot of development in this repo (a good thing :) ) and I think it is better to land something working and improve it than hold off trying to keep up with all the development.

roberthbailey · 2018-05-31T15:07:39Z

clusterctl/clusterdeployer/clusterapiserver.go

+var apiServerImage = "gcr.io/k8s-cluster-api/cluster-apiserver:0.0.3"
+
+func init() {
+	if img, ok := os.LookupEnv("CLUSTER_API_SERVER_IMAGE"); ok {


Why an env var instead of a flag?

It is what we have been doing so far in this repo. We can change it to a flag post v0 if folks really prefer a flag over a environment variable for a development hook.

roberthbailey · 2018-05-31T15:08:37Z

clusterctl/clusterdeployer/clusterapiservertemplate.go

+    api: clusterapi
+    apiserver: "true"
+---
+apiVersion: apps/v1beta1


Deployments should use v1 instead of v1beta1.

This is snapshot-ed to the version called out in the PR description. I want to check it in with a consistent working snapshot so we know what changes it is caught up with and what changes it has yet to catch up on.

roberthbailey · 2018-05-31T15:09:03Z

clusterctl/clusterdeployer/clusterapiservertemplate.go

+  name: default
+  namespace: default
+---
+apiVersion: apps/v1beta1


This is snapshot-ed to the version called out in the PR description. I want to check it in with a consistent working snapshot so we know what changes it is caught up with and what changes it has yet to catch up on.

roberthbailey · 2018-05-31T15:10:13Z

clusterctl/clusterdeployer/clusterclient.go

+		if err != nil {
+			return err
+		}
+		err = waitForMachineReady(c.clientSet, createdMachine)


Can you add a TODO into the code?

k4leung4 · 2018-05-31T15:25:14Z

clusterctl/README.md

-**NOT YET SUPPORTED!** - Use [provider-specific deployer](../README.md) to create clusters till cluster creation is supported.
-
-1. Create a `cluster.yaml` and `machines.yaml` files configured for your cluster. See the provider specific templates and generation tools at `$GOPATH/src/sigs.k8s.io/cluster-api/clusterctl/examples/<provider>`. 
+1. Create a `cluster.yaml`, `machines.yaml` and `provider-components.yaml` files configured for your cluster. See the provider specific templates and generation tools at `$GOPATH/src/sigs.k8s.io/cluster-api/clusterctl/examples/<provider>`. 


did you intend to include the example provider-components.yaml in this PR?

It is imminently coming in the next PR.

k4leung4 · 2018-05-31T17:54:28Z

lgtm

mkjelland · 2018-05-31T18:05:03Z

/lgtm

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

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 28, 2018

k8s-ci-robot assigned k4leung4 and mkjelland May 28, 2018

k4leung4 suggested changes May 29, 2018

View reviewed changes

spew reviewed May 29, 2018

View reviewed changes

jessicaochen force-pushed the cluster_create_v0 branch from 3cb2772 to f8a3d18 Compare May 30, 2018 22:26

mkjelland reviewed May 30, 2018

View reviewed changes

jessicaochen force-pushed the cluster_create_v0 branch from f8a3d18 to 2b016a2 Compare May 31, 2018 05:42

roberthbailey reviewed May 31, 2018

View reviewed changes

k4leung4 reviewed May 31, 2018

View reviewed changes

Initial version of clusterctl create

c36f757

jessicaochen force-pushed the cluster_create_v0 branch from 2b016a2 to c36f757 Compare May 31, 2018 16:30

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

k8s-ci-robot merged commit cf9bcf6 into kubernetes-sigs:master May 31, 2018

jessicaochen mentioned this pull request May 31, 2018

Working vsphere clusterctl example #263

Merged

Initial version of clusterctl create #219

Initial version of clusterctl create #219

Conversation

jessicaochen commented May 28, 2018

k8s-ci-robot commented May 28, 2018

jessicaochen commented May 28, 2018

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

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

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

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

k4leung4 commented May 31, 2018

mkjelland commented May 31, 2018