Add life-cycle labels to nodes #2615
Conversation
|
kind/improvement |
| @@ -27,6 +27,13 @@ AWS_SERVICES_DOMAIN="$(get_metadata services/domain)" | |||
|
|
|||
| source /etc/eksctl/kubelet.env # this can override MAX_PODS | |||
|
|
|||
| INSTANCE_LIFECYCLE=$(aws ec2 describe-instances --instance-ids ${INSTANCE_ID} --query 'Reservations[0].Instances[0].InstanceLifecycle' --output text) | |||
There was a problem hiding this comment.
We are trying to avoid adding extra dependencies to the bootstrap scripts. Please use the get_metadata function with instance-life-cycle instead of the aws cli.
There was a problem hiding this comment.
Hi @vaibhav-random , thank you for this PR, it's a good idea. The implementation needs a small change.
Also, could you add the same to the ubuntu script, please? We are trying to keep feature parity between them.
|
@martina-if Thanks for the review, made the changes. Can you please check now |
| @@ -27,6 +27,14 @@ AWS_SERVICES_DOMAIN="$(get_metadata services/domain)" | |||
|
|
|||
| source /etc/eksctl/kubelet.env # this can override MAX_PODS | |||
|
|
|||
| INSTANCE_LIFECYCLE="$(get_metadata instance-life-cycle | grep 'spot')" | |||
There was a problem hiding this comment.
I am curious, why the grep?
I think this would be simpler if we:
- assigned
"$(get_metadata instance-life-cycle)"to INSTANCE_LIFECYCLE - added its value directly to the
NODE_LABELSif it is not empty
So I would remove the grep and only check that it's not empty before adding it to the labels. Although I doubt that it will produce an empty string 🤔
There was a problem hiding this comment.
the endpoint instance-life-cycle is available only on spot instances. For on demand instances the endpoint doesn't exist - will return a 404
There was a problem hiding this comment.
The endpoint does exist for all instances :) And it returns on-demand:
$ TOKEN=`curl -X PUT "http://169.254.169.254/latest/api/token" -H "X-aws-ec2-metadata-token-ttl-seconds: 21600"` \
> && curl -H "X-aws-ec2-metadata-token: $TOKEN" -v http://169.254.169.254/latest/meta-data/instance-life-cycle
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 56 100 56 0 0 56000 0 --:--:-- --:--:-- --:--:-- 56000
* Trying 169.254.169.254...
* TCP_NODELAY set
* Connected to 169.254.169.254 (169.254.169.254) port 80 (#0)
> GET /latest/meta-data/instance-life-cycle HTTP/1.1
> Host: 169.254.169.254
> User-Agent: curl/7.61.1
> Accept: */*
> X-aws-ec2-metadata-token: AQAEAITQryp2rkEswx7rHzA4nAXSk3DsOaKVl2YNWfhAlNUejvsK0w==
>
< HTTP/1.1 200 OK
< X-Aws-Ec2-Metadata-Token-Ttl-Seconds: 21600
< Content-Type: text/plain
< Accept-Ranges: none
< Last-Modified: Mon, 07 Sep 2020 05:28:37 GMT
< Content-Length: 9
< Date: Mon, 07 Sep 2020 06:02:05 GMT
< Server: EC2ws
< Connection: close
<
* Closing connection 0
on-demand
There was a problem hiding this comment.
You're right. This was added to on demand instances also beginning IMDSv2. Made the changes
|
Hi @vaibhav-random thanks for the changes, I added another comment. Can you also tell us if you manually tested the changes for both AL2 and Ubuntu? |
|
@martina-if Hey! yeah, I've manually tested the changes both the places |
|
@martina-if did the changes, please check now |
martina-if
changed the title
@vaibhav-random I tried it but I didn't get the label set and I think this PR missed a step to get it working. Can you explain how you tested it? I can't seem to get it working... |
|
@martina-if oops! my bad, I tested it locally by overriding the bootstrap script, but did not check in a piece. I corrected it here - #2623 Can you please review this |
closes #2614
Description
Added a default label to indicate if a node's life cycle is on demand or spot
Checklist
README.md, or theuserdocsdirectory)area/nodegroup), target version (e.g.version/0.12.0) and kind (e.g.kind/improvement)