Skip to content

Support 'Context' on ec2.CreateFleet API call#1395

Closed
gjmveloso wants to merge 13 commits intoaws:mainfrom
gjmveloso:aws/context-on-ec2-fleet
Closed

Support 'Context' on ec2.CreateFleet API call#1395
gjmveloso wants to merge 13 commits intoaws:mainfrom
gjmveloso:aws/context-on-ec2-fleet

Conversation

@gjmveloso
Copy link
Copy Markdown
Contributor

@gjmveloso gjmveloso commented Feb 21, 2022

1. Issue, if available:
#1394

2. Description of changes:
When the AWS Cloud Provider issues a CreateFleet API call as part of instance.launchInstances() a Context parameter as string will be added if fleetContext was set

3. How was this change tested?
N/A

4. Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: link to issue
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@netlify
Copy link
Copy Markdown

netlify bot commented Feb 21, 2022

✅ Deploy Preview for karpenter-docs-prod canceled.

🔨 Explore the source changes: fc7adc1

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/6238d2c60ad4c200089c7049

@bwagner5 bwagner5 self-requested a review February 22, 2022 22:55
@bwagner5
Copy link
Copy Markdown
Contributor

I think making the Fleet Context a CLI parameter might be better since not many users will need to touch it.

Here's where the CLI args are defined:

https://github.com/aws/karpenter/blob/main/pkg/utils/options/options.go#L43-L45

The options struct is injected into the context so that the cloud provider can use them. Here's an example:
https://github.com/aws/karpenter/blob/main/pkg/cloudprovider/aws/launchtemplate.go#L124

@ellistarn
Copy link
Copy Markdown
Contributor

I'd like to hold on this feature, since it tightly couples Karpenter to the fleet API and leaks Fleet details through the API. This will break if Karpenter ever decides to use ASG for RunInstances.

@ellistarn ellistarn added the blocked Unable to make progress due to some dependency label Feb 23, 2022
@bwagner5
Copy link
Copy Markdown
Contributor

I'd like to hold on this feature, since it tightly couples Karpenter to the fleet API and leaks Fleet details through the API. This will break if Karpenter ever decides to use ASG for RunInstances.

FYI ASG supports Context as well :)

https://docs.aws.amazon.com/cli/latest/reference/autoscaling/create-auto-scaling-group.html#:~:text=instance%2Dlifetime%20%3Cvalue%3E%5D-,%5B%2D%2Dcontext%20%3Cvalue%3E%5D,-%5B%2D%2Ddesired%2Dcapacity%2Dtype

@gjmveloso
Copy link
Copy Markdown
Contributor Author

I'd like to hold on this feature, since it tightly couples Karpenter to the fleet API and leaks Fleet details through the API. This will break if Karpenter ever decides to use ASG for RunInstances.

FYI ASG supports Context as well :)

https://docs.aws.amazon.com/cli/latest/reference/autoscaling/create-auto-scaling-group.html#:~:text=instance%2Dlifetime%20%3Cvalue%3E%5D-,%5B%2D%2Dcontext%20%3Cvalue%3E%5D,-%5B%2D%2Ddesired%2Dcapacity%2Dtype

Probably we would then change the options var from AWSEC2FleetContext to AWSCapacityLaunchContext

@ellistarn
Copy link
Copy Markdown
Contributor

Given that this is also included in ASG, I'm much more comfortable with the idea of exposing it through the API.
I can potentially see

spec:
  provider:
    context:

or

spec:
  provider:
    launchContext:

AWSNodeNameConvention string
AWSENILimitedPodDensity bool
AWSDefaultInstanceProfile string
AWSEC2FleetContext string
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we're serious about exposing this, we should absolutely do so at the provisioner level. The only reason to make this a CLI arg is to enable this without promoting to the full API. There are compatibility concerns with CLI args too, so it doesn't get it fully out of the woods.

Is it interesting to use multiple fleet contexts in a single cluster?

