Spotinst: New hybrid integration mode #7252

liranp · 2019-07-15T16:46:30Z

Description

This PR allows users to gradually integrate with Spotinst.

When the Spotinst feature flag is toggled on (export KOPS_FEATURE_FLAGS="+Spotinst"), kops manages all instance groups through the Spotinst API using both Spotinst Elastigroup and Spotinst Ocean.

The new hybrid integration mode allows users to continue managing their instance groups through AWS Auto Scaling Groups, except for specific instance groups labeled with a predefined metadata label.

Usage

Toggle on the hybrid integration feature flag:

export KOPS_FEATURE_FLAGS="+Spotinst,SpotinstHybrid"

Create a new instance group or update an existing one:

apiVersion: kops/v1alpha2
kind: InstanceGroup
metadata:
  labels:
    ...
    spotinst.io/hybrid: "true"
    ...
spec:
  ...

k8s-ci-robot · 2019-07-15T16:46:38Z

Hi @liranp. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rdrgmnzs · 2019-07-17T22:06:56Z

/ok-to-test

justinsb · 2019-07-19T13:12:23Z

pkg/model/spotinstmodel/instance_group.go

@@ -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"


This is a cool idea. One thing we could also do (with the move to adopt cluster-api), is to actually create a "provider" or "manager" or "strategy" field on the InstanceGroup. My personal choice would be provider, but I don't have strong feelings.

Yeah, I agree. It'd be great.

BTW, do you think we should change the metadata label to spotinst.io/hybrid: "true" (instead of spotinst.io/managed: "true") so it will better match the feature flag?

I do like the idea of matching the feature flag there

Well, we already have customers that use this feature using a custom build of kops and we prefer to not introduce a breaking change by renaming the label. So, we've changed the implementation to support both cases.

justinsb · 2019-07-19T13:15:47Z

upup/pkg/fi/cloudup/apply_cluster.go

