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: allow jobs and sts to have the same tolerations and nodeselectors #306

Merged
merged 2 commits into from
Feb 3, 2023

Conversation

alejandroEsc
Copy link
Contributor

@alejandroEsc alejandroEsc commented Jan 31, 2023

Tested this locally:

  • Creates tolerations and node-selectors for all pods/jobs from a global tolerations and nodeSelectors field.
  • Allow for sts to override these values if so needed

Fixes #305

Copy link
Member

@vuldin vuldin left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for getting this PR up so quickly! Also thanks for creating _statefulset-helpers.tpl, I think this approach to organizing named templates will help over time.

I've tested this change out and it works as intended. My only feedback is that the new named templates are an either/or approach when maybe they could allow an aggregate for both nodeSelector and tolerations. In other words. If values are found in both statefulset and the top-level parameter then both are used. This isn't a blocker though, we can talk about it and improve the implementation over time if we agree.

@joejulian
Copy link
Contributor

My only feedback is that the new named templates are an either/or approach when maybe they could allow an aggregate for both nodeSelector and tolerations. In other words. If values are found in both statefulset and the top-level parameter then both are used. This isn't a blocker though, we can talk about it and improve the implementation over time if we agree.

This would break if the user wanted only the sts to be on one set of nodes, and all the ancillary work to run on another, ie. Jobs run on mylabel: jobs and redpanda runs on mylabel: redpanda. Merging them would create an un-matchable combination.

@joejulian
Copy link
Contributor

I'm working on a PR to ensure that the jobs and statefulset pods use distinct labels so as not to have jobs match the statefulset selector. That PR needs merged first.

@RafalKorepta
Copy link
Contributor

@alejandroEsc Can you add test that uses nodeSelector and tolerations? You could annotate kind workers to achieve that.

@alejandroEsc
Copy link
Contributor Author

tolerations

I know i can use the labels on kind workers. What do you mean by "use annotations"?

@RafalKorepta
Copy link
Contributor

I wrongly write annotate. I should write label them

@alejandroEsc alejandroEsc force-pushed the ae/issues/305 branch 2 times, most recently from 610df7d to 1037201 Compare February 3, 2023 21:38
@joejulian joejulian removed the blocked label Feb 3, 2023
@joejulian joejulian merged commit 27b3a6a into main Feb 3, 2023
@joejulian joejulian deleted the ae/issues/305 branch February 3, 2023 23:53
RafalKorepta pushed a commit to redpanda-data/redpanda-operator that referenced this pull request Dec 3, 2024
…e/issues/305

fix: allow jobs and sts to have the same tolerations and nodeselectors
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.

Jobs don't set tolerations or nodeSelector (if set)
4 participants