Add support for Spot block in launch template #8802

thejasbabu · 2020-03-26T16:03:37Z

Launch configuration does not support the field SpotDurationInMinutes which is used to reserve the spot instances, but however Launch Template does

k8s-ci-robot · 2020-03-26T16:03:45Z

Welcome @thejasbabu!

It looks like this is your first PR to kubernetes/kops 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kops has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-03-26T16:03:46Z

Hi @thejasbabu. 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.

rifelpet · 2020-03-26T16:10:43Z

/ok-to-test

thejasbabu · 2020-03-27T07:26:50Z

/assign @chrislovecnm

hakman · 2020-03-27T13:54:14Z

@thejasbabu Thanks for spending the time to add this. Could you address a few things:

an integration test should be added here https://github.com/kubernetes/kops/tree/master/tests/integration/update_cluster/mixed_instances_spot (you can add a new instance group or create a new test)
I don't see any code for CloudFormation, just for Terraform

Once you add these should be easier to see if this works correctly.

gjtempleton · 2020-03-27T16:21:27Z

pkg/model/awsmodel/autoscalinggroup.go

@@ -131,6 +131,9 @@ func (b *AutoscalingGroupModelBuilder) buildLaunchTemplateTask(c *fi.ModelBuilde
 	if ig.Spec.MixedInstancesPolicy == nil {
 		lt.SpotPrice = lc.SpotPrice
 	}
+	if ig.Spec.SpotDurationInMinutes != nil {
+		lt.SpotDurationInMinutes = ig.Spec.SpotDurationInMinutes


Should we do some validation of the value provided by the user given that these values have to be a multiple of 60?

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-launchtemplate-launchtemplatedata-instancemarketoptions-spotoptions.html

Added validation for this. Thanks!

Added the integration test to test spotblock

thejasbabu · 2020-03-27T17:54:03Z

@hakman Thanks for the feedback.

I have added the code for CloudFormation output
Will be adding the integration test by sometime tomorrow, can you redirect me to docs on how to run these integration tests?

pkg/apis/kops/validation/aws.go

thejasbabu · 2020-03-29T15:53:15Z

/retest

rifelpet · 2020-03-29T16:27:42Z

looks like the test is failing due to whitespace issues in the integration test expected output. You can run hack/updated-expected.sh to fix it

rifelpet · 2020-03-30T04:24:40Z

tests/integration/update_cluster/spotblock/in-v1alpha2.yaml

+  role: Node
+  instanceProtection: true
+  maxPrice: "0.1"
+  spotDurationInMinutes: 120


Apologies for the runaround but we're doing some significant refactoring in the integration tests (see #8737) and the timing of that and some other PRs is going to cause a merge conflict here.

I think it would be simpler to add this field to the existing launch_templates test cluster rather than creating a new integration test cluster. You could delete tests/integration/update_cluster/spotblock and revert your changes in cmd/kops/integration_test.go, then add the new field to tests/integration/update_cluster/launch_templates/in-v1alpha2.yaml and rerun ./hack/update-expected.sh once more. It will produce a much smaller PR and wont conflict with the other changes we're making.

I hope this makes sense, let me know if you have any questions.

I see that #8737 is not yet merged, will the above thing work if I rebase this branch with master?

@rifelpet meant that would be easier for him to rebase and modify #8737 if you add this instance group to the existing launch_templates test instead of creating a new one.

Sorry for my late reply. I've been a bit busy over the last few days.

There's few complication in adding the integration test as part of launch_templates. The TestLaunchTemplatesASG fails since the method runTestAWS is made for one type of node, now when I add a new asg with spot-node, the actual is correct but the expected is mis formed because of runTestAWS method.

Should I refactor the runTestAWS to run for multiple nodes for single kube-cluster?

Also there's an assumption that the instance group name is always nodes
https://github.com/kubernetes/kops/blob/master/cmd/kops/integration_test.go#L486

In this case, could you just add these to the existing IG for launch_templates?

maxPrice: "0.1" spotDurationInMinutes: 120

Seems like a quick fix, but will do.

hakman · 2020-03-30T10:17:19Z

Seems that tests are failing because of missing of the imports order in aws_test.go. You should run make goimports to fix it.

hakman · 2020-03-30T10:22:53Z

Considering that the list of accepted values is pretty small, maybe we should just list the accepted values instead of this logic? What do you think @gjtempleton @johngmyers ?

kops/pkg/apis/kops/validation/aws.go

Lines 115 to 126 in c511921

    
           if ig.Spec.SpotDurationInMinutes != nil { 
        
           	if *ig.Spec.SpotDurationInMinutes < 60 { 
        
           		errDetails := fmt.Sprintf("cannot be less than 60") 
        
           		allErrs = append(allErrs, field.Invalid(fieldPath, ig.Spec.SpotDurationInMinutes, errDetails)) 
        
           	} else if *ig.Spec.SpotDurationInMinutes > 360 { 
        
           		errDetails := fmt.Sprintf("cannot be greater than 360") 
        
           		allErrs = append(allErrs, field.Invalid(fieldPath, ig.Spec.SpotDurationInMinutes, errDetails)) 
        
           	} else if *ig.Spec.SpotDurationInMinutes%60 != 0 { 
        
           		errDetails := fmt.Sprintf("must be a multiple of 60") 
        
           		allErrs = append(allErrs, field.Invalid(fieldPath, ig.Spec.SpotDurationInMinutes, errDetails)) 
        
           	} 
        
           }

johngmyers · 2020-03-30T21:29:02Z

An explicit list checked with validation.IsValidValue() would indeed be better.

thejasbabu · 2020-03-31T07:33:16Z

@hakman @johngmyers Updated the validation to be just a basic value check. Let me know if there's any other changes that needs to be done 👍

pkg/apis/kops/validation/aws.go

tests/integration/update_cluster/launch_templates/in-v1alpha2.yaml

hakman · 2020-03-31T08:13:44Z

Thanks @thejasbabu. At the moment looks good to me. Please also squash the commits and rebase. It is a pretty small change to separate into multiple commits.

- Launch configuration does not support the field SpotDurationInMinutes which is used to reserve the spot instances, but however Launch Template does

hakman · 2020-03-31T16:11:22Z

/lgtm

rifelpet · 2020-04-01T16:33:39Z

This looks great, thanks for sticking with it!

/approve

k8s-ci-robot · 2020-04-01T16:34:30Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 26, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 26, 2020

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 26, 2020

k8s-ci-robot requested review from joshbranham and robinpercy March 26, 2020 16:04

k8s-ci-robot added area/api area/provider/aws Issues or PRs related to aws provider labels Mar 26, 2020

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 Mar 26, 2020

k8s-ci-robot assigned chrislovecnm Mar 27, 2020

gjtempleton reviewed Mar 27, 2020

View reviewed changes

thejasbabu force-pushed the feature/spot-block branch from 7805166 to 657d1dd Compare March 28, 2020 19:20

johngmyers requested changes Mar 29, 2020

View reviewed changes

pkg/apis/kops/validation/aws.go Outdated Show resolved Hide resolved

pkg/apis/kops/validation/aws.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 29, 2020

thejasbabu force-pushed the feature/spot-block branch 2 times, most recently from 5eab43d to 7be43b7 Compare March 29, 2020 16:55

rifelpet reviewed Mar 30, 2020

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 30, 2020

thejasbabu force-pushed the feature/spot-block branch from c511921 to 010124d Compare March 30, 2020 10:47

thejasbabu force-pushed the feature/spot-block branch 2 times, most recently from 5f40853 to 03adc95 Compare March 31, 2020 07:26

hakman requested changes Mar 31, 2020

View reviewed changes

pkg/apis/kops/validation/aws.go Outdated Show resolved Hide resolved

hakman reviewed Mar 31, 2020

View reviewed changes

tests/integration/update_cluster/launch_templates/in-v1alpha2.yaml Outdated Show resolved Hide resolved

thejasbabu force-pushed the feature/spot-block branch from 03adc95 to e89163e Compare March 31, 2020 07:49

thejasbabu force-pushed the feature/spot-block branch from e89163e to 7cbcad9 Compare March 31, 2020 14:33

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 31, 2020

Add support for Spot block in launch template

dda8dc3

- Launch configuration does not support the field SpotDurationInMinutes which is used to reserve the spot instances, but however Launch Template does

thejasbabu force-pushed the feature/spot-block branch from 7cbcad9 to dda8dc3 Compare March 31, 2020 14:37

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 31, 2020

k8s-ci-robot assigned hakman Mar 31, 2020

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

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

k8s-ci-robot merged commit 759e24a into kubernetes:master Apr 1, 2020

k8s-ci-robot added this to the v1.18 milestone Apr 1, 2020

martin-tomes mentioned this pull request Apr 20, 2020

Support for Launch Template - Instance Interruption Behaviour #8809

Closed

thejasbabu deleted the feature/spot-block branch May 29, 2020 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Spot block in launch template #8802

Add support for Spot block in launch template #8802

thejasbabu commented Mar 26, 2020

k8s-ci-robot commented Mar 26, 2020

k8s-ci-robot commented Mar 26, 2020

rifelpet commented Mar 26, 2020

thejasbabu commented Mar 27, 2020

hakman commented Mar 27, 2020

gjtempleton Mar 27, 2020

thejasbabu Mar 27, 2020

thejasbabu Mar 29, 2020

thejasbabu commented Mar 27, 2020 •

edited

Loading

thejasbabu commented Mar 29, 2020

rifelpet commented Mar 29, 2020

rifelpet Mar 30, 2020

thejasbabu Mar 30, 2020

hakman Mar 30, 2020

hakman Mar 30, 2020

thejasbabu Mar 30, 2020

thejasbabu Mar 30, 2020

hakman Mar 30, 2020

thejasbabu Mar 30, 2020

hakman commented Mar 30, 2020

hakman commented Mar 30, 2020

johngmyers commented Mar 30, 2020

thejasbabu commented Mar 31, 2020 •

edited

Loading

hakman commented Mar 31, 2020

hakman commented Mar 31, 2020

rifelpet commented Apr 1, 2020

k8s-ci-robot commented Apr 1, 2020

Add support for Spot block in launch template #8802

Add support for Spot block in launch template #8802

Conversation

thejasbabu commented Mar 26, 2020

k8s-ci-robot commented Mar 26, 2020

k8s-ci-robot commented Mar 26, 2020

rifelpet commented Mar 26, 2020

thejasbabu commented Mar 27, 2020

hakman commented Mar 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thejasbabu commented Mar 27, 2020 • edited Loading

thejasbabu commented Mar 29, 2020

rifelpet commented Mar 29, 2020

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

hakman commented Mar 30, 2020

hakman commented Mar 30, 2020

johngmyers commented Mar 30, 2020

thejasbabu commented Mar 31, 2020 • edited Loading

hakman commented Mar 31, 2020

hakman commented Mar 31, 2020

rifelpet commented Apr 1, 2020

k8s-ci-robot commented Apr 1, 2020

thejasbabu commented Mar 27, 2020 •

edited

Loading

thejasbabu commented Mar 31, 2020 •

edited

Loading