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

Allow setting tolerations & node affinity rules for Jobs #704

Open
Tracked by #261
jashandeep-sohi opened this issue Apr 19, 2024 · 6 comments
Open
Tracked by #261

Allow setting tolerations & node affinity rules for Jobs #704

jashandeep-sohi opened this issue Apr 19, 2024 · 6 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed improvement

Comments

@jashandeep-sohi
Copy link
Contributor

jashandeep-sohi commented Apr 19, 2024

I need to be able to set tolerations and affinity.nodeAffinity for Pods created by Jobs created by this operator. As far as I know, there's no way to do this right now.

@alexandrevilain
Copy link
Owner

Hi @jashandeep-sohi !

Thanks for reporting this issue.
Are you interested on implementing this ?

@alexandrevilain alexandrevilain added help wanted Extra attention is needed good first issue Good for newcomers improvement labels Apr 22, 2024
@jashandeep-sohi
Copy link
Contributor Author

Yeah, I could give it a go. What's the best way to do this?

Add a generic temporalcluster.spec.jobOverrides? There's already some Job related attributes at that level, so those would probably have to merged/deprecated.

The other option is to just keep it simple for now and add the things I need, i.e. temporalcluster.spec.jobTolerations and temporalcluster.spec.jobAffinity

@alexandrevilain
Copy link
Owner

I think that maybe we have to do some cleaning on the API.

For now the api provides the following fields:

  • spec.jobTtlSecondsAfterFinished
  • spec.jobResources
  • spec.jobInitContainers

Adding more and more fields on the object's root is becoming a non-sense.

I would go for it in some extra steps:

  • Create a new spec.persistence.jobs, which is more explicit that we're configuring persistence jobs.
  • Deprecate spec.jobTtlSecondsAfterFinished, spec.jobResources and spec.jobInitContainers to become spec.persistence.jobs.TTLSecondsAfterFinished, spec.persistence.jobs.resources, spec.persistence.jobs.initContainers
  • Add the missing field spec.persistence.jobs.overrides.

Then when moving to v1 remove the spec.jobTtlSecondsAfterFinished, spec.jobResources and spec.jobInitContainers.

WDYT ?

@jashandeep-sohi
Copy link
Contributor Author

Deprecate spec.jobTtlSecondsAfterFinished, spec.jobResources and spec.jobInitContainers to become spec.persistence.jobs.TTLSecondsAfterFinished, spec.persistence.jobs.resources, spec.persistence.jobs.initContainers

Sounds good, but can you help me understand the reason behind the extra indirection for existing Job overrides if they're going to be removed in v1 anyways? Isn't it enough to just add spec.persistence.jobs.overrides?

@alexandrevilain
Copy link
Owner

Yes @jashandeep-sohi sorry if my explanations weren't clear enought.

Let's do it in 3 steps:

  • You can create the PR to add: spec.persistence.jobs.overrides.
  • Then you or me can create a PR to deprecate existing spec.job** fields to spec.persistence.jobs.**
  • Then in a next PR we can remove spec.job**.

Is this clearer ? :)

@jashandeep-sohi
Copy link
Contributor Author

Ok, yeah that clarifies things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed improvement
Projects
None yet
Development

No branches or pull requests

2 participants