-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add NTH Queue Processor Mode #10995
Add NTH Queue Processor Mode #10995
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Welcome @haugenj! |
Hi @haugenj. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Thanks for this PR. Looks like it is going in the right direction
If the templates are radically different and easiest to maintain separatly, you can switch the location here:
I suspect so, yes. I am not sure what is configurable here.
Not quite sure what you mean here. The configuration for NTH is in the same place in master as in the 1.19 branch: |
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.
Definitely on the right path here. Thanks for this and keep up the good work!
--- | ||
# Source: aws-node-termination-handler/templates/psp.yaml | ||
apiVersion: policy/v1beta1 | ||
kind: PodSecurityPolicy |
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.
Technically the PSP stuff isn't required. kube-system has a default allow-anything policy if the admission controller is enabled. But fine to leave it in if it makes copy/pasting from source easier.
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'm leaning towards leaving it in. IMO it'd be simpler to keep it exactly in line with the default config released by NTH
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 ended up removing it and merging the two templates, with an if/else to switch between the IMDS daemonset and the Queue Processor deployment, the rest of the templates were the same
Not sure how I didn't see that tbh 😅. I'll move the changes to the master branch on the next revision |
@olemarkus can you give me some pointers for what kind of tests I should add to this? |
Typically validation tests: https://kops.sigs.k8s.io/contributing/adding_a_feature/#tests In addition, it would be nice with an integration test that pretty much does a full deployment and ends up in "golden" cloudformation/terraform files. See https://kops.sigs.k8s.io/contributing/testing/#adding-an-integration-test. Ping me if you need any help |
e665193
to
afaa68d
Compare
Except for the comments made, I think this one is good to go. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rifelpet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks for sticking with this @haugenj ! I think a lot of people will be eager to use this functionality |
Issue:
#7119
Adds the queue-processor mode to the Node Termination Handler Add on and provisions the requisite resources (SQS Queue, Eventbridge rules/targets, ASG Lifecycle Hooks, IAM policy updates).
Opening this up early to see if there are any big gaps in what I've done
Todo:
Questions:
{{ with .NodeTerminationHandler.EnableSqsTerminationDraining }}
at the top of the file? I'm not familiar with this logic... is it bootstrap?release-1.19
instead of master because I didn't see the existing NTH config in master. Is this alright?