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

WIP Upgrade Karpenter to 1.0.0 #16758

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

rifelpet
Copy link
Member

@rifelpet rifelpet commented Aug 18, 2024

TODO list:

  • Set the EC2NodeClass userdata - this is a bit tricky because userdata is normally generated via fi.CloudupTask dependencies but none of the bootstrapchannelbuilder code uses fi.Resource
  • Update awsmodel to not add awstasks.LaunchTemplates for Karpenter instance groups
  • Review rolling-update, delete cluster, and toolbox dump code to confirm it works without kops-managed launch templates
  • Add release note and update documentation
  • Figure out why the --kubeconfig flag is being removed from the CLI options
  • Reuse the root volume defaulting logic from pkg/awsmodel

Note that this doesn't follow the required karpenter upgrade procedure - therefore existing kops clusters with karpenter enabled may not have an upgrade path to this version of karpenter. We'll need to figure out a low-effort workaround to include in the upgrade instructions.

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 18, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from rifelpet. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

- id: ami-12345678
blockDeviceMappings:
- deviceName: /dev/xvda
ebs: {}
Copy link
Member Author

@rifelpet rifelpet Aug 18, 2024

Choose a reason for hiding this comment

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

looks like reuse the awsmodel's defaulting logic for root volumes:

rootVolumeSize, err := defaults.DefaultInstanceGroupVolumeSize(ig.Spec.Role)
if err != nil {
return nil, err
}
var rootVolumeType ec2types.VolumeType
rootVolumeEncryption := DefaultVolumeEncryption
rootVolumeKmsKey := ""
if ig.Spec.RootVolume != nil {
if fi.ValueOf(ig.Spec.RootVolume.Size) > 0 {
rootVolumeSize = fi.ValueOf(ig.Spec.RootVolume.Size)
}
rootVolumeType = ec2types.VolumeType(fi.ValueOf(ig.Spec.RootVolume.Type))
if ig.Spec.RootVolume.Encryption != nil {
rootVolumeEncryption = fi.ValueOf(ig.Spec.RootVolume.Encryption)
}
if fi.ValueOf(ig.Spec.RootVolume.Encryption) && ig.Spec.RootVolume.EncryptionKey != nil {
rootVolumeKmsKey = *ig.Spec.RootVolume.EncryptionKey
}
}
if rootVolumeType == "" {
rootVolumeType = DefaultVolumeType
}

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2024
@justinsb
Copy link
Member

I had a look at the userdata problem. As part of the bare metal WIP, I've refactored the bootstrap script building so it isn't as tricky as it used to be: https://github.com/kubernetes/kops/blob/master/pkg/commands/toolbox_enroll.go#L401

I had a go at piping that through, it isn't too bad except for the wellKnownAddresses. But getting those would indeed be tricky...

But - I was wondering if we should build this in the template anyway, vs uploading the kops InstanceGroups and having kops-controller (for example) expand them. This is what I've been playing with for cluster-api, and it seems to work I think (though I should probably revive and clean that up)

Another advantage of doing it that way is that users could then dynamically create InstanceGroups. The gotcha is that those would not be stored in the state store so would have to be managed differently, but we don't have to support (or encourage) dynamic InstanceGroups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/addons area/api cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants