From 23c0cdab3602eb22f36737cced98a1c120669a27 Mon Sep 17 00:00:00 2001 From: liranp Date: Mon, 15 Jul 2019 11:54:43 +0300 Subject: [PATCH 1/2] feat(spotinst): new hybrid mode --- pkg/featureflag/featureflag.go | 2 ++ pkg/model/awsmodel/BUILD.bazel | 1 + pkg/model/awsmodel/autoscalinggroup.go | 10 +++++++ pkg/model/spotinstmodel/BUILD.bazel | 1 - pkg/model/spotinstmodel/instance_group.go | 27 +++++++++++++++--- pkg/resources/aws/aws.go | 4 +-- upup/pkg/fi/cloudup/apply_cluster.go | 34 ++++++++++++++--------- upup/pkg/fi/cloudup/awsup/aws_cloud.go | 31 +++++++++++++++++++-- 8 files changed, 87 insertions(+), 23 deletions(-) diff --git a/pkg/featureflag/featureflag.go b/pkg/featureflag/featureflag.go index 637f72f61b7fe..577bbb60a77a9 100644 --- a/pkg/featureflag/featureflag.go +++ b/pkg/featureflag/featureflag.go @@ -76,6 +76,8 @@ var ( Spotinst = New("Spotinst", Bool(false)) // SpotinstOcean toggles the use of Spotinst Ocean instance group implementation. SpotinstOcean = New("SpotinstOcean", Bool(false)) + // SpotinstHybrid toggles between hybrid and full instance group implementations. + SpotinstHybrid = New("SpotinstHybrid", Bool(false)) // SpotinstController toggles the installation of the Spotinst controller addon. SpotinstController = New("SpotinstController", Bool(true)) // VPCSkipEnableDNSSupport if set will make that a VPC does not need DNSSupport enabled. diff --git a/pkg/model/awsmodel/BUILD.bazel b/pkg/model/awsmodel/BUILD.bazel index df24fc207282b..a67667c2d65cc 100644 --- a/pkg/model/awsmodel/BUILD.bazel +++ b/pkg/model/awsmodel/BUILD.bazel @@ -15,6 +15,7 @@ go_library( "//pkg/featureflag:go_default_library", "//pkg/model:go_default_library", "//pkg/model/defaults:go_default_library", + "//pkg/model/spotinstmodel:go_default_library", "//upup/pkg/fi:go_default_library", "//upup/pkg/fi/cloudup/awstasks:go_default_library", "//upup/pkg/fi/fitasks:go_default_library", diff --git a/pkg/model/awsmodel/autoscalinggroup.go b/pkg/model/awsmodel/autoscalinggroup.go index ab2438ce8c514..1c9ca4d5306dd 100644 --- a/pkg/model/awsmodel/autoscalinggroup.go +++ b/pkg/model/awsmodel/autoscalinggroup.go @@ -20,9 +20,12 @@ import ( "fmt" "strings" + "k8s.io/klog" "k8s.io/kops/pkg/apis/kops" + "k8s.io/kops/pkg/featureflag" "k8s.io/kops/pkg/model" "k8s.io/kops/pkg/model/defaults" + "k8s.io/kops/pkg/model/spotinstmodel" "k8s.io/kops/upup/pkg/fi" "k8s.io/kops/upup/pkg/fi/cloudup/awstasks" @@ -54,6 +57,13 @@ func (b *AutoscalingGroupModelBuilder) Build(c *fi.ModelBuilderContext) error { for _, ig := range b.InstanceGroups { name := b.AutoscalingGroupName(ig) + if featureflag.SpotinstHybrid.Enabled() { + if spotinstmodel.ManageInstanceGroup(ig) { + klog.V(2).Infof("Skipping instance group: %q", name) + continue + } + } + // @check if his instancegroup is backed by a fleet and override with a launch template task, err := func() (fi.Task, error) { switch UseLaunchTemplate(ig) { diff --git a/pkg/model/spotinstmodel/BUILD.bazel b/pkg/model/spotinstmodel/BUILD.bazel index 20fb46591822a..2a65d6eb582e2 100644 --- a/pkg/model/spotinstmodel/BUILD.bazel +++ b/pkg/model/spotinstmodel/BUILD.bazel @@ -9,7 +9,6 @@ go_library( "//pkg/apis/kops:go_default_library", "//pkg/featureflag:go_default_library", "//pkg/model:go_default_library", - "//pkg/model/awsmodel:go_default_library", "//pkg/model/defaults:go_default_library", "//upup/pkg/fi:go_default_library", "//upup/pkg/fi/cloudup/awstasks:go_default_library", diff --git a/pkg/model/spotinstmodel/instance_group.go b/pkg/model/spotinstmodel/instance_group.go index b1eec12b5466c..356f751e0a1f1 100644 --- a/pkg/model/spotinstmodel/instance_group.go +++ b/pkg/model/spotinstmodel/instance_group.go @@ -26,7 +26,6 @@ import ( "k8s.io/kops/pkg/apis/kops" "k8s.io/kops/pkg/featureflag" "k8s.io/kops/pkg/model" - "k8s.io/kops/pkg/model/awsmodel" "k8s.io/kops/pkg/model/defaults" "k8s.io/kops/upup/pkg/fi" "k8s.io/kops/upup/pkg/fi/cloudup/awstasks" @@ -34,6 +33,10 @@ import ( ) const ( + // InstanceGroupLabelManaged is the metadata label used on the instance + // group to specify that the Spotinst provider should be used to upon creation. + InstanceGroupLabelManaged = "spotinst.io/managed" + // InstanceGroupLabelSpotPercentage is the metadata label used on the // instance group to specify the percentage of Spot instances that // should spin up from the target capacity. @@ -101,7 +104,7 @@ const ( // InstanceGroupModelBuilder configures InstanceGroup objects type InstanceGroupModelBuilder struct { - *awsmodel.AWSModelContext + *model.KopsModelContext BootstrapScript *model.BootstrapScript Lifecycle *fi.Lifecycle @@ -115,8 +118,16 @@ func (b *InstanceGroupModelBuilder) Build(c *fi.ModelBuilderContext) error { var err error for _, ig := range b.InstanceGroups { - klog.V(2).Infof("Building instance group: %q", b.AutoscalingGroupName(ig)) + name := b.AutoscalingGroupName(ig) + + if featureflag.SpotinstHybrid.Enabled() { + if !ManageInstanceGroup(ig) { + klog.V(2).Infof("Skipping instance group: %q", name) + continue + } + } + klog.V(2).Infof("Building instance group: %q", name) switch ig.Spec.Role { // Create both Master and Bastion instance groups as Elastigroups. @@ -642,7 +653,7 @@ func (b *InstanceGroupModelBuilder) buildRootVolumeOpts(ig *kops.InstanceGroup) { typ := fi.StringValue(ig.Spec.RootVolumeType) if typ == "" { - typ = awsmodel.DefaultVolumeType + typ = "gp2" } opts.Type = fi.String(typ) } @@ -913,3 +924,11 @@ func defaultSpotPercentage(ig *kops.InstanceGroup) *float64 { return &percentage } + +// ManageInstanceGroup indicates whether the instance group labeled with +// a metadata label `spotinst.io/managed` which means the Spotinst provider +// should be used to upon creation if the `SpotinstHybrid` feature flag is on. +func ManageInstanceGroup(ig *kops.InstanceGroup) bool { + managed, _ := strconv.ParseBool(ig.ObjectMeta.Labels[InstanceGroupLabelManaged]) + return managed +} diff --git a/pkg/resources/aws/aws.go b/pkg/resources/aws/aws.go index edf1b9b2dfe43..291498cd31e0b 100644 --- a/pkg/resources/aws/aws.go +++ b/pkg/resources/aws/aws.go @@ -61,6 +61,7 @@ func ListResourcesAWS(cloud awsup.AWSCloud, clusterName string) (map[string]*res //ListCloudFormationStacks, // EC2 + ListAutoScalingGroups, ListInstances, ListKeypairs, ListSecurityGroups, @@ -86,9 +87,6 @@ func ListResourcesAWS(cloud awsup.AWSCloud, clusterName string) (map[string]*res if featureflag.Spotinst.Enabled() { // Spotinst resources listFunctions = append(listFunctions, ListSpotinstResources) - } else { - // AutoScaling Groups - listFunctions = append(listFunctions, ListAutoScalingGroups) } for _, fn := range listFunctions { diff --git a/upup/pkg/fi/cloudup/apply_cluster.go b/upup/pkg/fi/cloudup/apply_cluster.go index 1e7e218ff4498..f8c33d3be8e55 100644 --- a/upup/pkg/fi/cloudup/apply_cluster.go +++ b/upup/pkg/fi/cloudup/apply_cluster.go @@ -741,24 +741,32 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error { } switch kops.CloudProviderID(cluster.Spec.CloudProvider) { case kops.CloudProviderAWS: - awsModelContext := &awsmodel.AWSModelContext{ - KopsModelContext: modelContext, - } + { + awsModelContext := &awsmodel.AWSModelContext{ + KopsModelContext: modelContext, + } - if featureflag.Spotinst.Enabled() { - l.Builders = append(l.Builders, &spotinstmodel.InstanceGroupModelBuilder{ + awsModelBuilder := &awsmodel.AutoscalingGroupModelBuilder{ AWSModelContext: awsModelContext, BootstrapScript: bootstrapScriptBuilder, Lifecycle: &clusterLifecycle, SecurityLifecycle: &securityLifecycle, - }) - } else { - l.Builders = append(l.Builders, &awsmodel.AutoscalingGroupModelBuilder{ - AWSModelContext: awsModelContext, - BootstrapScript: bootstrapScriptBuilder, - Lifecycle: &clusterLifecycle, - SecurityLifecycle: &securityLifecycle, - }) + } + + if featureflag.Spotinst.Enabled() { + l.Builders = append(l.Builders, &spotinstmodel.InstanceGroupModelBuilder{ + KopsModelContext: modelContext, + BootstrapScript: bootstrapScriptBuilder, + Lifecycle: &clusterLifecycle, + SecurityLifecycle: &securityLifecycle, + }) + + if featureflag.SpotinstHybrid.Enabled() { + l.Builders = append(l.Builders, awsModelBuilder) + } + } else { + l.Builders = append(l.Builders, awsModelBuilder) + } } case kops.CloudProviderDO: doModelContext := &domodel.DOModelContext{ diff --git a/upup/pkg/fi/cloudup/awsup/aws_cloud.go b/upup/pkg/fi/cloudup/awsup/aws_cloud.go index bd999460aba2a..e47d103dd3d14 100644 --- a/upup/pkg/fi/cloudup/awsup/aws_cloud.go +++ b/upup/pkg/fi/cloudup/awsup/aws_cloud.go @@ -344,6 +344,12 @@ func NewEC2Filter(name string, values ...string) *ec2.Filter { // DeleteGroup deletes an aws autoscaling group func (c *awsCloudImplementation) DeleteGroup(g *cloudinstances.CloudInstanceGroup) error { if c.spotinst != nil { + if featureflag.SpotinstHybrid.Enabled() { + if _, ok := g.Raw.(*autoscaling.Group); ok { + return deleteGroup(c, g) + } + } + return spotinst.DeleteInstanceGroup(c.spotinst, g) } @@ -425,6 +431,12 @@ func deleteGroup(c AWSCloud, g *cloudinstances.CloudInstanceGroup) error { // DeleteInstance deletes an aws instance func (c *awsCloudImplementation) DeleteInstance(i *cloudinstances.CloudInstanceGroupMember) error { if c.spotinst != nil { + if featureflag.SpotinstHybrid.Enabled() { + if _, ok := i.CloudInstanceGroup.Raw.(*autoscaling.Group); ok { + return deleteInstance(c, i) + } + } + return spotinst.DeleteInstance(c.spotinst, i) } @@ -490,8 +502,23 @@ func detachInstance(c AWSCloud, i *cloudinstances.CloudInstanceGroupMember) erro // GetCloudGroups returns a groups of instances that back a kops instance groups func (c *awsCloudImplementation) GetCloudGroups(cluster *kops.Cluster, instancegroups []*kops.InstanceGroup, warnUnmatched bool, nodes []v1.Node) (map[string]*cloudinstances.CloudInstanceGroup, error) { if c.spotinst != nil { - return spotinst.GetCloudGroups(c.spotinst, cluster, - instancegroups, warnUnmatched, nodes) + sgroups, err := spotinst.GetCloudGroups(c.spotinst, cluster, instancegroups, warnUnmatched, nodes) + if err != nil { + return nil, err + } + + if featureflag.SpotinstHybrid.Enabled() { + agroups, err := getCloudGroups(c, cluster, instancegroups, warnUnmatched, nodes) + if err != nil { + return nil, err + } + + for name, group := range agroups { + sgroups[name] = group + } + } + + return sgroups, nil } return getCloudGroups(c, cluster, instancegroups, warnUnmatched, nodes) From 04d83c6c04215763ef079fa862ca8847a166999a Mon Sep 17 00:00:00 2001 From: liranp Date: Sun, 27 Oct 2019 11:22:54 +0200 Subject: [PATCH 2/2] fix(spotinst): rename the label to match the feature flag --- pkg/model/awsmodel/autoscalinggroup.go | 2 +- pkg/model/spotinstmodel/instance_group.go | 24 ++++++++++++++--------- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/pkg/model/awsmodel/autoscalinggroup.go b/pkg/model/awsmodel/autoscalinggroup.go index 1c9ca4d5306dd..6e34eb433f7c4 100644 --- a/pkg/model/awsmodel/autoscalinggroup.go +++ b/pkg/model/awsmodel/autoscalinggroup.go @@ -58,7 +58,7 @@ func (b *AutoscalingGroupModelBuilder) Build(c *fi.ModelBuilderContext) error { name := b.AutoscalingGroupName(ig) if featureflag.SpotinstHybrid.Enabled() { - if spotinstmodel.ManageInstanceGroup(ig) { + if spotinstmodel.HybridInstanceGroup(ig) { klog.V(2).Infof("Skipping instance group: %q", name) continue } diff --git a/pkg/model/spotinstmodel/instance_group.go b/pkg/model/spotinstmodel/instance_group.go index 356f751e0a1f1..801930bf5d488 100644 --- a/pkg/model/spotinstmodel/instance_group.go +++ b/pkg/model/spotinstmodel/instance_group.go @@ -33,9 +33,10 @@ import ( ) const ( - // InstanceGroupLabelManaged is the metadata label used on the instance - // group to specify that the Spotinst provider should be used to upon creation. - InstanceGroupLabelManaged = "spotinst.io/managed" + // InstanceGroupLabelHybrid is the metadata label used on the instance group + // to specify that the Spotinst provider should be used to upon creation. + InstanceGroupLabelHybrid = "spotinst.io/hybrid" + InstanceGroupLabelManaged = "spotinst.io/managed" // for backward compatibility // InstanceGroupLabelSpotPercentage is the metadata label used on the // instance group to specify the percentage of Spot instances that @@ -121,7 +122,7 @@ func (b *InstanceGroupModelBuilder) Build(c *fi.ModelBuilderContext) error { name := b.AutoscalingGroupName(ig) if featureflag.SpotinstHybrid.Enabled() { - if !ManageInstanceGroup(ig) { + if !HybridInstanceGroup(ig) { klog.V(2).Infof("Skipping instance group: %q", name) continue } @@ -925,10 +926,15 @@ func defaultSpotPercentage(ig *kops.InstanceGroup) *float64 { return &percentage } -// ManageInstanceGroup indicates whether the instance group labeled with -// a metadata label `spotinst.io/managed` which means the Spotinst provider +// HybridInstanceGroup indicates whether the instance group labeled with +// a metadata label `spotinst.io/hybrid` which means the Spotinst provider // should be used to upon creation if the `SpotinstHybrid` feature flag is on. -func ManageInstanceGroup(ig *kops.InstanceGroup) bool { - managed, _ := strconv.ParseBool(ig.ObjectMeta.Labels[InstanceGroupLabelManaged]) - return managed +func HybridInstanceGroup(ig *kops.InstanceGroup) bool { + v, ok := ig.ObjectMeta.Labels[InstanceGroupLabelHybrid] + if !ok { + v = ig.ObjectMeta.Labels[InstanceGroupLabelManaged] + } + + hybrid, _ := strconv.ParseBool(v) + return hybrid }