-				AWSModelContext:   awsModelContext,
+			}
+
+			spotinstModelBuilder := &spotinstmodel.InstanceGroupModelBuilder{


Moving this outside the featureflag looks scary, but it's fine :-)

Got it, fixed.

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

justinsb · 2019-07-19T13:19:45Z

Looks good. One query on the deletion logic.

cluster-api is also going to have similar switching of behaviour, so we could define a field on the InstanceGroup that switches how an instancegroup is created (AWS ASG vs SpotInst vs cluster-api). But that doesn't need to block this PR.

liranp · 2019-07-21T09:58:11Z

@justinsb Thank you so much! I'd love to hear your opinion about changing the metadata label. Not sure which one is better.

liranp · 2019-08-05T09:41:59Z

@justinsb Sorry to bother you, but can you please go over this? Thanks!

liranp · 2019-09-04T10:21:23Z

Anyone? Thanks!

liranp · 2019-10-02T11:03:05Z

Bumping... anyone? Thanks.

chrisz100 · 2019-10-10T11:14:24Z

/retest

liranp · 2019-11-07T18:40:31Z

/assign @rdrgmnzs

liranp · 2019-11-24T11:05:29Z

Bumping... anyone? Thanks.

johngmyers · 2019-12-23T03:30:33Z

/retest

liranp · 2020-01-18T17:02:35Z

Bumping... anyone? Thanks.

bycEEE · 2020-02-21T17:46:58Z

Bump as well, would like to see this in the official binary instead of using a custom one.

k8s-ci-robot · 2020-02-23T09:51:07Z

@liranp: you cannot LGTM your own PR.

In response to this:

/approve
/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fejta-bot · 2020-05-23T13:19:23Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

liranp · 2020-05-23T18:48:20Z

/remove-lifecycle stale

rifelpet · 2020-05-23T19:13:40Z

this looks good to me, sorry for the delay!
/lgtm
/approve

k8s-ci-robot · 2020-05-23T19:13:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liranp, rifelpet

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~~ [rifelpet]

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

liranp · 2020-05-23T19:17:37Z

Thank you so much, @rifelpet!

estahn · 2020-05-23T22:30:57Z

@liranp Will you provide documentation?

liranp · 2020-05-23T22:35:29Z

@estahn Already did that, see #9139. The documentation is available here.

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. labels Jul 15, 2019

k8s-ci-robot requested review from chrisz100 and rdrgmnzs July 15, 2019 16:46

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 15, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 17, 2019

liranp force-pushed the feature-spotinst-hybrid branch from 928f3ec to 7bbd43f Compare July 18, 2019 10:08

justinsb reviewed Jul 19, 2019

View reviewed changes

liranp force-pushed the feature-spotinst-hybrid branch from 7bbd43f to 163f878 Compare July 21, 2019 09:46

liranp force-pushed the feature-spotinst-hybrid branch from 163f878 to fdb6a98 Compare August 14, 2019 13:28

liranp force-pushed the feature-spotinst-hybrid branch from fdb6a98 to 6e897b8 Compare September 12, 2019 12:56

liranp force-pushed the feature-spotinst-hybrid branch from 6e897b8 to 07aef9a Compare October 2, 2019 10:22

liranp force-pushed the feature-spotinst-hybrid branch from 07aef9a to 24fe9e0 Compare October 10, 2019 09:03

liranp force-pushed the feature-spotinst-hybrid branch 3 times, most recently from 1ca8767 to 9df66ec Compare October 29, 2019 14:25

liranp force-pushed the feature-spotinst-hybrid branch 2 times, most recently from cb4b21e to d1d4152 Compare November 7, 2019 17:47

k8s-ci-robot assigned rdrgmnzs Nov 7, 2019

liranp force-pushed the feature-spotinst-hybrid branch from d1d4152 to ebe0bce Compare December 22, 2019 20:26

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 22, 2019

This was referenced Feb 12, 2020

Spotinst: Add OWNERS file to Spotinst specific packages #8533

Merged

REQUEST: New membership for liranp kubernetes/org#1634

Closed

liranp force-pushed the feature-spotinst-hybrid branch from ebe0bce to ef96a0b Compare February 23, 2020 13:03

k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 23, 2020

liranp added 2 commits May 23, 2020 21:39

feat(spotinst): new hybrid mode

23c0cda

fix(spotinst): rename the label to match the feature flag

04d83c6

liranp force-pushed the feature-spotinst-hybrid branch from ef96a0b to 04d83c6 Compare May 23, 2020 18:40

k8s-ci-robot added area/provider/aws Issues or PRs related to aws provider and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 23, 2020

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 23, 2020

k8s-ci-robot assigned rifelpet May 23, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 23, 2020

k8s-ci-robot merged commit af2d6b1 into kubernetes:master May 23, 2020

k8s-ci-robot added this to the v1.18 milestone May 23, 2020

liranp deleted the feature-spotinst-hybrid branch May 23, 2020 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spotinst: New hybrid integration mode #7252

Spotinst: New hybrid integration mode #7252

liranp commented Jul 15, 2019 •

edited

Loading

k8s-ci-robot commented Jul 15, 2019

rdrgmnzs commented Jul 17, 2019

justinsb Jul 19, 2019

liranp Jul 21, 2019

liranp Jul 21, 2019

chrisz100 Oct 10, 2019

liranp Oct 27, 2019

justinsb Jul 19, 2019

liranp Jul 21, 2019

justinsb commented Jul 19, 2019

liranp commented Jul 21, 2019

liranp commented Aug 5, 2019

liranp commented Sep 4, 2019

liranp commented Oct 2, 2019

chrisz100 commented Oct 10, 2019

liranp commented Nov 7, 2019

liranp commented Nov 24, 2019

johngmyers commented Dec 23, 2019

liranp commented Jan 18, 2020

bycEEE commented Feb 21, 2020

k8s-ci-robot commented Feb 23, 2020

fejta-bot commented May 23, 2020

liranp commented May 23, 2020

rifelpet commented May 23, 2020

k8s-ci-robot commented May 23, 2020

liranp commented May 23, 2020

estahn commented May 23, 2020

liranp commented May 23, 2020

Spotinst: New hybrid integration mode #7252

Spotinst: New hybrid integration mode #7252

Conversation

liranp commented Jul 15, 2019 • edited Loading

Description

Usage

k8s-ci-robot commented Jul 15, 2019

rdrgmnzs commented Jul 17, 2019

justinsb Jul 19, 2019

Choose a reason for hiding this comment

liranp Jul 21, 2019

Choose a reason for hiding this comment

liranp Jul 21, 2019

Choose a reason for hiding this comment

chrisz100 Oct 10, 2019

Choose a reason for hiding this comment

liranp Oct 27, 2019

Choose a reason for hiding this comment

justinsb Jul 19, 2019

Choose a reason for hiding this comment

liranp Jul 21, 2019

Choose a reason for hiding this comment

justinsb commented Jul 19, 2019

liranp commented Jul 21, 2019

liranp commented Aug 5, 2019

liranp commented Sep 4, 2019

liranp commented Oct 2, 2019

chrisz100 commented Oct 10, 2019

liranp commented Nov 7, 2019

liranp commented Nov 24, 2019

johngmyers commented Dec 23, 2019

liranp commented Jan 18, 2020

bycEEE commented Feb 21, 2020

k8s-ci-robot commented Feb 23, 2020

fejta-bot commented May 23, 2020

liranp commented May 23, 2020

rifelpet commented May 23, 2020

k8s-ci-robot commented May 23, 2020

liranp commented May 23, 2020

estahn commented May 23, 2020

liranp commented May 23, 2020

liranp commented Jul 15, 2019 •

edited

Loading