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

feat(#5354): Allow adding toleration into builder pod #5667

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

vkrejcirik
Copy link
Contributor

Closes: #5354

Release Note

feat: Allow defining tolerations for Builder pod.

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

Thanks for the work. I think it would be better to reuse the same approach we've used for Integrations' tolerations. See https://github.com/apache/camel-k/blob/main/pkg/apis/camel/v1/trait/toleration.go - you may reuse entirely the same logic. Ideally we should provide the concept of taint, and have a way to express it in the same syntax so it is easier from CLI to set via -t builder.taint=... (which should also the way we unit test it).

@squakez
Copy link
Contributor

squakez commented Jun 25, 2024

BTW, also better to squash the commits into a single one in order to have a cleaner history.

@squakez
Copy link
Contributor

squakez commented Jun 25, 2024

Maybe some alternative approach would be to work directly on the Toleration trait, having an additional option like -t toleration.pipeline-taint=... and have all the logic encapsulated in a single place. In the same way we may rework the affinity etc.

@vkrejcirik
Copy link
Contributor Author

Thanks for the work. I think it would be better to reuse the same approach we've used for Integrations' tolerations. See https://github.com/apache/camel-k/blob/main/pkg/apis/camel/v1/trait/toleration.go - you may reuse entirely the same logic. Ideally we should provide the concept of taint, and have a way to express it in the same syntax so it is easier from CLI to set via -t builder.taint=... (which should also the way we unit test it).

I was essentially following the approach from #4968 as we discussed. I like the approach of extending Toleration trait with new option for Builder pod and having the logic in a single place. So, if we are on the same page and that's what you prefer, I can look into adding this functionality there.

Thanks

@squakez
Copy link
Contributor

squakez commented Jun 25, 2024

I was essentially following the approach from #4968 as we discussed. I like the approach of extending Toleration trait with new option for Builder pod and having the logic in a single place. So, if we are on the same page and that's what you prefer, I can look into adding this functionality there.

Thanks

Yeah, I would have preferred that. However, I think that now we've already taken the other path, ie, the nodeselector is already on the builder trait, so, it would feel weird to require another trait for toleration. Let's keep this here for now, but try to reuse the logic already available in the other trait (ie, transforming the taints).

@vkrejcirik
Copy link
Contributor Author

Okay, I will look into it.

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.

Ability to define K8s toleration for Builder pod
2 participants