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

feat: Dedicated host api structure #3859

Closed

Conversation

preflightsiren
Copy link

@preflightsiren preflightsiren commented May 5, 2023

Fixes #3182

Description

Wanted to create a draft PR to start a conversation about the approach. I believe following https://karpenter.sh/v0.27.3/contributing/design-guide/ that adding to the AWSNodeTemplate best follows "Identify an opinionated default that solves the majority of use cases."
Of course I'd appreciate any and all help to mould this design.

This design focuses on a simplified configuration, just the minimum needed to use host resource groups.

apiVersion: karpenter.k8s.aws/v1alpha1
kind: AWSNodeTemplate
metadata:
  name: default
spec:
  subnetSelector: { ... }        # required, discovers tagged subnets to attach to instances
  securityGroupSelector: { ... } # required, discovers tagged security groups to attach to instances
  licenseConfiguration: 
    - arn:aws:license-manager:eu-east-1:123456789012:license-configuration:lic-edf7f9e241f5e16f29996c842111f448
  placement:
    hostResourceGroup: arn:aws:resource-groups:us-east-1:123456789012:group/my-hrg-name

This design directly maps https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-launchtemplate-launchtemplatedata.html#cfn-ec2-launchtemplate-launchtemplatedata-placement and would therefore allow more reuse of features - eg. if people wanted to use licenses without dedicated hosts.

How was this change tested?

  • unit tests
  • e2e testing in our aws environment - screenshots attached.

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

Release Note


* Adds support for configuring AWS Node Templates to launch instances on Dedicated Hosts.

** Questions for the maintainers **

  • Are there other cases for testing? I'm thinking in particular validation of strings being ARNs, but the aws API will validate it on the ec2.CreateFleet() call.
  • How would you like a feature like this to be documented? I've extended the example NodeTemplate, but I would totally understand if these features shouldn't be front-and-centre.

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

@netlify
Copy link

netlify bot commented May 5, 2023

Deploy Preview for karpenter-docs-prod ready!

Name Link
🔨 Latest commit dd38aa1
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/64ba1ed6a8a97e0008ef5ef1
😎 Deploy Preview https://deploy-preview-3859--karpenter-docs-prod.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@bwagner5
Copy link
Contributor

bwagner5 commented May 5, 2023

I would favor the second approach that maps directly for this feature. The reason you mentioned seems strong; i.e. a license config without a placement.

There are some fields missing from the placement structure as well. I don't think we'd want to add AvailabilityZone or Spread but the others seem like something that should be included.

@njtran njtran added the needs-design Design required label May 8, 2023
@preflightsiren
Copy link
Author

Thanks @bwagner5 , the use cases is the same as described in #3182

In order to utilise aws dedicated hosts for financial control reasons, we use LaunchTemplates to configure those 3 fields only (license, host group, tenancy).

My understanding of your comment is you'd prefer to see those fields added to https://github.com/aws/karpenter/blob/main/pkg/apis/v1alpha1/provider.go#L51

@njtran This work would be covered by https://github.com/aws/karpenter/blob/main/designs/aws-launch-templates-v2.md I believe, as we're really only talking about

With this approach, we’ll continue to add overrides in our providerSpec based on user appetite, accepting PRs that solve a basic / common use case and asking all others to specify an entire launch template.

do you agree?

@preflightsiren preflightsiren force-pushed the feat-3182-dedicated-hosts branch 2 times, most recently from f318aa4 to e0f9a8f Compare May 12, 2023 02:51
@preflightsiren
Copy link
Author

@bwagner5 @njtran I've added a design document for dedicated hosts following the examples of some of the other designs.

Please let me know how I can help with the design process.

I'm going to look at what an implementation needs in terms of effort, so I'm in a better position to implement whatever the decided design is.

@preflightsiren
Copy link
Author

I've just pushed up some MVP/basic plumbing to validate getting the fields from the AWSNodeTemplate and passing them down to the ec2.CreateLaunchTemplate request. Tested and confirmed in AWS Cloudtrail

"Placement": {
  "HostResourceGroupArn": "sebtest hrg arn"
},
"LicenseSpecification": {
  "tag": 1,
  "LicenseConfigurationArn": "sebtest license spec"
},

with expected error:

2023-05-15T14:58:56.264+1000    ERROR   controller      Reconciler error        {"controller": "machine_lifecycle", "controllerGroup": "karpenter.sh", "controllerKind": "Machine", "Machine": {"name":"default-v27zx"}, "namespace": "", "name": "default-v27zx", "reconcileID": "fc5ff202-e96f-4a2b-931a-b61b853c0e19", "error": "creating machine, creating instance, getting launch template configs, getting launch templates, creating launch template, InvalidLicenseConfigurationId.Malformed: The ARN ‘sebtest license spec’ is not valid. The expected format is arn:aws:license-manager:us-west-2:<account-id>:license-configuration:<license-configuration-id>.\n\tstatus code: 400, request id: 06842a16-57ee-4759-818f-084fdc08fa12"}

@preflightsiren preflightsiren force-pushed the feat-3182-dedicated-hosts branch 2 times, most recently from 0656c2d to a1c7fbc Compare May 17, 2023 03:16
@preflightsiren
Copy link
Author

Ran this branch on our local cluster, and was able to start instances on dedicated hosts.

Screenshot 2023-05-23 at 4 56 40 pm Screenshot 2023-05-23 at 4 55 49 pm

@njtran
Copy link
Contributor

njtran commented May 31, 2023

Maybe good here to consider #3324 as the probable API surface looks similar. Any thoughts here @preflightsiren ?

@preflightsiren
Copy link
Author

