From a33acc0ae461ed15a974f15cbf19f18a6b461899 Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Tue, 30 Jun 2020 20:20:09 -0700 Subject: [PATCH 1/3] Move zone setup to pkg and refactor --- cmd/kops/BUILD.bazel | 1 - cmd/kops/create_cluster.go | 116 +---------------- upup/pkg/fi/cloudup/BUILD.bazel | 1 + upup/pkg/fi/cloudup/new_cluster.go | 197 ++++++++++++++++++++++++++++- 4 files changed, 196 insertions(+), 119 deletions(-) diff --git a/cmd/kops/BUILD.bazel b/cmd/kops/BUILD.bazel index 68605e6871..db31744065 100644 --- a/cmd/kops/BUILD.bazel +++ b/cmd/kops/BUILD.bazel @@ -85,7 +85,6 @@ go_library( "//pkg/validation:go_default_library", "//upup/pkg/fi:go_default_library", "//upup/pkg/fi/cloudup:go_default_library", - "//upup/pkg/fi/cloudup/aliup:go_default_library", "//upup/pkg/fi/cloudup/awsup:go_default_library", "//upup/pkg/fi/cloudup/gce:go_default_library", "//upup/pkg/fi/cloudup/openstack:go_default_library", diff --git a/cmd/kops/create_cluster.go b/cmd/kops/create_cluster.go index 8a2181580e..f4a8202d48 100644 --- a/cmd/kops/create_cluster.go +++ b/cmd/kops/create_cluster.go @@ -48,7 +48,6 @@ import ( "k8s.io/kops/pkg/model/components" "k8s.io/kops/upup/pkg/fi" "k8s.io/kops/upup/pkg/fi/cloudup" - "k8s.io/kops/upup/pkg/fi/cloudup/aliup" "k8s.io/kops/upup/pkg/fi/cloudup/awsup" "k8s.io/kops/upup/pkg/fi/cloudup/gce" "k8s.io/kops/upup/pkg/fi/cloudup/openstack" @@ -105,9 +104,6 @@ type CreateClusterOptions struct { // Specify tags for AWS instance groups CloudLabels string - // Egress configuration - FOR TESTING ONLY - Egress string - // Specify tenancy (default or dedicated) for masters and nodes MasterTenancy string NodeTenancy string @@ -426,115 +422,7 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr cluster = clusterResult.Cluster channel := clusterResult.Channel allZones := clusterResult.AllZones - - zoneToSubnetMap := make(map[string]*api.ClusterSubnetSpec) - 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 - for _, zoneName := range allZones.List() { - region, err := gce.ZoneToRegion(zoneName) - if err != nil { - return err - } - - // We create default subnets named the same as the regions - subnetName := region - - subnet := model.FindSubnet(cluster, subnetName) - if subnet == nil { - subnet = &api.ClusterSubnetSpec{ - Name: subnetName, - Region: region, - } - cluster.Spec.Subnets = append(cluster.Spec.Subnets, *subnet) - } - zoneToSubnetMap[zoneName] = subnet - } - } else if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderDO { - if len(c.Zones) > 1 { - return fmt.Errorf("digitalocean cloud provider currently only supports 1 region, expect multi-region support when digitalocean support is in beta") - } - - // For DO we just pass in the region for --zones - region := c.Zones[0] - subnet := model.FindSubnet(cluster, region) - - // for DO, subnets are just regions - subnetName := region - - if subnet == nil { - subnet = &api.ClusterSubnetSpec{ - Name: subnetName, - // region and zone are the same for DO - Region: region, - Zone: region, - } - cluster.Spec.Subnets = append(cluster.Spec.Subnets, *subnet) - } - zoneToSubnetMap[region] = subnet - } else if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderALI { - var zoneToSubnetSwitchID map[string]string - if len(c.Zones) > 0 && len(c.SubnetIDs) > 0 && api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderALI { - zoneToSubnetSwitchID, err = aliup.ZoneToVSwitchID(cluster.Spec.NetworkID, c.Zones, c.SubnetIDs) - if err != nil { - return err - } - } - for _, zoneName := range allZones.List() { - // We create default subnets named the same as the zones - subnetName := zoneName - - subnet := model.FindSubnet(cluster, subnetName) - if subnet == nil { - subnet = &api.ClusterSubnetSpec{ - Name: subnetName, - Zone: subnetName, - Egress: c.Egress, - } - if vswitchID, ok := zoneToSubnetSwitchID[zoneName]; ok { - subnet.ProviderID = vswitchID - } - cluster.Spec.Subnets = append(cluster.Spec.Subnets, *subnet) - } - zoneToSubnetMap[zoneName] = subnet - } - } else { - var zoneToSubnetProviderID map[string]string - if len(c.Zones) > 0 && len(c.SubnetIDs) > 0 { - if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderAWS { - zoneToSubnetProviderID, err = getZoneToSubnetProviderID(cluster.Spec.NetworkID, c.Zones[0][:len(c.Zones[0])-1], c.SubnetIDs) - if err != nil { - return err - } - } else if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderOpenstack { - tags := make(map[string]string) - tags[openstack.TagClusterName] = c.ClusterName - zoneToSubnetProviderID, err = getSubnetProviderID(&cluster.Spec, allZones.List(), c.SubnetIDs, tags) - if err != nil { - return err - } - } - } - for _, zoneName := range allZones.List() { - // We create default subnets named the same as the zones - subnetName := zoneName - - subnet := model.FindSubnet(cluster, subnetName) - if subnet == nil { - subnet = &api.ClusterSubnetSpec{ - Name: subnetName, - Zone: subnetName, - Egress: c.Egress, - } - if subnetID, ok := zoneToSubnetProviderID[zoneName]; ok { - subnet.ProviderID = subnetID - } - cluster.Spec.Subnets = append(cluster.Spec.Subnets, *subnet) - } - zoneToSubnetMap[zoneName] = subnet - } - } + zoneToSubnetMap := clusterResult.ZoneToSubnetMap var masters []*api.InstanceGroup var nodes []*api.InstanceGroup @@ -1360,6 +1248,7 @@ func initializeOpenstackAPI(c *CreateClusterOptions, cluster *api.Cluster) { } } +// TODO dedup or remove func getZoneToSubnetProviderID(VPCID string, region string, subnetIDs []string) (map[string]string, error) { res := make(map[string]string) cloudTags := map[string]string{} @@ -1391,6 +1280,7 @@ func getZoneToSubnetProviderID(VPCID string, region string, subnetIDs []string) return res, nil } +// TODO dedup or remove func getSubnetProviderID(spec *api.ClusterSpec, zones []string, subnetIDs []string, tags map[string]string) (map[string]string, error) { res := make(map[string]string) osCloud, err := openstack.NewOpenstackCloud(tags, spec) diff --git a/upup/pkg/fi/cloudup/BUILD.bazel b/upup/pkg/fi/cloudup/BUILD.bazel index 34408a218a..3cbe0002e5 100644 --- a/upup/pkg/fi/cloudup/BUILD.bazel +++ b/upup/pkg/fi/cloudup/BUILD.bazel @@ -32,6 +32,7 @@ go_library( "//dnsprovider/pkg/dnsprovider/providers/aws/route53:go_default_library", "//dnsprovider/pkg/dnsprovider/rrstype:go_default_library", "//pkg/apis/kops:go_default_library", + "//pkg/apis/kops/model:go_default_library", "//pkg/apis/kops/registry:go_default_library", "//pkg/apis/kops/util:go_default_library", "//pkg/apis/kops/validation:go_default_library", diff --git a/upup/pkg/fi/cloudup/new_cluster.go b/upup/pkg/fi/cloudup/new_cluster.go index 8c5ecb3a5a..d177600ab2 100644 --- a/upup/pkg/fi/cloudup/new_cluster.go +++ b/upup/pkg/fi/cloudup/new_cluster.go @@ -27,9 +27,12 @@ import ( "k8s.io/klog" "k8s.io/kops" api "k8s.io/kops/pkg/apis/kops" + "k8s.io/kops/pkg/apis/kops/model" "k8s.io/kops/pkg/client/simple" "k8s.io/kops/upup/pkg/fi" + "k8s.io/kops/upup/pkg/fi/cloudup/aliup" "k8s.io/kops/upup/pkg/fi/cloudup/awsup" + "k8s.io/kops/upup/pkg/fi/cloudup/gce" "k8s.io/kops/upup/pkg/fi/cloudup/openstack" ) @@ -62,6 +65,8 @@ type NewClusterOptions struct { NetworkID string // SubnetIDs are the IDs of the shared subnets. If empty, creates new subnets to be owned by the cluster. SubnetIDs []string + // Egress defines the method of traffic egress for subnets. + Egress string // OpenstackExternalNet is the name of the external network for the openstack router OpenstackExternalNet string @@ -83,8 +88,9 @@ type NewClusterResult struct { Cluster *api.Cluster // TODO remove after more create_cluster logic refactored in - Channel *api.Channel - AllZones sets.String + Channel *api.Channel + AllZones sets.String + ZoneToSubnetMap map[string]*api.ClusterSubnetSpec } // NewCluster initializes cluster and instance groups specifications as @@ -161,10 +167,16 @@ func NewCluster(opt *NewClusterOptions, clientset simple.Clientset) (*NewCluster return nil, err } + zoneToSubnetMap, err := setupZones(opt, &cluster, allZones) + if err != nil { + return nil, err + } + result := NewClusterResult{ - Cluster: &cluster, - Channel: channel, - AllZones: allZones, + Cluster: &cluster, + Channel: channel, + AllZones: allZones, + ZoneToSubnetMap: zoneToSubnetMap, } return &result, nil } @@ -236,3 +248,178 @@ func setupVPC(opt *NewClusterOptions, cluster *api.Cluster) error { return nil } + +func setupZones(opt *NewClusterOptions, cluster *api.Cluster, allZones sets.String) (map[string]*api.ClusterSubnetSpec, error) { + var err error + zoneToSubnetMap := make(map[string]*api.ClusterSubnetSpec) + + if len(opt.Zones) == 0 { + return nil, fmt.Errorf("must specify at least one zone for the cluster (use --zones)") + } + + var zoneToSubnetProviderID map[string]string + + switch api.CloudProviderID(cluster.Spec.CloudProvider) { + case api.CloudProviderGCE: + // On GCE, subnets are regional - we create one per region, not per zone + for _, zoneName := range allZones.List() { + region, err := gce.ZoneToRegion(zoneName) + if err != nil { + return nil, err + } + + // We create default subnets named the same as the regions + subnetName := region + + subnet := model.FindSubnet(cluster, subnetName) + if subnet == nil { + subnet = &api.ClusterSubnetSpec{ + Name: subnetName, + Region: region, + } + cluster.Spec.Subnets = append(cluster.Spec.Subnets, *subnet) + } + zoneToSubnetMap[zoneName] = subnet + } + return zoneToSubnetMap, nil + + case api.CloudProviderDO: + if len(opt.Zones) > 1 { + return nil, fmt.Errorf("digitalocean cloud provider currently only supports 1 region, expect multi-region support when digitalocean support is in beta") + } + + // For DO we just pass in the region for --zones + region := opt.Zones[0] + subnet := model.FindSubnet(cluster, region) + + // for DO, subnets are just regions + subnetName := region + + if subnet == nil { + subnet = &api.ClusterSubnetSpec{ + Name: subnetName, + // region and zone are the same for DO + Region: region, + Zone: region, + } + cluster.Spec.Subnets = append(cluster.Spec.Subnets, *subnet) + } + zoneToSubnetMap[region] = subnet + return zoneToSubnetMap, nil + + case api.CloudProviderALI: + if len(opt.Zones) > 0 && len(opt.SubnetIDs) > 0 { + zoneToSubnetProviderID, err = aliup.ZoneToVSwitchID(cluster.Spec.NetworkID, opt.Zones, opt.SubnetIDs) + if err != nil { + return nil, err + } + } + + case api.CloudProviderAWS: + if len(opt.Zones) > 0 && len(opt.SubnetIDs) > 0 { + zoneToSubnetProviderID, err = getZoneToSubnetProviderID(cluster.Spec.NetworkID, opt.Zones[0][:len(opt.Zones[0])-1], opt.SubnetIDs) + if err != nil { + return nil, err + } + } + + case api.CloudProviderOpenstack: + if len(opt.Zones) > 0 && len(opt.SubnetIDs) > 0 { + tags := make(map[string]string) + tags[openstack.TagClusterName] = opt.ClusterName + zoneToSubnetProviderID, err = getSubnetProviderID(&cluster.Spec, allZones.List(), opt.SubnetIDs, tags) + if err != nil { + return nil, err + } + } + } + + for _, zoneName := range allZones.List() { + // We create default subnets named the same as the zones + subnetName := zoneName + + subnet := model.FindSubnet(cluster, subnetName) + if subnet == nil { + subnet = &api.ClusterSubnetSpec{ + Name: subnetName, + Zone: subnetName, + Egress: opt.Egress, + } + if subnetID, ok := zoneToSubnetProviderID[zoneName]; ok { + subnet.ProviderID = subnetID + } + cluster.Spec.Subnets = append(cluster.Spec.Subnets, *subnet) + } + zoneToSubnetMap[zoneName] = subnet + } + + return zoneToSubnetMap, nil +} + +// TODO rename to getAWSZoneToSubnetProviderID +func getZoneToSubnetProviderID(VPCID string, region string, subnetIDs []string) (map[string]string, error) { + res := make(map[string]string) + cloudTags := map[string]string{} + awsCloud, err := awsup.NewAWSCloud(region, cloudTags) + if err != nil { + return res, fmt.Errorf("error loading cloud: %v", err) + } + vpcInfo, err := awsCloud.FindVPCInfo(VPCID) + if err != nil { + return res, fmt.Errorf("error describing VPC: %v", err) + } + if vpcInfo == nil { + return res, fmt.Errorf("VPC %q not found", VPCID) + } + subnetByID := make(map[string]*fi.SubnetInfo) + for _, subnetInfo := range vpcInfo.Subnets { + subnetByID[subnetInfo.ID] = subnetInfo + } + for _, subnetID := range subnetIDs { + subnet, ok := subnetByID[subnetID] + if !ok { + return res, fmt.Errorf("subnet %s not found in VPC %s", subnetID, VPCID) + } + if res[subnet.Zone] != "" { + return res, fmt.Errorf("subnet %s and %s have the same zone", subnetID, res[subnet.Zone]) + } + res[subnet.Zone] = subnetID + } + return res, nil +} + +// TODO rename to getOpenstackZoneToSubnetProviderID +func getSubnetProviderID(spec *api.ClusterSpec, zones []string, subnetIDs []string, tags map[string]string) (map[string]string, error) { + res := make(map[string]string) + osCloud, err := openstack.NewOpenstackCloud(tags, spec) + if err != nil { + return res, fmt.Errorf("error loading cloud: %v", err) + } + osCloud.UseZones(zones) + + networkInfo, err := osCloud.FindVPCInfo(spec.NetworkID) + if err != nil { + return res, fmt.Errorf("error describing Network: %v", err) + } + if networkInfo == nil { + return res, fmt.Errorf("network %q not found", spec.NetworkID) + } + + subnetByID := make(map[string]*fi.SubnetInfo) + for _, subnetInfo := range networkInfo.Subnets { + subnetByID[subnetInfo.ID] = subnetInfo + } + + for _, subnetID := range subnetIDs { + subnet, ok := subnetByID[subnetID] + if !ok { + return res, fmt.Errorf("subnet %s not found in network %s", subnetID, spec.NetworkID) + } + + if res[subnet.Zone] != "" { + return res, fmt.Errorf("subnet %s and %s have the same zone", subnetID, res[subnet.Zone]) + } + res[subnet.Zone] = subnetID + } + return res, nil +} From a5b60ccac3857d53c152d32efd97339ad73977e7 Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Tue, 30 Jun 2020 21:52:06 -0700 Subject: [PATCH 2/3] Move master setup to pkg and refactor --- cmd/kops/BUILD.bazel | 3 - cmd/kops/create_cluster.go | 168 +--------------- upup/pkg/fi/cloudup/BUILD.bazel | 2 + upup/pkg/fi/cloudup/new_cluster.go | 182 ++++++++++++++++++ .../pkg/fi/cloudup/new_cluster_test.go | 2 +- 5 files changed, 188 insertions(+), 169 deletions(-) rename cmd/kops/createcluster_test.go => upup/pkg/fi/cloudup/new_cluster_test.go (98%) diff --git a/cmd/kops/BUILD.bazel b/cmd/kops/BUILD.bazel index db31744065..7ffc185b3b 100644 --- a/cmd/kops/BUILD.bazel +++ b/cmd/kops/BUILD.bazel @@ -59,7 +59,6 @@ go_library( "//:go_default_library", "//cmd/kops/util:go_default_library", "//pkg/apis/kops:go_default_library", - "//pkg/apis/kops/model:go_default_library", "//pkg/apis/kops/registry:go_default_library", "//pkg/apis/kops/util:go_default_library", "//pkg/apis/kops/validation:go_default_library", @@ -101,7 +100,6 @@ go_library( "//vendor/github.com/spf13/viper:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", - "//vendor/k8s.io/apimachinery/pkg/api/resource:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library", @@ -136,7 +134,6 @@ go_test( srcs = [ "create_cluster_integration_test.go", "create_cluster_test.go", - "createcluster_test.go", "delete_confirm_test.go", "integration_test.go", "lifecycle_integration_test.go", diff --git a/cmd/kops/create_cluster.go b/cmd/kops/create_cluster.go index f4a8202d48..e0cd7c2843 100644 --- a/cmd/kops/create_cluster.go +++ b/cmd/kops/create_cluster.go @@ -24,20 +24,17 @@ import ( "io" "io/ioutil" "os" - "strconv" "strings" "github.com/blang/semver" "github.com/spf13/cobra" apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/klog" "k8s.io/kops/cmd/kops/util" api "k8s.io/kops/pkg/apis/kops" - "k8s.io/kops/pkg/apis/kops/model" "k8s.io/kops/pkg/apis/kops/registry" version "k8s.io/kops/pkg/apis/kops/util" "k8s.io/kops/pkg/apis/kops/validation" @@ -62,12 +59,9 @@ type CreateClusterOptions struct { Target string NodeSize string MasterSize string - MasterCount int32 NodeCount int32 MasterVolumeSize int32 NodeVolumeSize int32 - EncryptEtcdStorage bool - EtcdStorageType string Project string KubernetesVersion string ContainerRuntime string @@ -420,13 +414,12 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr // TODO: push more of the following logic into cloudup.NewCluster() cluster = clusterResult.Cluster + instanceGroups := clusterResult.InstanceGroups channel := clusterResult.Channel allZones := clusterResult.AllZones zoneToSubnetMap := clusterResult.ZoneToSubnetMap + masters := clusterResult.Masters - var masters []*api.InstanceGroup - var nodes []*api.InstanceGroup - var instanceGroups []*api.InstanceGroup cloudLabels, err := parseCloudLabels(c.CloudLabels) if err != nil { return fmt.Errorf("error parsing global cloud labels: %v", err) @@ -435,138 +428,7 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr cluster.Spec.CloudLabels = cloudLabels } - // Build the master subnets - // The master zones is the default set of zones unless explicitly set - // The master count is the number of master zones unless explicitly set - // We then round-robin around the zones - if len(masters) == 0 { - masterCount := c.MasterCount - masterZones := c.MasterZones - if len(masterZones) != 0 { - if c.MasterCount != 0 && c.MasterCount < int32(len(c.MasterZones)) { - return fmt.Errorf("specified %d master zones, but also requested %d masters. If specifying both, the count should match.", len(masterZones), c.MasterCount) - } - - if masterCount == 0 { - // If master count is not specified, default to the number of master zones - masterCount = int32(len(c.MasterZones)) - } - } else { - // masterZones not set; default to same as node Zones - masterZones = c.Zones - - if masterCount == 0 { - // If master count is not specified, default to 1 - masterCount = 1 - } - } - - if len(masterZones) == 0 { - // Should be unreachable - return fmt.Errorf("cannot determine master zones") - } - - for i := 0; i < int(masterCount); i++ { - zone := masterZones[i%len(masterZones)] - name := zone - if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderDO { - if int(masterCount) >= len(masterZones) { - name += "-" + strconv.Itoa(1+(i/len(masterZones))) - } - } else { - if int(masterCount) > len(masterZones) { - name += "-" + strconv.Itoa(1+(i/len(masterZones))) - } - } - - g := &api.InstanceGroup{} - g.Spec.Role = api.InstanceGroupRoleMaster - g.Spec.MinSize = fi.Int32(1) - g.Spec.MaxSize = fi.Int32(1) - g.ObjectMeta.Name = "master-" + name - - subnet := zoneToSubnetMap[zone] - if subnet == nil { - klog.Fatalf("subnet not found in zoneToSubnetMap") - } - - g.Spec.Subnets = []string{subnet.Name} - if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderGCE { - g.Spec.Zones = []string{zone} - } - - instanceGroups = append(instanceGroups, g) - masters = append(masters, g) - } - } - - if len(cluster.Spec.EtcdClusters) == 0 { - masterAZs := sets.NewString() - duplicateAZs := false - for _, ig := range masters { - zones, err := model.FindZonesForInstanceGroup(cluster, ig) - if err != nil { - return err - } - for _, zone := range zones { - if masterAZs.Has(zone) { - duplicateAZs = true - } - - masterAZs.Insert(zone) - } - } - - if duplicateAZs { - klog.Warningf("Running with masters in the same AZs; redundancy will be reduced") - } - - for _, etcdCluster := range cloudup.EtcdClusters { - etcd := &api.EtcdClusterSpec{} - etcd.Name = etcdCluster - - // if this is the main cluster, we use 200 millicores by default. - // otherwise we use 100 millicores by default. 100Mi is always default - // for event and main clusters. This is changeable in the kops cluster - // configuration. - if etcd.Name == "main" { - cpuRequest := resource.MustParse("200m") - etcd.CPURequest = &cpuRequest - } else { - cpuRequest := resource.MustParse("100m") - etcd.CPURequest = &cpuRequest - } - memoryRequest := resource.MustParse("100Mi") - etcd.MemoryRequest = &memoryRequest - - var names []string - for _, ig := range masters { - name := ig.ObjectMeta.Name - // We expect the IG to have a `master-` prefix, but this is both superfluous - // and not how we named things previously - name = strings.TrimPrefix(name, "master-") - names = append(names, name) - } - - names = trimCommonPrefix(names) - - for i, ig := range masters { - m := &api.EtcdMemberSpec{} - if c.EncryptEtcdStorage { - m.EncryptedVolume = &c.EncryptEtcdStorage - } - if len(c.EtcdStorageType) > 0 { - m.VolumeType = fi.String(c.EtcdStorageType) - } - m.Name = names[i] - - m.InstanceGroup = fi.String(ig.ObjectMeta.Name) - etcd.Members = append(etcd.Members, m) - } - cluster.Spec.EtcdClusters = append(cluster.Spec.EtcdClusters, etcd) - } - } - + var nodes []*api.InstanceGroup if len(nodes) == 0 { g := &api.InstanceGroup{} g.Spec.Role = api.InstanceGroupRoleNode @@ -1176,30 +1038,6 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr return nil } -func trimCommonPrefix(names []string) []string { - // Trim shared prefix to keep the lengths sane - // (this only applies to new clusters...) - for len(names) != 0 && len(names[0]) > 1 { - prefix := names[0][:1] - allMatch := true - for _, name := range names { - if !strings.HasPrefix(name, prefix) { - allMatch = false - } - } - - if !allMatch { - break - } - - for i := range names { - names[i] = strings.TrimPrefix(names[i], prefix) - } - } - - return names -} - // parseCloudLabels takes a CSV list of key=value records and parses them into a map. Nested '='s are supported via // quoted strings (eg `foo="bar=baz"` parses to map[string]string{"foo":"bar=baz"}. Nested commas are not supported. func parseCloudLabels(s string) (map[string]string, error) { diff --git a/upup/pkg/fi/cloudup/BUILD.bazel b/upup/pkg/fi/cloudup/BUILD.bazel index 3cbe0002e5..13e651ef15 100644 --- a/upup/pkg/fi/cloudup/BUILD.bazel +++ b/upup/pkg/fi/cloudup/BUILD.bazel @@ -81,6 +81,7 @@ go_library( "//vendor/github.com/aws/aws-sdk-go/service/ec2:go_default_library", "//vendor/github.com/blang/semver:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/api/resource:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//vendor/k8s.io/klog:go_default_library", @@ -96,6 +97,7 @@ go_test( "defaults_test.go", "dns_test.go", "networking_test.go", + "new_cluster_test.go", "populatecluster_test.go", "populateinstancegroup_test.go", "subnets_test.go", diff --git a/upup/pkg/fi/cloudup/new_cluster.go b/upup/pkg/fi/cloudup/new_cluster.go index d177600ab2..a2ade8d3d1 100644 --- a/upup/pkg/fi/cloudup/new_cluster.go +++ b/upup/pkg/fi/cloudup/new_cluster.go @@ -18,10 +18,12 @@ package cloudup import ( "fmt" + "strconv" "strings" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog" @@ -76,6 +78,14 @@ type NewClusterOptions struct { OpenstackLbSubnet string // OpenstackLBOctavia is boolean value should we use octavia or old loadbalancer api OpenstackLBOctavia bool + + // MasterCount is the number of masters to create. Defaults to the length of MasterZones + // if MasterZones is explicitly nonempty, otherwise defaults to 1. + MasterCount int32 + // EncryptEtcdStorage is whether to encrypt the etcd volumes. + EncryptEtcdStorage bool + // EtcdStorageType is the underlying cloud storage class of the etcd volumes. + EtcdStorageType string } func (o *NewClusterOptions) InitDefaults() { @@ -86,11 +96,14 @@ func (o *NewClusterOptions) InitDefaults() { type NewClusterResult struct { // Cluster is the initialized Cluster resource. Cluster *api.Cluster + // InstanceGroups are the initialized InstanceGroup resources. + InstanceGroups []*api.InstanceGroup // TODO remove after more create_cluster logic refactored in Channel *api.Channel AllZones sets.String ZoneToSubnetMap map[string]*api.ClusterSubnetSpec + Masters []*api.InstanceGroup } // NewCluster initializes cluster and instance groups specifications as @@ -172,11 +185,18 @@ func NewCluster(opt *NewClusterOptions, clientset simple.Clientset) (*NewCluster return nil, err } + masters, err := setupMasters(opt, &cluster, zoneToSubnetMap) + if err != nil { + return nil, err + } + result := NewClusterResult{ Cluster: &cluster, + InstanceGroups: masters, Channel: channel, AllZones: allZones, ZoneToSubnetMap: zoneToSubnetMap, + Masters: masters, } return &result, nil } @@ -423,3 +443,165 @@ func getSubnetProviderID(spec *api.ClusterSpec, zones []string, subnetIDs []stri } return res, nil } + +func setupMasters(opt *NewClusterOptions, cluster *api.Cluster, zoneToSubnetMap map[string]*api.ClusterSubnetSpec) ([]*api.InstanceGroup, error) { + var masters []*api.InstanceGroup + + // Build the master subnets + // The master zones is the default set of zones unless explicitly set + // The master count is the number of master zones unless explicitly set + // We then round-robin around the zones + { + masterCount := opt.MasterCount + masterZones := opt.MasterZones + if len(masterZones) != 0 { + if masterCount != 0 && masterCount < int32(len(masterZones)) { + return nil, fmt.Errorf("specified %d master zones, but also requested %d masters. If specifying both, the count should match.", len(masterZones), masterCount) + } + + if masterCount == 0 { + // If master count is not specified, default to the number of master zones + masterCount = int32(len(masterZones)) + } + } else { + // masterZones not set; default to same as node Zones + masterZones = opt.Zones + + if masterCount == 0 { + // If master count is not specified, default to 1 + masterCount = 1 + } + } + + if len(masterZones) == 0 { + // Should be unreachable + return nil, fmt.Errorf("cannot determine master zones") + } + + for i := 0; i < int(masterCount); i++ { + zone := masterZones[i%len(masterZones)] + name := zone + if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderDO { + if int(masterCount) >= len(masterZones) { + name += "-" + strconv.Itoa(1+(i/len(masterZones))) + } + } else { + if int(masterCount) > len(masterZones) { + name += "-" + strconv.Itoa(1+(i/len(masterZones))) + } + } + + g := &api.InstanceGroup{} + g.Spec.Role = api.InstanceGroupRoleMaster + g.Spec.MinSize = fi.Int32(1) + g.Spec.MaxSize = fi.Int32(1) + g.ObjectMeta.Name = "master-" + name + + subnet := zoneToSubnetMap[zone] + if subnet == nil { + klog.Fatalf("subnet not found in zoneToSubnetMap") + } + + g.Spec.Subnets = []string{subnet.Name} + if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderGCE { + g.Spec.Zones = []string{zone} + } + + masters = append(masters, g) + } + } + + // Build the Etcd clusters + { + masterAZs := sets.NewString() + duplicateAZs := false + for _, ig := range masters { + zones, err := model.FindZonesForInstanceGroup(cluster, ig) + if err != nil { + return nil, err + } + for _, zone := range zones { + if masterAZs.Has(zone) { + duplicateAZs = true + } + + masterAZs.Insert(zone) + } + } + + if duplicateAZs { + klog.Warningf("Running with masters in the same AZs; redundancy will be reduced") + } + + for _, etcdCluster := range EtcdClusters { + etcd := &api.EtcdClusterSpec{} + etcd.Name = etcdCluster + + // if this is the main cluster, we use 200 millicores by default. + // otherwise we use 100 millicores by default. 100Mi is always default + // for event and main clusters. This is changeable in the kops cluster + // configuration. + if etcd.Name == "main" { + cpuRequest := resource.MustParse("200m") + etcd.CPURequest = &cpuRequest + } else { + cpuRequest := resource.MustParse("100m") + etcd.CPURequest = &cpuRequest + } + memoryRequest := resource.MustParse("100Mi") + etcd.MemoryRequest = &memoryRequest + + var names []string + for _, ig := range masters { + name := ig.ObjectMeta.Name + // We expect the IG to have a `master-` prefix, but this is both superfluous + // and not how we named things previously + name = strings.TrimPrefix(name, "master-") + names = append(names, name) + } + + names = trimCommonPrefix(names) + + for i, ig := range masters { + m := &api.EtcdMemberSpec{} + if opt.EncryptEtcdStorage { + m.EncryptedVolume = fi.Bool(opt.EncryptEtcdStorage) + } + if opt.EtcdStorageType != "" { + m.VolumeType = fi.String(opt.EtcdStorageType) + } + m.Name = names[i] + + m.InstanceGroup = fi.String(ig.ObjectMeta.Name) + etcd.Members = append(etcd.Members, m) + } + cluster.Spec.EtcdClusters = append(cluster.Spec.EtcdClusters, etcd) + } + } + + return masters, nil +} + +func trimCommonPrefix(names []string) []string { + // Trim shared prefix to keep the lengths sane + // (this only applies to new clusters...) + for len(names) != 0 && len(names[0]) > 1 { + prefix := names[0][:1] + allMatch := true + for _, name := range names { + if !strings.HasPrefix(name, prefix) { + allMatch = false + } + } + + if !allMatch { + break + } + + for i := range names { + names[i] = strings.TrimPrefix(names[i], prefix) + } + } + + return names +} diff --git a/cmd/kops/createcluster_test.go b/upup/pkg/fi/cloudup/new_cluster_test.go similarity index 98% rename from cmd/kops/createcluster_test.go rename to upup/pkg/fi/cloudup/new_cluster_test.go index 0917537092..8ee9d2bd54 100644 --- a/cmd/kops/createcluster_test.go +++ b/upup/pkg/fi/cloudup/new_cluster_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package main +package cloudup import ( "reflect" From f1a9297cb55e3181a1cb80bcb80f890ce38673d0 Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Tue, 30 Jun 2020 22:24:08 -0700 Subject: [PATCH 3/3] Move node setup to pkg and refactor --- cmd/kops/create_cluster.go | 75 +++++++++++------------------- upup/pkg/fi/cloudup/new_cluster.go | 59 +++++++++++++++++++---- 2 files changed, 76 insertions(+), 58 deletions(-) diff --git a/cmd/kops/create_cluster.go b/cmd/kops/create_cluster.go index e0cd7c2843..2758ba9ce8 100644 --- a/cmd/kops/create_cluster.go +++ b/cmd/kops/create_cluster.go @@ -30,7 +30,6 @@ import ( "github.com/spf13/cobra" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/klog" "k8s.io/kops/cmd/kops/util" @@ -59,7 +58,6 @@ type CreateClusterOptions struct { Target string NodeSize string MasterSize string - NodeCount int32 MasterVolumeSize int32 NodeVolumeSize int32 Project string @@ -390,17 +388,19 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr return fmt.Errorf("--name is required") } - cluster, err := clientset.GetCluster(ctx, c.ClusterName) - if err != nil { - if apierrors.IsNotFound(err) { - cluster = nil - } else { - return err + { + cluster, err := clientset.GetCluster(ctx, c.ClusterName) + if err != nil { + if apierrors.IsNotFound(err) { + cluster = nil + } else { + return err + } } - } - if cluster != nil { - return fmt.Errorf("cluster %q already exists; use 'kops update cluster' to apply changes", c.ClusterName) + if cluster != nil { + return fmt.Errorf("cluster %q already exists; use 'kops update cluster' to apply changes", c.ClusterName) + } } if c.OpenstackNetworkID != "" { @@ -412,13 +412,19 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr return err } - // TODO: push more of the following logic into cloudup.NewCluster() - cluster = clusterResult.Cluster + cluster := clusterResult.Cluster instanceGroups := clusterResult.InstanceGroups - channel := clusterResult.Channel - allZones := clusterResult.AllZones - zoneToSubnetMap := clusterResult.ZoneToSubnetMap - masters := clusterResult.Masters + + var masters []*api.InstanceGroup + var nodes []*api.InstanceGroup + for _, ig := range instanceGroups { + switch ig.Spec.Role { + case api.InstanceGroupRoleMaster: + masters = append(masters, ig) + case api.InstanceGroupRoleNode: + nodes = append(nodes, ig) + } + } cloudLabels, err := parseCloudLabels(c.CloudLabels) if err != nil { @@ -428,30 +434,6 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr cluster.Spec.CloudLabels = cloudLabels } - var nodes []*api.InstanceGroup - if len(nodes) == 0 { - g := &api.InstanceGroup{} - g.Spec.Role = api.InstanceGroupRoleNode - g.ObjectMeta.Name = "nodes" - - subnetNames := sets.NewString() - for _, zone := range c.Zones { - subnet := zoneToSubnetMap[zone] - if subnet == nil { - klog.Fatalf("subnet not found in zoneToSubnetMap") - } - subnetNames.Insert(subnet.Name) - } - g.Spec.Subnets = subnetNames.List() - - if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderGCE { - g.Spec.Zones = c.Zones - } - - instanceGroups = append(instanceGroups, g) - nodes = append(nodes, g) - } - if c.NodeSize != "" { for _, group := range nodes { group.Spec.MachineType = c.NodeSize @@ -480,13 +462,6 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr } } - if c.NodeCount != 0 { - for _, group := range nodes { - group.Spec.MinSize = fi.Int32(c.NodeCount) - group.Spec.MaxSize = fi.Int32(c.NodeCount) - } - } - if c.MasterTenancy != "" { for _, group := range masters { group.Spec.Tenancy = c.MasterTenancy @@ -533,6 +508,10 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr cluster.Spec.DNSZone = c.DNSZone } + // TODO: push more of the following logic into cloudup.NewCluster() + channel := clusterResult.Channel + allZones := clusterResult.AllZones + if c.CloudProvider != "" { if featureflag.Spotinst.Enabled() { if cluster.Spec.CloudConfig == nil { diff --git a/upup/pkg/fi/cloudup/new_cluster.go b/upup/pkg/fi/cloudup/new_cluster.go index a2ade8d3d1..791ab917d2 100644 --- a/upup/pkg/fi/cloudup/new_cluster.go +++ b/upup/pkg/fi/cloudup/new_cluster.go @@ -86,6 +86,10 @@ type NewClusterOptions struct { EncryptEtcdStorage bool // EtcdStorageType is the underlying cloud storage class of the etcd volumes. EtcdStorageType string + + // NodeCount is the number of nodes to create. Defaults to leaving the count unspecified + // on the InstanceGroup, which results in a count of 2. + NodeCount int32 } func (o *NewClusterOptions) InitDefaults() { @@ -100,10 +104,8 @@ type NewClusterResult struct { InstanceGroups []*api.InstanceGroup // TODO remove after more create_cluster logic refactored in - Channel *api.Channel - AllZones sets.String - ZoneToSubnetMap map[string]*api.ClusterSubnetSpec - Masters []*api.InstanceGroup + Channel *api.Channel + AllZones sets.String } // NewCluster initializes cluster and instance groups specifications as @@ -190,13 +192,19 @@ func NewCluster(opt *NewClusterOptions, clientset simple.Clientset) (*NewCluster return nil, err } + nodes, err := setupNodes(opt, &cluster, zoneToSubnetMap) + if err != nil { + return nil, err + } + + instanceGroups := append([]*api.InstanceGroup(nil), masters...) + instanceGroups = append(instanceGroups, nodes...) + result := NewClusterResult{ - Cluster: &cluster, - InstanceGroups: masters, - Channel: channel, - AllZones: allZones, - ZoneToSubnetMap: zoneToSubnetMap, - Masters: masters, + Cluster: &cluster, + InstanceGroups: instanceGroups, + Channel: channel, + AllZones: allZones, } return &result, nil } @@ -605,3 +613,34 @@ func trimCommonPrefix(names []string) []string { return names } + +func setupNodes(opt *NewClusterOptions, cluster *api.Cluster, zoneToSubnetMap map[string]*api.ClusterSubnetSpec) ([]*api.InstanceGroup, error) { + var nodes []*api.InstanceGroup + + g := &api.InstanceGroup{} + g.Spec.Role = api.InstanceGroupRoleNode + g.ObjectMeta.Name = "nodes" + + subnetNames := sets.NewString() + for _, zone := range opt.Zones { + subnet := zoneToSubnetMap[zone] + if subnet == nil { + klog.Fatalf("subnet not found in zoneToSubnetMap") + } + subnetNames.Insert(subnet.Name) + } + g.Spec.Subnets = subnetNames.List() + + if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderGCE { + g.Spec.Zones = opt.Zones + } + + if opt.NodeCount != 0 { + g.Spec.MinSize = fi.Int32(opt.NodeCount) + g.Spec.MaxSize = fi.Int32(opt.NodeCount) + } + + nodes = append(nodes, g) + + return nodes, nil +}