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

Fix custom nodeAffinity via values.yaml #515

Merged
merged 4 commits into from
Nov 19, 2021

Conversation

ngoyal16
Copy link
Contributor

@ngoyal16 ngoyal16 commented Oct 20, 2021

Issue #495

Description of changes:

  1. Removed arch affinity from nodeAffinity completely
  2. Removed OS node selector from nodeAffinity. As it already exists in nodeSelector

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

@bwagner5 bwagner5 self-requested a review October 23, 2021 01:48
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.

We need to include the OS in the node selector for the daemonset definitions.

The Fargate node affinity can be moved into the values.yaml.

Thanks for taking on this task! :)

@ngoyal16
Copy link
Contributor Author

ngoyal16 commented Oct 23, 2021

We need to include the OS in the node selector for the daemonset definitions.

Already there

The Fargate node affinity can be moved into the values.yaml.

can't we add following key pair in node selector for deamonset

eks.amazonaws.com/compute-type : ec2

As we need it on EC2 servers only

@ngoyal16 ngoyal16 requested a review from bwagner5 October 23, 2021 02:40
@bwagner5
Copy link
Contributor

We need to include the OS in the node selector for the daemonset definitions.

Already there

The Fargate node affinity can be moved into the values.yaml.

can't we add following key pair in node selector for deamonset

eks.amazonaws.com/compute-type : ec2

As we need it on EC2 servers only

Ah sorry, I did not see the OS and Arch in the node selector, but I see it now :)

The potential problem with doing an eks.amazonaws.com/compute-type node selector is when operating in an environment that does not set that label. I don't think Kops will set that label.

@ngoyal16
Copy link
Contributor Author

ngoyal16 commented Nov 5, 2021

@bwagner5 as per the issue, peoples are facing issue with deployment type only not with the dameonsets one. so i guess we can go with this fix.

@bwagner5
Copy link
Contributor

@ngoyal16 can you move the fargate anti-affinity to the values.yaml file?

@ngoyal16 ngoyal16 requested a review from a team as a code owner November 18, 2021 01:21
@ngoyal16 ngoyal16 requested a review from snay2 November 18, 2021 01:23
@ngoyal16
Copy link
Contributor Author

\rerun

Copy link
Contributor

@snay2 snay2 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

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.

/lgtm

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

Successfully merging this pull request may close these issues.

3 participants