Skip to content

Conversation

scottshotgg
Copy link
Contributor

@scottshotgg scottshotgg commented Jun 20, 2025

This PR adds the ability to specify tolerations at the agent level similar to annotations, labels, node selector, etc. I know this ability is available from within a pipeline but as an administrator I should be able to set this at a global level as well as prevent any pipeline from overriding this.
In similar vein to how annotations are done, I have introduced two new env variables and updated the documentation to match:

  • WOODPECKER_BACKEND_K8S_POD_TOLERATIONS
  • WOODPECKER_BACKEND_K8S_POD_TOLERATIONS_ALLOW_FROM_STEP

I initially started this feature because I have a "runner" node in my k8s cluster specifically for jobs like this (crons, ci/cd, etc) so that the core compute on the worker nodes isn't effected by random variadic scaling patterns. I felt that the ability to define these tolerations and node selectors at the individual step level was really nice and could come in handy, but the inability to define them at a global level was necessary; I needed a simple way to say that every pipeline should be scheduled to a particular node, which WOODPECKER_BACKEND_K8S_POD_NODE_SELECTOR was good for - however it is a bit underwhelming because you are then left with a pod that has been told to schedule onto a node for which it has no toleration for the nodes taint unless you include this in every pipeline, every step, etc.

I think this could be a useful feature that others could benefit from. Overall I found it simple to implement - thanks for the easy code to work in, and thanks for Woodpecker!

@qwerty287 qwerty287 added enhancement improve existing features backend/kubernetes labels Jun 21, 2025
@qwerty287
Copy link
Contributor

Hello and sorry that it took a long for us to answer.
This looks good with my very limited k8s knowledge, but you added the docs also for the old versions. Only the files in docs/docs should get changed.

@scottshotgg
Copy link
Contributor Author

Ahh okay, so don't change the versioned docs - that makes sense. I'll fix that up

@pat-s
Copy link
Contributor

pat-s commented Jun 29, 2025

With proper rules applied, this might be a solution to #3070.

@scottshotgg
Copy link
Contributor Author

scottshotgg commented Jul 2, 2025

@pat-s
I didn't thoroughly read through your issue, but if your goal is to have a builder for arm based images hosted on an arm node, and a corollary one on amd - it could definitely be a solution for that.

If you need/want an example or two I can try to help you out.

@scottshotgg
Copy link
Contributor Author

@qwerty287
I've restored the docs/versioned_docs folder back to main. I would really like to merge this in soon if that's possible. Right now I am running this branch but would like to run the official image, etc

@qwerty287
Copy link
Contributor

This looks good to me, but could you add the backend options parameter to the schema? https://github.com/woodpecker-ci/woodpecker/blob/main/pipeline/frontend/yaml/linter/schema/schema.json

And I'd like to have a review from someone else, I don't have much knowledge of k8s. @woodpecker-ci/maintainers

@scottshotgg
Copy link
Contributor Author

@qwerty287
Done!

@pat-s
Copy link
Contributor

pat-s commented Jul 3, 2025

I didn't thoroughly read through your issue, but if your goal is to have a builder for arm based images hosted on an arm node, and a corollary one on amd - it could definitely be a solution for that.

No, that's not the scope of the linked issue at all.

Copy link
Contributor

@pat-s pat-s left a comment

Choose a reason for hiding this comment

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

As this is a quite dynamic change, especially to the option to deny/allow override on the user-level, this needs tests. @scottshotgg can you add some that test (ideally all) behaviors of this PR?

@scottshotgg
Copy link
Contributor Author

@pat-s
Do you have any tests that you could point me to? I'd preferably like to replicate the tests for annotations since this feature is 99% the same as that one with the exception of the default value

@qwerty287
Copy link
Contributor

I guess this test is the one you're looking for:

func TestFullPod(t *testing.T) {

Copy link
Member

@xoxys xoxys left a comment

Choose a reason for hiding this comment

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

LGTM after tests were added.

@qwerty287
Copy link
Contributor

@scottshotgg do you plan to add the tests?

@scottshotgg
Copy link
Contributor Author

I will try to work on the tests at some point

@marcusramberg
Copy link
Contributor

I'm interested in getting this merged. Here's a patch to add tests for tolerations (as well as fixing a failing existing test)

https://gist.github.com/marcusramberg/c18a22a191efba8810151c1d2d7fbb34

@scottshotgg
Copy link
Contributor Author

Thank you @marcusramberg for implementing those tests - I've been pretty busy as of late.

@xoxys @qwerty287
Are we g2g now?

Copy link
Member

@xoxys xoxys left a comment

Choose a reason for hiding this comment

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

Thank you both.

@xoxys xoxys enabled auto-merge (squash) August 7, 2025 19:52
@woodpecker-bot
Copy link
Contributor

woodpecker-bot commented Aug 7, 2025

Surge PR preview deployment was removed

@xoxys
Copy link
Member

xoxys commented Aug 7, 2025

@scottshotgg @marcusramberg Tests failed, can you take a look?

@xoxys xoxys merged commit d749535 into woodpecker-ci:main Aug 10, 2025
6 of 7 checks passed
@woodpecker-bot woodpecker-bot mentioned this pull request Aug 10, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend/kubernetes enhancement improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants