New integration: Spotinst #5922

liranp · 2018-10-11T14:44:34Z

This PR adds Spotinst as an extension of the existing cloud providers. As part of this implementation, AWS Auto Scaling Groups have been replaced with Spotinst Elastigroups.

chrisz100

Thanks @liranp - much better!
Some remarks below.

chrisz100 · 2018-10-11T15:12:14Z

cmd/kops/create_cluster.go

@@ -139,6 +139,10 @@ type CreateClusterOptions struct {
 	// We can remove this once we support higher versions.
 	VSphereDatastore string

+	// Spotinst options
+	SpotinstProduct     string
+	SpotinstOrientation string


What is the difference here? No worries about implementation, but I just want to understand what these things affect.

Both are cluster-wide options compared to those labels which can be used to toggle on/off some features at the instance group level.

SpotinstProduct is the product description associated with the Spot Instance while SpotinstOrientation is the prediction strategy (Valid values: balanced, cost, equal-distribution and availability. Defaults to balanced).

Can you document this at some point within kops as well? If we just add this and tell people that well if you want to use spotinst look at their docs could be a bad user experience. Just a few words with a link to a more detailed explanation at your own documentation would be sufficient.

nit: Same here - can you add this to our documentation? A few words and a link to more details on your website would be more then enough!

chrisz100 · 2018-10-11T15:12:53Z

cmd/kops/create_cluster.go

+
+	if featureflag.SpotinstIntegration.Enabled() {
+		// Spotinst flags
+		cmd.Flags().StringVar(&options.SpotinstProduct, "spotinst-product", options.SpotinstProduct, "Set the product code.")


What product codes do you offer?

It's the product description associated with the Spot Instance (see productDescription).

Valid values:

Linux/UNIX

Linux/UNIX (Amazon VPC)

Windows

Windows (Amazon VPC)

can you put a reference to this into the help output there? Without I see many people feeling lost and as it's just four that should be quite straight forward to show them the options

@chrisz100 Thanks, fixed.

chrisz100 · 2018-10-11T15:15:19Z

pkg/apis/kops/validation/aws.go

@@ -76,8 +76,10 @@ func awsValidateMachineType(fieldPath *field.Path, machineType string) field.Err
 	allErrs := field.ErrorList{}

 	if machineType != "" {
-		if _, err := awsup.GetMachineTypeInfo(machineType); err != nil {
-			allErrs = append(allErrs, field.Invalid(fieldPath, machineType, "machine type specified is invalid"))
+		for _, typ := range strings.Split(machineType, ",") {


machine type seemed to be a single machine string. how can this be multiple types at the same time now? Where does this come from?

Spotinst Elastigroup supports multiple instance types (and even weights, if needed), for example:

{ "group": { ... "compute": { ... "instanceTypes": { "ondemand": "c3.large", "spot": [ "c3.large", "c4.large", "m3.large", "r3.large" ] } ... } ... } }

By leveraging multiple instance types (and/or availability zones), Spot Instances are far less volatile as there is a far greater chance that Spotinst will find an unused instance for one of the fitting instance types.

Have you tested this with kops and if that doesn't backfire when not using spotinst?

It works fine for me after installing this fix.

Great, thanks!

chrisz100 · 2018-10-11T15:16:04Z

pkg/featureflag/featureflag.go

@@ -77,6 +77,9 @@ var GoogleCloudBucketAcl = New("GoogleCloudBucketAcl", Bool(false))
 // EnableNodeAuthorization enables the node authorization features
 var EnableNodeAuthorization = New("EnableNodeAuthorization", Bool(false))

+// SpotinstIntegration toggles the use of Spotinst integration.
+var SpotinstIntegration = New("SpotinstIntegration", Bool(false))


I'd keep this simpler - just call it "spotinst" - as you activate handling via spotinst and don't activate the possibility to use it.

chrisz100 · 2018-10-11T15:17:06Z

pkg/model/spotinstmodel/elastigroup.go

+		}
+
+		// Cloud config.
+		{


what's this { for?

chrisz100 · 2018-10-11T15:21:25Z

upup/pkg/fi/cloudup/apply_cluster.go

-
-			SecurityLifecycle: &securityLifecycle,
-		})
+		if featureflag.SpotinstIntegration.Enabled() {


I think we can just leave the deleted part intact and overwrite it if spotinst is active. That way we always have it set, no matter what happens and spotinst will just overload it.

chrisz100 · 2018-10-11T15:24:02Z

Also, please have a look at travis as the formatting is incorrect with upup/pkg/fi/cloudup/apply_cluster.go
FAIL: please execute make gofmt

chrisz100 · 2018-10-11T16:46:07Z

/assign @chrislovecnm @justinsb

chrisz100 · 2018-10-11T16:54:44Z

/ok-to-test

Thanks, appreciate it @liranp !

justinsb · 2018-10-11T19:34:37Z

cloudmock/aws/mockautoscaling/group.go

+		LaunchConfigurationName:          input.LaunchConfigurationName,
+		LoadBalancerNames:                input.LoadBalancerNames,
+		MaxSize:                          input.MaxSize,
+		MinSize:                          input.MinSize,


FYI go 1.11 changed gofmt, and we're still validating against go 1.10 gofmt rules (until k8s/kops 1.13 which moves to 1.11)

@justinsb Thanks, fixed.

justinsb · 2018-10-11T19:51:48Z

pkg/model/spotinstmodel/elastigroup.go

+		// Cloud config.
+		if cfg := b.Cluster.Spec.CloudConfig; cfg != nil {
+			// Product.
+			if cfg.SpotinstProduct != nil {


Nit: It looks like you don't need the nil check here (nor for SpotinstOrientation)

You're right! Fixed.

justinsb · 2018-10-11T20:14:32Z

pkg/model/spotinstmodel/elastigroup.go

+					group.UtilizeReservedInstances = fi.Bool(true)
+				} else if v == "false" {
+					group.UtilizeReservedInstances = fi.Bool(false)
+				}


Nit: A warning here is the value is "bad" would probably save you a lot of head-scratching. I typically like to strings.TrimSpace & strings.ToLower. Same for InstanceGroupLabelFallbackToOnDemand.

justinsb · 2018-10-12T01:55:03Z

pkg/model/spotinstmodel/elastigroup.go

+		if ig.Spec.MaxSize != nil {
+			maxSize = *ig.Spec.MaxSize
+		} else if ig.Spec.Role == kops.InstanceGroupRoleNode {
+			maxSize = 10


I think "pure" AWS uses 2 here - any reason to switch the default behaviour?

@justinsb don't we have some file to store default values like these? I am not a big fan of having default values like that somewhere in the models. Makes the behaviour so unpredictable in some cases.

Fixed (same as the AWS implementation).

justinsb · 2018-10-12T01:55:57Z

pkg/model/spotinstmodel/elastigroup.go

+						} else if v == "false" {
+							autoScalerDisabled = false
+						}
+						break


Maybe have a function to parse these boolean labels (not a blocker)?

justinsb · 2018-10-12T02:10:56Z

Gopkg.lock

+[[projects]]
+  branch = "master"
+  digest = "1:b410da742bf89fa5f255ef1d95bdf42d2a263a0c8f35272b8d5b4faff49017cf"
+  name = "github.com/spotinst/spotinst-sdk-go"


Can we add a LICENSE to this library please? I know Apache 2 is OK for kubernetes projects, not sure what other licenses are OK

justinsb · 2018-10-12T02:44:00Z

upup/pkg/fi/cloudup/awsup/aws_cloud.go

@@ -325,6 +336,10 @@ 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 {


This is going to make it hard to "graduate" spotinst from using a feature-flag - we'll probably need a field somewhere. But we can get this in and then figure it out.. One option that might help is that AWSCloud is an interface, so it might be possible to have a spotinst implementation (even if it delegated most of the functionality to an awsCloudImplementation)

I agree to @justinsb here - flagging spotinst to be active is one way of using it, but in the future it would be nice to just have it as a separate provider you select, that then falls back to the cloud if no special implementation was given. From what I understand you just use your own autoscaling implementation, but that also won't change that much for other clouds as you abstract that away in your own api's?

@chrisz100 You're right about our implementation. I'm not sure I'm understanding your suggestion. How it differs from the implementation introduced in #5195 (where we just wrapped the AWS implementation)?

justinsb · 2018-10-12T02:46:31Z

upup/pkg/fi/cloudup/spotinsttasks/elastigroup.go

+			if lc.UserData != nil {
+				userData, err := base64.StdEncoding.DecodeString(*lc.UserData)
+				if err != nil {
+					return nil, fmt.Errorf("spotinst: error decoding user data: %v", err)


FYI: I think (hope) when this is printed it'll be printed alongside the failing task, so you likely don't need the spotinst: bit. But definitely not a blocker - our error messages repeat themselves a lot already :-)

justinsb · 2018-10-12T02:51:19Z

Looks really great @liranp - I'm guessing we should do this before #5195.

A few nits, and some small questions, but really great stuff!

One blocker though: I'm reasonably sure we need a license on the library we're adding as a dependency. Ideally it would be Apache 2, though I think others are permitted. (I went looking for the list of approved licenses and couldn't immediately find it.) Do you want to add a license to github.com/spotinst/spotinst-sdk-go - and will it be Apache :-) ?

chrisz100 · 2018-10-12T12:23:09Z

pkg/model/awsmodel/autoscalinggroup.go

@@ -98,7 +99,7 @@ func (b *AutoscalingGroupModelBuilder) Build(c *fi.ModelBuilderContext) error {
 				},
 				IAMInstanceProfile: link,
 				ImageID:            s(ig.Spec.Image),
-				InstanceType:       s(ig.Spec.MachineType),
+				InstanceType:       s(strings.Split(ig.Spec.MachineType, ",")[0]),


Did you test what happens if you have just one or multiple but not with default aws behaviour here as well?

chrisz100 · 2018-10-12T19:55:42Z

As the license is in there now:
/lgtm

chrisz100 · 2018-10-12T20:41:53Z

/lgtm

liranp · 2018-10-12T20:50:59Z

Thank you @chrisz100 and @justinsb for being so responsive!

chrisz100 · 2018-10-12T20:56:59Z

@liranp this PR is.the only thing on the list before cutting the 1.11 alpha this weekend. Looking forward to it!

chrisz100

/lgtm

justinsb · 2018-10-15T14:14:56Z

/approve
/lgtm

Thanks @liranp

🎉

k8s-ci-robot · 2018-10-15T14:15:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrisz100, justinsb, liranp

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

chrisz100 · 2018-10-15T15:38:14Z

/retest

justinsb · 2018-10-15T16:00:32Z

Opened test-infra issue for flake, as it is weird: kubernetes/test-infra#9805

liranp · 2018-10-16T06:26:43Z

Thank you @chrisz100 and @justinsb, much appreciated!

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 11, 2018

k8s-ci-robot requested review from chrisz100 and rdrgmnzs October 11, 2018 14:44

liranp mentioned this pull request Oct 11, 2018

New cloud provider: Spotinst #5195

Closed

chrisz100 reviewed Oct 11, 2018

View reviewed changes

k8s-ci-robot assigned chrislovecnm and justinsb Oct 11, 2018

liranp force-pushed the feature-spotinst-aws branch from 4bae2a0 to 1a11498 Compare October 11, 2018 16:50

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 11, 2018

liranp force-pushed the feature-spotinst-aws branch from 631942e to a5bf55e Compare October 11, 2018 17:37

justinsb reviewed Oct 11, 2018

View reviewed changes

liranp force-pushed the feature-spotinst-aws branch from a5bf55e to 3750ab4 Compare October 11, 2018 21:46

justinsb reviewed Oct 12, 2018

View reviewed changes

chrisz100 reviewed Oct 12, 2018

View reviewed changes

k8s-ci-robot assigned chrisz100 Oct 12, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 12, 2018

liranp force-pushed the feature-spotinst-aws branch from adc9061 to 0b1d1e8 Compare October 12, 2018 20:23

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2018

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 14, 2018

liranp added 15 commits October 14, 2018 11:37

feature: new integration: spotinst

7654a92

deps: vendor dependencies

4c8ac60

fix: rename spotinst's feature flag

9f94c06

fix: don't use curly brackets for additional scoping

0b9ab26

fix: ignore additional instance types

46b1c70

fix: execute gofmt using go1.10

7ae8515

fix: boilerplate headers and packages list

6509e32

fix: add product descriptions to the help output

4ecc03a

fix: generate conversion functions (apimachinery)

eec98c6

deps: vendor spotinst-sdk-go

037b6a1

fix: remove unnecessary fmt.Errorf call

2faf3be

fix: max size defaults to 2

07376e5

fix: parse boolean values

cfa14b6

fix: remove unnecessary nil check

67fd316

fix: update doc comments

e1f85e0

liranp force-pushed the feature-spotinst-aws branch from 34aee59 to e1f85e0 Compare October 14, 2018 08:37

liranp added 2 commits October 14, 2018 11:54

fix: rename cluster_identifier field

a8db93b

fix: rename autoscaler node labels metadata label

07a509b

chrisz100 approved these changes Oct 14, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 14, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2018

k8s-ci-robot merged commit 1fbc633 into kubernetes:master Oct 15, 2018

liranp mentioned this pull request Feb 13, 2019

Add support for Spotinst jenkins-x/jx#3090

Merged

2 tasks

liranp deleted the feature-spotinst-aws branch January 22, 2020 14:58

liranp mentioned this pull request Feb 17, 2020

REQUEST: New membership for liranp kubernetes/org#1634

Closed

6 tasks

New integration: Spotinst #5922

New integration: Spotinst #5922

Conversation

liranp commented Oct 11, 2018

chrisz100 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liranp Oct 11, 2018 • edited

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

liranp Oct 11, 2018 • edited

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

chrisz100 commented Oct 11, 2018 • edited

chrisz100 commented Oct 11, 2018

chrisz100 commented Oct 11, 2018

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

liranp Oct 12, 2018 • edited

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

justinsb commented Oct 12, 2018

Choose a reason for hiding this comment

chrisz100 commented Oct 12, 2018

chrisz100 commented Oct 12, 2018

liranp commented Oct 12, 2018

chrisz100 commented Oct 12, 2018

chrisz100 left a comment

Choose a reason for hiding this comment

justinsb commented Oct 15, 2018

k8s-ci-robot commented Oct 15, 2018

chrisz100 commented Oct 15, 2018

justinsb commented Oct 15, 2018

liranp commented Oct 16, 2018

chrisz100 left a comment •

edited

liranp Oct 11, 2018 •

edited

liranp Oct 11, 2018 •

edited

chrisz100 commented Oct 11, 2018 •

edited

liranp Oct 12, 2018 •

edited