Add Zones field to InstanceGroup #3439

justinsb · 2017-09-24T00:57:31Z

The Zones field can specify zones where they are not specified on a
Subnet, for example on GCE where we have regional subnets.

justinsb · 2017-09-24T00:59:29Z

#3438 should have the first 3 commits, so this is only the last commit. Will rebase after #3438.

Split because otherwise the PR is harder to read (though you can always go commit by commit), but I'm not sure whether I'll be able to get #3438 to pass without this PR. Fingers crossed :-)

k8s-github-robot · 2017-09-24T01:40:39Z

@justinsb PR needs rebase

justinsb · 2017-09-24T03:52:40Z

cmd/kops/create_cluster.go

+	allZones.Insert(c.Zones...)
+	allZones.Insert(c.MasterZones...)
+
+	if cluster.Spec.CloudProvider == "" {


This is just moved up from lower...

justinsb · 2017-09-24T03:53:44Z

cmd/kops/create_cluster.go

 		}
 	}

-	if len(cluster.Spec.Subnets) == 0 {


This is covered by the explicit Zones check above

justinsb · 2017-09-24T03:54:16Z

cmd/kops/create_cluster.go

-
-			if c.MasterCount != 0 && c.MasterCount < int32(len(masterSubnets)) {
-				return fmt.Errorf("specified %d master zones, but also requested %d masters.  If specifying both, the count should match.", len(masterSubnets), c.MasterCount)
+		masterZones := c.MasterZones


I think this logic is clearer and fixes the bug when you specify zones and master-count

justinsb · 2017-09-24T03:57:13Z

cmd/kops/create_cluster.go

 		g.ObjectMeta.Name = "nodes"
+
+		subnetNames := sets.NewString()


We explicitly initialize subnets/zones - it is safer, and we don't necessarily have all the information later in the GCE case

justinsb · 2017-09-24T03:58:23Z

pkg/apis/kops/v1alpha1/conversion.go

@@ -235,15 +235,25 @@ func Convert_kops_EtcdMemberSpec_To_v1alpha1_EtcdMemberSpec(in *kops.EtcdMemberS
 }

 func Convert_v1alpha1_InstanceGroupSpec_To_kops_InstanceGroupSpec(in *InstanceGroupSpec, out *kops.InstanceGroupSpec, s conversion.Scope) error {
+	err := autoConvert_v1alpha1_InstanceGroupSpec_To_kops_InstanceGroupSpec(in, out, s)


Changed order here to preserve the logic, now that the autogenerated code has Zones fields...

Will this impact auto-generated code?

Should not.

justinsb · 2017-09-24T03:59:22Z

pkg/apis/kops/validation/legacy.go

 	switch kops.CloudProviderID(c.Spec.CloudProvider) {
 	case kops.CloudProviderBareMetal:
 		requiresSubnets = false
 		requiresNetworkCIDR = false
+		if c.Spec.NetworkCIDR != "" {
+			return field.Invalid(fieldSpec.Child("NetworkCIDR"), c.Spec.NetworkCIDR, "NetworkCIDR should not be set on bare metal")


As the NetworkCIDR is ignored, makes more sense to error if it is set

Can we move this to another PR? I love it, but it kinda does not apply to the work you are doing.

justinsb · 2017-09-24T04:00:33Z

upup/pkg/fi/cloudup/defaults.go

@@ -43,29 +43,31 @@ func PerformAssignments(c *kops.Cluster) error {
 		return err
 	}

-	if c.SharedVPC() && c.Spec.NetworkCIDR == "" {


Same code just refactored, and only done for AWS now

justinsb · 2017-09-24T04:00:56Z

upup/pkg/fi/cloudup/gce/utils.go

@@ -69,3 +70,13 @@ func LastComponent(s string) string {
 	}
 	return s
 }
+
+// ZoneToRegion maps a GCE zone name to a GCE region name, returning an error if it cannot be mapped
+func ZoneToRegion(zone string) (string, error) {


Pulled this into a helper function

justinsb · 2017-09-24T04:01:14Z

upup/pkg/fi/cloudup/utils.go

@@ -40,20 +40,13 @@ func BuildCloud(cluster *kops.Cluster) (fi.Cloud, error) {
 	switch kops.CloudProviderID(cluster.Spec.CloudProvider) {
 	case kops.CloudProviderGCE:
 		{
-			nodeZones := make(map[string]bool)
 			for _, subnet := range cluster.Spec.Subnets {


This got much easier :-)

justinsb · 2017-09-24T04:02:14Z

#3438 can't pass without this (which is why I had to do it :-) ). #3438 is the first commit here; not sure whether it's easier to review the two commits here therefore...

chrislovecnm

Lots of questions... You want this in for 1.8?? Refactoring like this kinda makes me nervous before a release :)

chrislovecnm · 2017-09-24T05:03:19Z

cmd/kops/create_cluster.go

+	if len(c.Zones) == 0 {
+		return fmt.Errorf("must specify at least one zone for the cluster (use --zones)")
+	} else if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderGCE {
+		// On GCE, subnets are regional - we create one per region, not per zone


Should we have this code in the cloud provider rather than create? Move it into an interfaced method as all cloud providers would have to implement it.

We can see how this goes I suspect as we add more clouds

chrislovecnm · 2017-09-24T05:05:41Z

pkg/apis/kops/cluster.go

@@ -324,8 +325,10 @@ type ClusterSubnetSpec struct {
 	Name string `json:"name,omitempty"`
 	// CIDR is the network cidr of the subnet
 	CIDR string `json:"cidr,omitempty"`
-	// Zone is the zone the subnet resides
+	// Zone is the zone the subnet is in, set for subnets that are zonally scoped


Can we just have one value to use across all providers?

Not sure I understand

chrislovecnm · 2017-09-24T05:06:44Z

pkg/apis/kops/v1alpha1/conversion.go

@@ -235,15 +235,25 @@ func Convert_kops_EtcdMemberSpec_To_v1alpha1_EtcdMemberSpec(in *kops.EtcdMemberS
 }

 func Convert_v1alpha1_InstanceGroupSpec_To_kops_InstanceGroupSpec(in *InstanceGroupSpec, out *kops.InstanceGroupSpec, s conversion.Scope) error {
+	err := autoConvert_v1alpha1_InstanceGroupSpec_To_kops_InstanceGroupSpec(in, out, s)


Will this impact auto-generated code?

chrislovecnm · 2017-09-24T05:08:06Z

pkg/apis/kops/validation/legacy.go

 	switch kops.CloudProviderID(c.Spec.CloudProvider) {
 	case kops.CloudProviderBareMetal:
 		requiresSubnets = false
 		requiresNetworkCIDR = false
+		if c.Spec.NetworkCIDR != "" {
+			return field.Invalid(fieldSpec.Child("NetworkCIDR"), c.Spec.NetworkCIDR, "NetworkCIDR should not be set on bare metal")


Can we move this to another PR? I love it, but it kinda does not apply to the work you are doing.

chrislovecnm · 2017-09-24T05:08:40Z

pkg/apis/kops/validation/legacy.go

+			return field.Invalid(fieldSpec.Child("NetworkCIDR"), c.Spec.NetworkCIDR, "NetworkCIDR should not be set on bare metal")
+		}
+
+	case kops.CloudProviderGCE:


Same comment as above. Can we break this into a different PR?

Will move the bare metal one into another PR, but it's not really practical to split out the GCE logic here..

Done in #3495

k8s-github-robot · 2017-09-28T08:02:20Z

@justinsb PR needs rebase

chrislovecnm · 2017-09-30T20:27:50Z

/lgtm
/label queue/blocks-others

k8s-github-robot · 2017-09-30T20:29:57Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @chrislovecnm @justinsb

chrislovecnm · 2017-09-30T21:13:37Z

/lgtm

k8s-github-robot · 2017-09-30T21:40:19Z

@justinsb PR needs rebase

justinsb · 2017-09-30T21:46:12Z

Trivial rebase (imports), will reapply LGTM

k8s-github-robot · 2017-09-30T21:47:57Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @chrislovecnm @justinsb

k8s-github-robot · 2017-09-30T23:01:42Z

/test all [submit-queue is verifying that this PR is safe to merge]

We also make the subnet zone and CIDR optional on GCE

The Zones field can specify zones where they are not specified on a Subnet, for example on GCE where we have regional subnets.

k8s-github-robot · 2017-09-30T23:46:09Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @chrislovecnm @justinsb

chrislovecnm · 2017-09-30T23:49:16Z

/lgtm

k8s-github-robot · 2017-09-30T23:50:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [chrislovecnm]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-10-01T00:28:27Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-10-01T01:17:32Z

Automatic merge from submit-queue.

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 24, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2017

justinsb force-pushed the gce_zones_in_instancegroup branch from 92b3f54 to 8eba0f1 Compare September 24, 2017 03:51

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 24, 2017

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2017

justinsb commented Sep 24, 2017

View reviewed changes

cmd/kops/create_cluster.go

}

}

if len(cluster.Spec.Subnets) == 0 {

Copy link

Member Author

justinsb Sep 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is covered by the explicit Zones check above

justinsb commented Sep 24, 2017

View reviewed changes

justinsb mentioned this pull request Sep 24, 2017

Subnets on GCE are regional, create one per region #3438

Closed

chrislovecnm reviewed Sep 24, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2017

justinsb mentioned this pull request Sep 30, 2017

baremetal: error if NetworkCIDR set #3495

Merged

justinsb force-pushed the gce_zones_in_instancegroup branch from 8eba0f1 to 2658cc7 Compare September 30, 2017 19:59

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2017

k8s-ci-robot assigned chrislovecnm Sep 30, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 30, 2017

justinsb force-pushed the gce_zones_in_instancegroup branch from 2658cc7 to 4ab27fb Compare September 30, 2017 20:27

chrislovecnm added the queue/blocks-others label Sep 30, 2017

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2017

justinsb force-pushed the gce_zones_in_instancegroup branch from 4ab27fb to 67e3dea Compare September 30, 2017 21:45

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 30, 2017

justinsb added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2017

justinsb added 2 commits September 30, 2017 19:22

Subnets on GCE are regional, create one per region

da99a72

We also make the subnet zone and CIDR optional on GCE

Add Zones field to InstanceGroup

7fd1196

The Zones field can specify zones where they are not specified on a Subnet, for example on GCE where we have regional subnets.

justinsb force-pushed the gce_zones_in_instancegroup branch from 67e3dea to 7fd1196 Compare September 30, 2017 23:44

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2017

k8s-github-robot merged commit 852455a into kubernetes:master Oct 1, 2017

Add Zones field to InstanceGroup #3439

Add Zones field to InstanceGroup #3439

Conversation

justinsb commented Sep 24, 2017

justinsb commented Sep 24, 2017

k8s-github-robot commented Sep 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb Sep 24, 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

justinsb commented Sep 24, 2017

chrislovecnm 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

k8s-github-robot commented Sep 28, 2017

chrislovecnm commented Sep 30, 2017

k8s-github-robot commented Sep 30, 2017

chrislovecnm commented Sep 30, 2017

k8s-github-robot commented Sep 30, 2017

justinsb commented Sep 30, 2017

k8s-github-robot commented Sep 30, 2017

k8s-github-robot commented Sep 30, 2017

k8s-github-robot commented Sep 30, 2017

chrislovecnm commented Sep 30, 2017

k8s-github-robot commented Sep 30, 2017

k8s-github-robot commented Oct 1, 2017

k8s-github-robot commented Oct 1, 2017

justinsb Sep 24, 2017 •

edited

Loading