-
Notifications
You must be signed in to change notification settings - Fork 904
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: deprecated AMIs should be discoverable when specifying ami id o… #6500
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for karpenter-docs-prod canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'll need to edit the sorting logic here too
76282c4
to
cbb40af
Compare
pkg/providers/amifamily/ami.go
Outdated
sort.Slice(a, func(i, j int) bool { | ||
ideptime := lo.TernaryF( | ||
a[i].DeprecationTime != "", | ||
func() time.Time { t, _ := time.Parse(time.RFC3339, a[i].DeprecationTime); return t }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fail loudly if this invariant is violated
func() time.Time { t, _ := time.Parse(time.RFC3339, a[i].DeprecationTime); return t }, | |
func() time.Time { t := lo.Must(time.Parse(time.RFC3339, a[i].DeprecationTime)); return t }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Updated :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellistarn how is this different from the existing, where errors are ignored? Should that as well become lo.Must
?
karpenter-provider-aws/pkg/providers/amifamily/ami.go
Lines 89 to 90 in 8bc2030
itime, _ := time.Parse(time.RFC3339, a[i].CreationDate) | |
jtime, _ := time.Parse(time.RFC3339, a[j].CreationDate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question - this is the same for the existing piece for CreationDate
where we have the following lines - so i used the same logic for DeprecationTime
itime, _ := time.Parse(time.RFC3339, a[i].CreationDate)
jtime, _ := time.Parse(time.RFC3339, a[j].CreationDate)
if itime.Unix() != jtime.Unix() {
return itime.Unix() > jtime.Unix()
}
return a[i].AmiID < a[j].AmiID
cbb40af
to
be11695
Compare
Pull Request Test Coverage Report for Build 10087006300Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
pkg/providers/amifamily/ami.go
Outdated
itime, _ := time.Parse(time.RFC3339, a[i].CreationDate) | ||
jtime, _ := time.Parse(time.RFC3339, a[j].CreationDate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that we want to do a Must here, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a lot of unrelated tests with the changes in PR fail when using Must here
Summarizing 6 Failures:
[PANICKED!] NodeClass InstanceProfile Status Controller [It] should resolve the specified instance profile into the status when using instanceProfile field
/Users/shabbirkagalwala/go/pkg/mod/github.com/samber/lo@v1.44.0/errors.go:53
[PANICKED!] NodeClass InstanceProfile Status Controller [It] should not call the the IAM API when specifying an instance profile
/Users/shabbirkagalwala/go/pkg/mod/github.com/samber/lo@v1.44.0/errors.go:53
[PANICKED!] NodeClass InstanceProfile Status Controller [It] should not call CreateInstanceProfile or AddRoleToInstanceProfile when instance profile exists with correct role
/Users/shabbirkagalwala/go/pkg/mod/github.com/samber/lo@v1.44.0/errors.go:53
[PANICKED!] NodeClass InstanceProfile Status Controller [It] should update the role for the instance profile when the wrong role exists
/Users/shabbirkagalwala/go/pkg/mod/github.com/samber/lo@v1.44.0/errors.go:53
[PANICKED!] NodeClass InstanceProfile Status Controller [It] should create the instance profile when it doesn't exist
/Users/shabbirkagalwala/go/pkg/mod/github.com/samber/lo@v1.44.0/errors.go:53
[PANICKED!] NodeClass InstanceProfile Status Controller [It] should add the role to the instance profile when it exists without a role
/Users/shabbirkagalwala/go/pkg/mod/github.com/samber/lo@v1.44.0/errors.go:53
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. We can address separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated based on conversation.
c61174d
to
75f54a8
Compare
…n the amiSelectorTerms
75f54a8
to
02056e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
I'd like to get @engedaam's approval on this, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main comments are round observability and drift
MaxResults: aws.Int64(1000), | ||
Filters: lo.Ternary(len(q.Filters) > 0, q.Filters, nil), | ||
Owners: lo.Ternary(len(q.Owners) > 0, lo.ToSlicePtr(q.Owners), nil), | ||
IncludeDeprecated: aws.Bool(true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should notify users when Karpenter is launching instances with deprecated AMIs. IMO we should at least have warning logs. I think we should have a mechanism for users to be able to identify nodes that are deprecated. I'm not sure what that mechanism should be, but so option could be adding an annotation to the nodeclaim/node, eventing, nodeclaims status condition, metrics, or even repeatedly logging all nodes that are using deprecated AMIs. I personally like the idea of having an annotation to signal a Karpenter owned instances that is using a deprecated AMI. @ellistarn any opinions here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am with you that adding an annotation that an ami is deprecated is a good idea on the nodeclaim
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@engedaam Thoughts on a warning status condition on the NodeClass?
Logging/events strike me as spammy, and potentially ignored. Condition will cause a metric, which has nice benefits, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on a warning status condition on the NodeClass?
I like the idea of having the warning status condition on the NodeClass. Customers will need to do the leg work of finding the instances using the deprecated that are owned by the NodeClass themselves, but I don't think that would be a heavy lift for them.
Logging/events strike me as spammy, and potentially ignored. Condition will cause a metric, which has nice benefits, too.
I agree with that
Filters: lo.Ternary(len(q.Filters) > 0, q.Filters, nil), | ||
Owners: lo.Ternary(len(q.Owners) > 0, lo.ToSlicePtr(q.Owners), nil), | ||
IncludeDeprecated: aws.Bool(true), | ||
MaxResults: aws.Int64(1000), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what is the expectation around drift? Currenlty, karpenter will use the resolved list of AMIs to check against the instances that were launched by Karpenter. If the AMI used on a node exist in the resolved list of AMIs the node will not be drifted. With this PR, if an AMI is deprecated and a new AMI is resolved by the same amiSelectorTerms
, the existing nodes would not be drifted however new nodes launched will use the new non-deprecated AMI. What is thinking here? Should we drift nodes that are using deprecated AMIs if there is a new non-deprecated AMI is resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general the idea was when following the pattern as described here that Karpenter should not disrupt workloads when an AMI is deprecated and amiSelectorTerms
contain only an id
. Karpenter does a real amazing job of detecting newer amis when launching nodes when amiSelectorTerms
contains names or tags which support wildcards.
Should we drift nodes that are using deprecated AMIs if there is a new non-deprecated AMI is resolved?
In my opinion yes if a new non-deprecated AMI is available nodes should be drifted. I think that would warrant a change here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC our previous conversations decided:
- We will migrate off of deprecated AMIs if we have multiple in the selector
- We will keep using deprecated AMIs if there is no non-deprecated alternative
e.g., we will only use a deprecated AMI if there is no other option, but we won't take an outage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, Karpenter will scale-up nodes based on the sorted AMIs, but not for drift. We only check if the AMI used on the instance is within the resolved list of AMIs for the nodeclass. We will need to update our drift detection to find nodes that are using deprecated AMIs that can be updated to drifted to use non-deprecated AMIs.
Karpenter does a real amazing job of detecting newer amis when launching nodes when amiSelectorTerms contains names or tags which support wildcards.
- We will migrate off of deprecated AMIs if we have multiple in the selector
In the current state of the PR, we will need to make updates here or we break that behavior all together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this decision might require broader review from the other maintainers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellistarn @engedaam my apologies for the delayed response but I want to share my findings and make sure I am not misunderstanding the requirement and discussion here.
IncludeDeprecated
only takes effect when the AMI owner is not self (the current account) for example when a central AWS account is responsible for building and sharing AMIs to multiple accounts in the organization. By default deprecated AMIs in the same AWS account i.e the owner is self are visible always.- I ran a few tests on my sandbox EKS cluster and the drift works as expected as long as a new non deprecated AMI is available and the
amiSelectorTerms
don't have the AMI id pinned. As an example here's what i tested with (as an example with nodepool of type of only 1instance-family
andarch
)
I ran the same tests below with using tags and name on the amiSelectorTerms
spec:
amiFamily: AL2
amiSelectorTerms:
- name: dummy-amazon-eks-node-1.29-*
owner: "111111111111"
The above is in AWS account 999999999999
and the AMI is being shared from AWS account 111111111111
- Next due to the selector terms we see that all nodes from this ec2nc are using the AMI
status:
amis:
- id: ami-053ccf7a5d1c364ac
name: dummy-amazon-eks-node-1.29-test-20240625
requirements:
- key: kubernetes.io/arch
operator: In
values:
- amd64
and the node claim
conditions:
- lastTransitionTime: "2024-07-25T04:18:14Z"
message: ""
reason: Initialized
status: "True"
type: Initialized
- lastTransitionTime: "2024-07-25T04:17:12Z"
message: ""
reason: Launched
status: "True"
type: Launched
- lastTransitionTime: "2024-07-25T04:18:14Z"
message: ""
reason: Ready
status: "True"
type: Ready
- lastTransitionTime: "2024-07-25T04:18:06Z"
message: ""
reason: Registered
status: "True"
type: Registered
imageID: ami-053ccf7a5d1c364ac
nodeName: ip-172-xx-xxx-xx.us-west-2.compute.internal
providerID: aws:///us-west-2b/i-0xxxxxxxxxxxxxx82
- I then deprecated this AMI as visible below and scaled up pods in an namespace which scaled up and added new nodeclaims and worked as expected - otherwise without the changes in this PR Karpenter would fail to scale up new nodes
✗ aws ec2 describe-images --region us-west-2 --image-ids ami-053ccf7a5d1c364ac
{
"Images": [
{
"Architecture": "x86_64",
"CreationDate": "2024-07-25T04:07:58.000Z",
"ImageId": "ami-053ccf7a5d1c364ac",
"ImageLocation": "111111111111/dummy-amazon-eks-node-1.29-test-20240625",
"ImageType": "machine",
"Public": false,
"OwnerId": "111111111111",
"PlatformDetails": "Linux/UNIX",
"UsageOperation": "RunInstances",
"State": "available",
...
...
...
...
"VirtualizationType": "hvm",
"DeprecationTime": "2024-07-25T22:30:00.000Z",
"ImdsSupport": "v2.0"
}
]
}
- Then, I created a new AMI in AWS account
111111111111
and shared it with account999999999999
. Since the creationDate of this new AMI was more recent - I see the drift working as expected.
Here's the nodeclaims from the new AMI
conditions:
- lastTransitionTime: "2024-07-29T21:49:52Z"
message: ""
reason: Initialized
status: "True"
type: Initialized
- lastTransitionTime: "2024-07-29T21:48:58Z"
message: ""
reason: Launched
status: "True"
type: Launched
- lastTransitionTime: "2024-07-29T21:49:52Z"
message: ""
reason: Ready
status: "True"
type: Ready
- lastTransitionTime: "2024-07-29T21:49:44Z"
message: ""
reason: Registered
status: "True"
type: Registered
imageID: ami-0ab3fd8c11a5baed9
nodeName: ip-172-xx-xxx-xxx.us-west-2.compute.internal
providerID: aws:///us-west-2c/i-0xxxxxxxxxxxxxxdc
and the ec2nc
status:
amis:
- id: ami-0ab3fd8c11a5baed9
name: dummy-amazon-eks-node-1.29-test-20240703
requirements:
- key: kubernetes.io/arch
operator: In
values:
- amd64
and the nodeclaims (notice the drift working where old nodes 4d17h
of age were drifted and new nodes with the new AMI were spun up)
→ [04:50:17] (git:main *=) ✗ k get nodeclaim
NAME TYPE ZONE NODE READY AGE
cosmic-lamb-1-5kxj7 m5.xlarge us-west-2c ip-172-xx-xxx-xxx.us-west-2.compute.internal True 3m27s
cosmic-lamb-1-cqjcm m5.xlarge us-west-2a ip-172-xx-xxx-xxx.us-west-2.compute.internal True 4d17h
cosmic-lamb-1-fccr4 m5.xlarge us-west-2b ip-172-xx-xxx-xxx.us-west-2.compute.internal True 4m39s
→ [04:52:23] (git:main *=) ✗ k get nodeclaim
NAME TYPE ZONE NODE READY AGE
cosmic-lamb-1-5kxj7 m5.xlarge us-west-2c ip-172-xx-xxx-xxx.us-west-2.compute.internal True 3m48s
cosmic-lamb-1-fccr4 m5.xlarge us-west-2b ip-172-xx-xxx-xxx.us-west-2.compute.internal True 5m
and the new AMI
✗ aws ec2 describe-images --region us-west-2 --image-ids ami-0ab3fd8c11a5baed9
{
"Images": [
{
"Architecture": "x86_64",
"CreationDate": "2024-07-29T21:16:39.000Z",
"ImageId": "ami-0ab3fd8c11a5baed9",
"ImageLocation": "111111111111/dummy-amazon-eks-node-1.29-test-20240703",
"ImageType": "machine",
"Public": false,
"OwnerId": "111111111111",
"PlatformDetails": "Linux/UNIX",
"UsageOperation": "RunInstances",
"State": "available",
...
...
...
...
"EnaSupport": true,
"Hypervisor": "xen",
"Name": "dummy-amazon-eks-node-1.29-test-20240703",
"RootDeviceName": "/dev/xvda",
"RootDeviceType": "ebs",
"SriovNetSupport": "simple",
"VirtualizationType": "hvm",
"ImdsSupport": "v2.0"
}
]
}
The only scenario where drift doesn't work is when an AMI is deprecated and no new AMIs are available, Karpenter will not fall back to a non deprecated AMI with an older creationDate from the deprecated AMI.
Do we need to fall back to an older non deprecated AMI if there are no new non deprecated AMIs available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/disable-an-ami.html
When an AMI is disabled - there's a separate flag for that, to make it discoverable IncludeDisabled
Additionally
A disabled AMI can't be shared. If an AMI was public or previously shared, it is made private. If an AMI was shared with an AWS account, organization, or Organizational Unit, they lose access to the disabled AMI.
and
EC2 instances that were previously launched using an AMI that is subsequently disabled are not affected, and can be stopped, started, and rebooted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellistarn @engedaam @jonathan-innis awesome 🥳 karpenter v1 is released, i am excited to try it out soon. I just wanted to check with you all if we can reinitiate the conversation for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@engedaam @jonathan-innis apologies for the tag but just wanted to bump this PR again and see if we can move forward.
…n the amiSelectorTerms
Fixes #6393
Description
Deprecated AMI's should be discovered by Karpenter for existing node pool to avoid disruptions to workloads in production systems.
How was this change tested?
make verify
- to update the CRD definitions and apply to a development EKS clusterKO_DOCKER_REPO=4xxxxxxxxxx5.dkr.ecr.us-west-2.amazonaws.com/shabbskagalwala/karpenter-provider-aws/controller make image
to build and push the image, which was consumed in EKS.ec2nodeclass
to use a deprecated amiami-002c17c43a2d082b5
which resulted in Karpenter successfully creating a nodeclaim with no errors as seen belownodeclaim using deprecated ami
deprecated ami as seen from aws cli
Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.