For internal review only #41

luomiao · 2017-04-10T23:53:36Z

DON'T MERGE!

This is a review for vSphere support code in kops before upstreaming.
Please refer the following guide for an easier review :)

For those who never use or read about kops before, the following docs might be helpful for a high-level understanding:
https://github.com/kubernetes/kops/blob/master/docs/development/how_it_works.md
https://github.com/kubernetes/kops/blob/master/docs/philosophy.md
About how to use kops with AWS:
https://github.com/kubernetes/kops/blob/master/docs/aws.md
About how to use kops on vSphere:
https://github.com/vmware/kops/blob/vsphere-develop/docs/development/vsphere-dev.md

The changes to support vSphere mainly include 5 parts:

Cloud Specs: add vSphere as one of the Cloud Providers. Currently AWS and GCE are the only two Cloud Providers in kops.
Cloudup: Cloudup is the entry of kops core logics. Cloudup created builders for different models and include tasks into models. pkg/model/vspheremodel and upup/pkg/fi/cloudup/vspheretasks to initialize vSphere specific tasks, including clone VMs/poweron VMs/AttachISOs, and so on.
Nodeup: Nodeup is a binary will be started on each nodes. It's used to bootstrap the whole k8s cluster. It will also start protokube.
Protokube: Protokube is a docker container started at the very beginning to ensure the environment of kubelet is ready. More introduction about protokube can be found here.
DNS: We use CoreDNS. The steps to setup CoreDNS can be found in here. In order to use CoreDNS, changes are needed for Nodeup/Protokube and DNS Controller addon.

Accept vSphere's server, datacenter, cluster setting by flags "vsphere-server", "vsphere-datacenter", and "vsphere-resource-pool". Username and password can be set by environment variables: "VSPHERE_USERNAME" and "VSPHERE_PASSWORD".

1. Check the emptiness of VSPHERE_USERNAME and VSPHERE_PASSWORD 2. Move vSphere specific fields from clusterSpec to clusterSpec.CloudConfig

…ed conversion files. To make sure the vSphere fields in CloudConfiguration can be successfully serialized and output into config file.

Implemented CreateLinkClonedVM cloud interface to create a link cloned VM from a template VM. The code checks if the template VM has a snapshot, if no it creates it before creating a link cloned VM. If snapshot already exists, it uses it to create the link cloned VM. Testing done: 1. kops cluster create goes through fine and creates the link cloned VM for the master and worker. Verified that it creates the snapshot on the template VM if it does not exists before creating a link cloned VM. In case the snapshot exists, it uses it to create the link cloned VM. 2. "make ci" is successful.