Maybe good here to consider #3324 as the probable API surface looks similar. Any thoughts here @preflightsiren ?

So far I've only implemented licenseSpecification and placement.HostResourceGroupArn. Do you have a sense of how much you'd want to implement for #3324 - I guess regardless of the scope if we stick to the AWS LT API they're going to play nice together :)

@preflightsiren preflightsiren force-pushed the feat-3182-dedicated-hosts branch 2 times, most recently from d1fe300 to 868150a Compare June 2, 2023 01:12
@preflightsiren preflightsiren marked this pull request as ready for review June 5, 2023 10:16
@preflightsiren preflightsiren requested a review from a team as a code owner June 5, 2023 10:16
@preflightsiren preflightsiren changed the title feat: WIP: Dedicated host api structure feat: Dedicated host api structure Jun 5, 2023
@preflightsiren preflightsiren force-pushed the feat-3182-dedicated-hosts branch 2 times, most recently from 1d93a3d to f8b8f1e Compare June 9, 2023 06:12
@preflightsiren
Copy link
Author

Added fix for #3324 as well

Copy link
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.

nice! great progress! I just made a few comments, mostly some casing nits and some simplifications :) Nice work!

pkg/apis/crds/karpenter.k8s.aws_awsnodetemplates.yaml Outdated Show resolved Hide resolved
pkg/apis/crds/karpenter.k8s.aws_awsnodetemplates.yaml Outdated Show resolved Hide resolved
pkg/apis/v1alpha1/awsnodetemplate.go Outdated Show resolved Hide resolved
pkg/apis/v1alpha1/awsnodetemplate.go Outdated Show resolved Hide resolved
pkg/providers/amifamily/resolver.go Outdated Show resolved Hide resolved
pkg/apis/v1alpha1/awsnodetemplate.go Outdated Show resolved Hide resolved
pkg/providers/amifamily/resolver.go Outdated Show resolved Hide resolved
pkg/providers/launchtemplate/launchtemplate.go Outdated Show resolved Hide resolved
website/content/en/preview/concepts/node-templates.md Outdated Show resolved Hide resolved
@preflightsiren
Copy link
Author

Thanks @bwagner5 appreciate it.

// https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-launchtemplate-launchtemplatedata.html#cfn-ec2-launchtemplate-launchtemplatedata-placement
type Placement struct {
// +optional
HostResourceGroupARN *string `json:"hostResourceGroupARN,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed this PR with the team today. Currently, we don't have any other api fields in Karpenter's CRDs that reference an ARN. Could we make the HostResourceGroup referenced by name and then construct the arn for input within Karpenter?

Same question for the License Configuration. It seems a little different since the name is generated. It may be a good place the use of a selector similar to the subnetSelector and securityGroupSelector.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, it does feel a bit odd writing selectors for things that aren't collections (in the Kubernetes sense).

Sometimes, when what you want is an ARN, and the API is AWS-specific, an ARN is going to be the right fit.

Copy link
Author

Choose a reason for hiding this comment

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

I'll do some research into the AWS APIs but I don't believe these resources have any searchable properties. For example LicenseConfigurations are just ARNs - https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_LicenseConfiguration.html they don't have anything look-up-able.

Copy link
Author

Choose a reason for hiding this comment

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

yeah I think this is the right conclusion. I would agree with your direction if there was some ability to query and discover license configurations, and host resource groups, but the APIs do not have anything I could find. Happy to be corrected if someone can provide the reference docs.

The reason we want ARNs and not just name's like "If you provide the license id 12345, karpenter will rewrite this as an arn", but this won't work in a number of scenarios:

  • the resource is shared between AWS accounts (RAM)
  • can we even find the account ID? what about aws partition for aws-gov environments?
    Providing the full ARN bypasses all of these issues, and I think is the simplest and safest way to provide this feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

Synced in slack, @preflightsiren is kind enough to come to WG this Thursday at 2PM PDT to present this doc. Let's review there and sync up on this decision.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you share the results of your deliberation on this topic? If possible, I would like to help with the delivery of this particular feature. Thank you.

Copy link
Author

Choose a reason for hiding this comment

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

I'm picking this up again this week, just refamiliarising myself and will push the new API up, perhaps as a new PR

@preflightsiren preflightsiren force-pushed the feat-3182-dedicated-hosts branch 2 times, most recently from bdc03fe to c906c2c Compare July 7, 2023 04:38
// +optional
HostResourceGroupARN *string `json:"hostResourceGroupARN,omitempty"`
// +optional
GroupID *string `json:"groupdID,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this to be ID or name? https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_CreatePlacementGroup.html. Seems like name is more discoverable.

Copy link
Author

Choose a reason for hiding this comment

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

I was basing this off of #3324 (comment) I guess we could pull this apart and reimplement with the select model (assuming we can find appropriate aws APIs)

metadata:
name: default
spec:
licenseConfiguration: arn:aws:license-manager:eu-east-1:123456789012:license-configuration:lic-edf7f9e241f5e16f29996c842111f448 # optional, arn of the license configuration
Copy link
Contributor

@jonathan-innis jonathan-innis Jul 14, 2023

Choose a reason for hiding this comment

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

I looked at the APIs very briefly but I see that you can tag both license configurations and resource groups. I think we may be able to achieve discovery here since both resource groups and license configuration have tags and naming.

Granted, we may have to do some client-side filtering for the license configuration since it looks like you can't filter on tags directly from the server-side.

@preflightsiren
Copy link
Author

Created #4553 to track the new approach (selectors) and keep all the review comments from the original approach isolated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-design Design required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Host Resource Groups
7 participants