Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spotinst: New hybrid integration mode #7252

Merged
merged 2 commits into from
May 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/featureflag/featureflag.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions pkg/model/awsmodel/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
10 changes: 10 additions & 0 deletions pkg/model/awsmodel/autoscalinggroup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean when I activate this feature flag I just override the use of AutoScaling Groups?
I am not a big fan of changing behaviour with feature flags. As the name suggests, it should add features, not modify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not. It just means that you'd like to use Spotinst for one or more instance groups. It allows users to continue managing their instance groups through AWS Auto Scaling groups, except for specific ones labeled with our metadata label (spotinst.io/hybrid: "true"). So, if you toggle the feature flag on but without setting the metadata label you do not override anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thank you for clarifying!

if spotinstmodel.HybridInstanceGroup(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) {
Expand Down
1 change: 0 additions & 1 deletion pkg/model/spotinstmodel/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
33 changes: 29 additions & 4 deletions pkg/model/spotinstmodel/instance_group.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,18 @@ 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"
"k8s.io/kops/upup/pkg/fi/cloudup/spotinsttasks"
)

const (
// 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
// should spin up from the target capacity.
Expand Down Expand Up @@ -101,7 +105,7 @@ const (

// InstanceGroupModelBuilder configures InstanceGroup objects
type InstanceGroupModelBuilder struct {
*awsmodel.AWSModelContext
*model.KopsModelContext

BootstrapScript *model.BootstrapScript
Lifecycle *fi.Lifecycle
Expand All @@ -115,8 +119,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 !HybridInstanceGroup(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.
Expand Down Expand Up @@ -642,7 +654,7 @@ func (b *InstanceGroupModelBuilder) buildRootVolumeOpts(ig *kops.InstanceGroup)
{
typ := fi.StringValue(ig.Spec.RootVolumeType)
if typ == "" {
typ = awsmodel.DefaultVolumeType
typ = "gp2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no other compatibility within spotinst? Hardcoding defaults looks weird to me - why is this an option to set then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We just follow the AWS implementation, but have to replace the use of the const awsmodel.DefaultVolumeType with its value to avoid cyclic imports (since the AWS model imports the Spotinst one, see: autoscalinggroup.go#L28 and autoscalinggroup.go#L58-L63).

Do you think we should implement it differently? If so, I'd change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh this is just about cyclic imports, I see. Thanks!

}
opts.Type = fi.String(typ)
}
Expand Down Expand Up @@ -913,3 +925,16 @@ func defaultSpotPercentage(ig *kops.InstanceGroup) *float64 {

return &percentage
}

// 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 HybridInstanceGroup(ig *kops.InstanceGroup) bool {
v, ok := ig.ObjectMeta.Labels[InstanceGroupLabelHybrid]
if !ok {
v = ig.ObjectMeta.Labels[InstanceGroupLabelManaged]
}

hybrid, _ := strconv.ParseBool(v)
return hybrid
}
4 changes: 1 addition & 3 deletions pkg/resources/aws/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ func ListResourcesAWS(cloud awsup.AWSCloud, clusterName string) (map[string]*res
//ListCloudFormationStacks,

// EC2
ListAutoScalingGroups,
ListInstances,
ListKeypairs,
ListSecurityGroups,
Expand All @@ -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 {
Expand Down
34 changes: 21 additions & 13 deletions upup/pkg/fi/cloudup/apply_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
31 changes: 29 additions & 2 deletions upup/pkg/fi/cloudup/awsup/aws_cloud.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
rifelpet marked this conversation as resolved.
Show resolved Hide resolved
}
}

return spotinst.DeleteInstanceGroup(c.spotinst, g)
}

Expand Down Expand Up @@ -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)
rifelpet marked this conversation as resolved.
Show resolved Hide resolved
}
}

return spotinst.DeleteInstance(c.spotinst, i)
}

Expand Down Expand Up @@ -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)
Expand Down