…akefile. (#1) * Add vSphere volumes to protokube. Update vSphere testing doc and makefile. * Updated vsphere_volume to get correct IP. Addressed comments.

* Add support of CoreDNS for vSphere provider. * Add instructions about how to setup CoreDNS for vSphere provider. * Address comments for CoreDNS support code.

AttachISO task creates the user-data/meta-data cloud init files and creates cloud-init.iso file using "genisoimage" tool. It then uploads it to the datastore where the master/worker VM resides and inserts it into the cd-rom device of the master/worker VM. When the master/worker VM powers on, the cloud-init package in it runs the bootstrap script that downloads nodeup and runs it. Also removed redundant VirtualMachineModelBuilder that does nothing. Testing done: 1. Tested end to end that the master and worker VMs executes the cloud-init script successfully. 2, "make ci" is successful.

* Enable CoreDNS in nodeup/protokube. * Address comments.

…vSphere VM (#11) * Added code to enable nodeup and protokube building and execution for vSphere VM. * Fixed nodeup template for vSphere.

…rovider

…Provider

Add vm-uuid in cloud-config file required by Kubernetes vSphere CloudProvider

pdhamdhere

Looks solid for first draft. Terrific work everyone!

Some minor comments, mostly clarifying questions. We can discuss in-person as well.

Few general suggestions:

Add file header comments with purpose of code in that file.
Add function header comments for exported functions (that will be called from outside)
Did we run golint?

pdhamdhere · 2017-04-13T02:49:16Z

cmd/kops/create_cluster.go

@@ -202,6 +213,14 @@ func NewCmdCreateCluster(f *util.Factory, out io.Writer) *cobra.Command {
 	cmd.Flags().StringVar(&options.MasterTenancy, "master-tenancy", options.MasterTenancy, "The tenancy of the master group on AWS. Can either be default or dedicated.")
 	cmd.Flags().StringVar(&options.NodeTenancy, "node-tenancy", options.NodeTenancy, "The tenancy of the node group on AWS. Can be either default or dedicated.")

+	if featureflag.VSphereCloudProvider.Enabled() {
+		// vSphere flags
+		cmd.Flags().StringVar(&options.VSphereServer, "vsphere-server", options.VSphereServer, "vsphere-server is required for vSphere. Set vCenter URL Ex: 10.192.10.30 or myvcenter.io (without https://)")


Port? I believe we had customer using kube-anywhere with vCenter running on different port. CHeck with Divyen.

No the vCenter port is not necessary. And it has been deprecated after this PR:
https://github.com/kubernetes/kubernetes/pull/42024/files
Even without the port there is no problem to talk to vCenter.
The kube-anywhere readme should be updated too.

pdhamdhere · 2017-04-13T02:51:33Z

cmd/kops/create_cluster.go

+		cmd.Flags().StringVar(&options.VSphereDatacenter, "vsphere-datacenter", options.VSphereDatacenter, "vsphere-datacenter is required for vSphere. Set the name of the datacenter in which to deploy Kubernetes VMs.")
+		cmd.Flags().StringVar(&options.VSphereResourcePool, "vsphere-resource-pool", options.VSphereDatacenter, "vsphere-resource-pool is required for vSphere. Set a valid Cluster, Host or Resource Pool in which to deploy Kubernetes VMs.")
+		cmd.Flags().StringVar(&options.VSphereCoreDNSServer, "vsphere-coredns-server", options.VSphereCoreDNSServer, "vsphere-coredns-server is required for vSphere.")
+		cmd.Flags().StringVar(&options.VSphereDatastore, "vsphere-datastore", options.VSphereDatastore, "vsphere-datastore is required for vSphere.  Set a valid datastore in which to store dynamic provision volumes.")


This is optional rt?

The datastore field seems is necessary for v1.5.3 support. @abrarshivani Hi Abrar, can you confirm this?

Current stable versions for vSphere Cloud Provider requires datastore

pdhamdhere · 2017-04-13T02:54:05Z

cmd/kops/create_cluster.go

+			}
+			cluster.Spec.CloudConfig.VSphereCoreDNSServer = fi.String(c.VSphereCoreDNSServer)
+
+			if c.VSphereDatastore == "" {


This is optional rt?

pdhamdhere · 2017-04-13T04:28:12Z

dns-controller/pkg/dns/dnscontroller.go

+		}
+
+		if dnsRecord != nil && string(dnsRecord.Type()) == string(k.RecordType) {
+			glog.V(8).Infof("Found matching record: %s %s", k.RecordType, fqdn)


Move it to just before cs.Remove and change to glog.V(2) and "Deleting resource record: ..." to be consistent with AWS L495

pdhamdhere · 2017-04-13T04:36:18Z

docs/development/vsphere-dev.md

+### Pre-requisites
+ vSphere with at least one ESX, having sufficient free disk space on attached datastore. ESX VM's should have internet connectivity.
+ Setup DNS following steps given in relevant Section above.
+ Create the VM using this template (TBD).


pdhamdhere · 2017-04-13T05:35:08Z

upup/pkg/fi/cloudup/populate_instancegroup_spec.go

-	defaultNodeMachineTypeGCE = "n1-standard-2"
+	defaultNodeMachineTypeAWS     = "t2.medium"
+	defaultNodeMachineTypeGCE     = "n1-standard-2"
+	defaultNodeMachineTypeVSphere = "vsphere_node"


Where is configuration of node defined?

pdhamdhere · 2017-04-13T05:36:13Z

upup/pkg/fi/cloudup/populate_instancegroup_spec.go

+	defaultMasterMachineTypeAWS     = "m3.medium"
+	defaultMasterMachineTypeVSphere = "vsphere_master"
+
+	defaultVSphereNodeImage = "ubuntu_16_04"


I don't see AMI Id for AWS then why do we have vSphereNodeImage here?

@prashima Now we can use --image to define the image to be used. Do we still need defaultVSphereNodeImage?

We need to fix default image name extraction for vSphere. Here is the relevant issue #50.

We can remove this default image from here, but note that that will make '--image' a mandatory flag for vSphere cluster create operation.

pdhamdhere · 2017-04-13T05:40:20Z

hack/vsphere/set_env

+export KOPS_FEATURE_FLAGS=+VSphereCloudProvider
+
+# If set, coredns will be used for vsphere cloud provider.
+export VSPHERE_DNS=coredns


You already have CLI option "--dns=private --vsphere-coredns-server" so do we still need these environment variable?

pdhamdhere · 2017-04-13T05:41:03Z

upup/pkg/fi/cloudup/vsphere/vsphere_cloud.go

+
+func (c *VSphereCloud) DNS() (dnsprovider.Interface, error) {
+	// TODO: this is a temporary flag to toggle between CoreDNS and Route53, before CoreDNS is stable
+	dns_provider := os.Getenv("VSPHERE_DNS")


We pass this info to KOPS CLI rt? Why do we need env variable?

pdhamdhere · 2017-04-13T05:45:30Z

upup/pkg/fi/cloudup/vspheretasks/attachiso.go

+	lines = append(lines, "       echo \"nameserver "+dnsHost+"\" >> /etc/resolvconf/resolv.conf.d/head")
+	lines = append(lines, "       resolvconf -u")
+	dnsUpdateStr := strings.Join(lines, "\n")
+	data = strings.Replace(data, "$DNS_SCRIPT", dnsUpdateStr, -1)


Use sprintf or equivalent rather than strings.XXX???

luomiao · 2017-04-14T00:15:41Z

Makefile

+# Build protokube and nodeup. Pushes protokube to DOCKER_REGISTRY and scp nodeup binary to the TARGET. Uncomment ssh part to run nodeup at set TARGET.
+push-vsphere: nodeup protokube-push
+	scp -C .build/dist/nodeup  ${TARGET}:~/
+	# ssh -t [email protected] 'export AWS_ACCESS_KEY_ID="AKIAJ6UOKB6BGYLIQJEQ"; export AWS_SECRET_ACCESS_KEY="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"; export AWS_REGION="us-west-2"; SKIP_PACKAGE_UPDATE=1 ~/nodeup --conf=/var/cache/kubernetes-install/kube_env.yaml --v=8'


This "push-vsphere" is not needed anymore rt? @prashima

Address review comments and add installing instructions

When Environment variable S3_ENDPOINT is not empty, kops will use the bucket on this specific s3 endpoint, instead of using AWS S3 by default.

Support user-defined s3 endpoint

Implements delete cluster and toolbox dumper command

Change vm_uuid file location

Change the default image name for vsphere provider and other minor fix

SandeepPissay and others added 24 commits April 10, 2017 17:12

vsphere initial support.

29ab209

Sample vsphere tasks added

1c5feb9

Fixed the formatting and added copyright headers to new files

5e74f2c

Add more supports for vsphere.

b611357

Accept vSphere's server, datacenter, cluster setting by flags "vsphere-server", "vsphere-datacenter", and "vsphere-resource-pool". Username and password can be set by environment variables: "VSPHERE_USERNAME" and "VSPHERE_PASSWORD".

Address comments.

d32b174

1. Check the emptiness of VSPHERE_USERNAME and VSPHERE_PASSWORD 2. Move vSphere specific fields from clusterSpec to clusterSpec.CloudConfig

Fix the type of vSphere fields in CloudConfig and update auto-generat…

83a9aaf

…ed conversion files. To make sure the vSphere fields in CloudConfiguration can be successfully serialized and output into config file.

Implemented VM power on (#7)

270bb10

Added vSphere volumes to protokube, updated vSphere testing doc and m…

659eb42

…akefile. (#1) * Add vSphere volumes to protokube. Update vSphere testing doc and makefile. * Updated vsphere_volume to get correct IP. Addressed comments.

Use CoreDNS for vSphere provider (#2)

bc4fc60

* Add support of CoreDNS for vSphere provider. * Add instructions about how to setup CoreDNS for vSphere provider. * Address comments for CoreDNS support code.

Enable CoreDNS in nodeup/protokube (#6)

a535fa4

* Enable CoreDNS in nodeup/protokube. * Address comments.

Support for vSphere Cloud Provider

bf3f9bb

Fixed cdrom to be connected during startup (#8)

9319c4e

Add CoreDNS support into DNS controller addon image.

699c2bd

Add DNS server into /etc/resolv.conf by cloud-init.

d93cf19

Added code to enable nodeup and protokube building and execution for …

59e18f3

…vSphere VM (#11) * Added code to enable nodeup and protokube building and execution for vSphere VM. * Fixed nodeup template for vSphere.

Fixed environment variable setting process for vSphere. (#16)

059ba9e

Update readme to include command to launch cluster for vSphere CloudP…

c30f6a3

…rovider

Added support for multiple workers for vSphere.

9d77b41

Minor fix due to rebasing with upstream.

7d9fd5a

Added featureflag for vSphere cloud. Fixes #26. (#29)

81db287

Support for vSphere Cloud Provider < v1.5.3

8ff06ec

Add enablediskuuid extraconfig in VM spec

da16a9b

msterin force-pushed the vsphere-develop branch from 22063fe to da16a9b Compare April 11, 2017 00:31

Minor fix for gofmt ci test.

cfe7795

luomiao requested review from msterin, abrarshivani, SandeepPissay and prashima April 11, 2017 01:35

abrarshivani and others added 3 commits April 11, 2017 11:43

Add vm-uuid in cloud-config file required by Kubernetes vSphere Cloud…

206a395

…Provider

Merge pull request #34 from vmware/vm-uuid

500cb69

Add vm-uuid in cloud-config file required by Kubernetes vSphere CloudProvider

Removed hardcode from protokube logic. Fixes #15. (#46)

723bc7f

pdhamdhere reviewed Apr 13, 2017

View reviewed changes

luomiao commented Apr 14, 2017

View reviewed changes

msterin mentioned this pull request Apr 14, 2017

Address review comments and add installing instructions #53

Merged

Miao Luo and others added 13 commits April 19, 2017 00:12

Address review comments and add installing instructions.

0a65088

Merge pull request #53 from luomiao/vsphere-develop

e2e92a0

Address review comments and add installing instructions

Added comments for files and functions. Fixes #52. (#55)

59537b0

Support user-defined s3 endpoint.

0ed5326

When Environment variable S3_ENDPOINT is not empty, kops will use the bucket on this specific s3 endpoint, instead of using AWS S3 by default.

Merge pull request #47 from luomiao/userdefined-s3

f57cc8d

Support user-defined s3 endpoint

Implements delete cluster and toolbox dumper command

81c2a35

Merge pull request #54 from vmware/remote-delete_cluster

4e1b2a5

Implements delete cluster and toolbox dumper command

Change vm_uuid location

f2e03c2

Merge pull request #56 from vmware/vm_uuid_location

424c2e4

Change vm_uuid file location

Change the default image name for vsphere provider and other minor fix.

cc29f6d

Merge pull request #57 from luomiao/vsphere-develop

86c2473

Change the default image name for vsphere provider and other minor fix

Failing unsupported commands with proper message. Fixes #48. (#58)

ed42a93

Removed usage validation from update cmd, unblocking create cmd. (#60)

8051baa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For internal review only #41

For internal review only #41

luomiao commented Apr 10, 2017 •

edited

Loading

pdhamdhere left a comment

pdhamdhere Apr 13, 2017

luomiao Apr 13, 2017 •

edited

Loading

pdhamdhere Apr 13, 2017

luomiao Apr 13, 2017

abrarshivani Apr 17, 2017

pdhamdhere Apr 13, 2017

pdhamdhere Apr 13, 2017

luomiao Apr 13, 2017

pdhamdhere Apr 13, 2017

luomiao Apr 13, 2017

pdhamdhere Apr 13, 2017

pdhamdhere Apr 13, 2017

luomiao Apr 13, 2017

prashima Apr 14, 2017

pdhamdhere Apr 13, 2017

pdhamdhere Apr 13, 2017

pdhamdhere Apr 13, 2017

luomiao Apr 14, 2017

For internal review only #41

Are you sure you want to change the base?

For internal review only #41

Conversation

luomiao commented Apr 10, 2017 • edited Loading

pdhamdhere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luomiao Apr 13, 2017 • 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

Choose a reason for hiding this comment

luomiao commented Apr 10, 2017 •

edited

Loading

luomiao Apr 13, 2017 •

edited

Loading