@gjmveloso
Copy link
Copy Markdown
Contributor Author

Given that this is also included in ASG, I'm much more comfortable with the idea of exposing it through the API. I can potentially see

spec:
  provider:
    context:

or

spec:
  provider:
    launchContext:

Reverted to the original code changes and renamed fleetContext to context, more aligned with your last suggestion

@bwagner5 bwagner5 removed the blocked Unable to make progress due to some dependency label Mar 16, 2022
Copy link
Copy Markdown
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

a few comments to simplify the code a bit.

@bwagner5 bwagner5 self-assigned this Mar 16, 2022
@gjmveloso
Copy link
Copy Markdown
Contributor Author

a few comments to simplify the code a bit.

Done ;)

Copy link
Copy Markdown
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

can you run make verify as well

@ellistarn ellistarn changed the title Initial work on supporting 'Context' on ec2.CreateFleet API call Support 'Context' on ec2.CreateFleet API call Mar 21, 2022
@gjmveloso
Copy link
Copy Markdown
Contributor Author

can you run make verify as well

receiving the following error:

./hack/toolchain.sh
go: downloading github.com/norwoodj/helm-docs v1.7.0
go install: github.com/norwoodj/helm-docs/cmd/helm-docs@v1.7.0: github.com/norwoodj/helm-docs@v1.7.0: verifying module: checksum mismatch
        downloaded: h1:ibDij3KZY3nDKN62bYqxD8GlSvr89FagIfl9tEd5QA4=
        sum.golang.org: h1:m+IHHdNs8QEo2y5ooLFr7NuhLTpTuyLTOhGZfa1DgMU=

SECURITY ERROR
This download does NOT match the one reported by the checksum server.
The bits may have been replaced on the origin server, or an attacker may
have intercepted the download attempt.

For more information, see 'go help module-auth'.
make: *** [toolchain] Error 1

@tzneal
Copy link
Copy Markdown
Contributor

tzneal commented Mar 21, 2022

It looks like the author of that package re-tagged it. Try the following:

GOSUMDB=off ./hack/toolchain.sh

@ellistarn
Copy link
Copy Markdown
Contributor

can you run make verify as well

receiving the following error:

./hack/toolchain.sh
go: downloading github.com/norwoodj/helm-docs v1.7.0
go install: github.com/norwoodj/helm-docs/cmd/helm-docs@v1.7.0: github.com/norwoodj/helm-docs@v1.7.0: verifying module: checksum mismatch
        downloaded: h1:ibDij3KZY3nDKN62bYqxD8GlSvr89FagIfl9tEd5QA4=
        sum.golang.org: h1:m+IHHdNs8QEo2y5ooLFr7NuhLTpTuyLTOhGZfa1DgMU=

SECURITY ERROR
This download does NOT match the one reported by the checksum server.
The bits may have been replaced on the origin server, or an attacker may
have intercepted the download attempt.

For more information, see 'go help module-auth'.
make: *** [toolchain] Error 1

This is a bug upstream with helm-docs. You can just comment it out from toolchain.sh


The `Context` parameter that all underlying calls to [EC2 CreateFleet](https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_CreateFleet.html) made by the Provisioner would use.

Note: Only use this parameter if advised by AWS.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd prefer to simply not document this if this is the case.

@bwagner5
Copy link
Copy Markdown
Contributor

I was testing this out and noticed that Context cannot be used with $Latest and $Default LTs which is what Karpenter uses, so this will not work in Karpenter right now. We'll have to hold on this until we figure out why.

@bwagner5 bwagner5 added the blocked Unable to make progress due to some dependency label Mar 21, 2022
@bwagner5
Copy link
Copy Markdown
Contributor

Rebased and added test in a new PR (with all of your commits as well). Will test and get that new PR merged soon: #2007

@bwagner5 bwagner5 closed this Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked Unable to make progress due to some dependency

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants