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

Expose pod cfg via yaml to UI and merge tolerations #311

Merged
merged 2 commits into from Apr 23, 2018

Conversation

@gabemontero
Copy link
Contributor

@gabemontero gabemontero commented Apr 16, 2018

…lerations

Per discussion in #275 (comment) and #275 (comment)

And for https://trello.com/c/XBOXKjYA/1446-3-add-pod-tolerations-to-slave-pods-jenkinsintegration

@carlossg @bparees fyi / ptal

@gabemontero
Copy link
Contributor Author

@gabemontero gabemontero commented Apr 16, 2018

Loading

@bparees
Copy link
Contributor

@bparees bparees commented Apr 16, 2018

lgtm.

Loading

@carlossg carlossg changed the title expose pod cfg via yaml to jenkins config panel; enable setting of to… Expose pod cfg via yaml to UI and merge tolerations Apr 17, 2018
The raw yaml of a Pod API Object. Any pod fields set with non-empty values via the configuration fields above will take precedence
as they are merged with this yaml.

Fragments of a full Pod API Object yaml representation are allowed, but the yaml fragment must start with:
Copy link
Contributor

@carlossg carlossg Apr 17, 2018

Choose a reason for hiding this comment

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

must it ?

Loading

Copy link
Contributor Author

@gabemontero gabemontero Apr 18, 2018

Choose a reason for hiding this comment

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

Nope, you are right. Just need enough context ... i.e.

spec:
   tolerations:
      - key: CriticalAddonsOnly
        operator: Exists

works, but

   tolerations:
      - key: CriticalAddonsOnly
        operator: Exists

Does not since the code puts in a empty spec if one is not specified. I didn't try the middle ground before.

I'll update the help for this as well.

thanks

Loading

// Tolerations
List<Toleration> combinedTolerations = Lists.newLinkedList();
Optional.ofNullable(parent.getSpec().getTolerations()).ifPresent(combinedTolerations::addAll);
Optional.ofNullable(template.getSpec().getTolerations()).ifPresent(combinedTolerations::addAll);
Copy link
Contributor

@carlossg carlossg Apr 17, 2018

Choose a reason for hiding this comment

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

This would never replace parent tolerations, always append. Is that the expected behavior ?
IIUC you can have multiple tolerations with the same key, but just checking

Loading

Copy link
Contributor Author

@gabemontero gabemontero Apr 18, 2018

Choose a reason for hiding this comment

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

Yeah, per https://kubernetes.io/docs/concepts/configuration/taint-and-toleration/ you can have the same key with different contents ...several examples on that page that do that.

On whether to override or add ... for our uses cases at least, we can make some simplifying assumptions wrt the yaml field being the starting point, and most likely there won't be existing tolerations that we have to worry about overriding.

So I'm good with keeping it simple and not adding more levers around tuning the behaviour. But if you want those let me know.

Loading

Copy link
Contributor Author

@gabemontero gabemontero Apr 18, 2018

Choose a reason for hiding this comment

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

Though upon re-reading the help text, it could be misleading wrt that ... I'll update it to clarify.

Loading

@gabemontero
Copy link
Contributor Author

@gabemontero gabemontero commented Apr 18, 2018

updates pushed in separate commit ... can squash on your say-so @carlossg

Loading

@gabemontero
Copy link
Contributor Author

@gabemontero gabemontero commented Apr 23, 2018

bump @carlossg

Loading

@carlossg carlossg merged commit e1aae89 into jenkinsci:master Apr 23, 2018
2 checks passed
Loading
@gabemontero gabemontero deleted the pod-tolerations branch Apr 23, 2018
@gabemontero
Copy link
Contributor Author

@gabemontero gabemontero commented Apr 23, 2018

thanks @carlossg !

If there is a schedule for new releases of this plugin, apologies, I could not find it poking around the wiki, etc.

So I'll ask here: any sense on when a new version of this plugin, with this commit, will be cut?

thanks again

Loading

@carlossg
Copy link
Contributor

@carlossg carlossg commented Apr 24, 2018

there is no schedule but I try to release on every new commit

Loading

@nick4fake
Copy link

@nick4fake nick4fake commented May 3, 2018

image
image
image

It has somehow broken pod count limit.

Loading

@nick4fake
Copy link

@nick4fake nick4fake commented May 3, 2018

Sorry, just checked this PR. My problem is not related to it, but there seems to be a bug with yaml merging.

Loading

@nick4fake
Copy link

@nick4fake nick4fake commented May 3, 2018

Okay, so it looks like we need to manually add label to pod yaml:

apiVersion: v1
kind: Pod
metadata:
  labels:
    jenkins/kube-default: true
    app: jenkins
    component: agent
spec:
  nodeSelector:
    werkint.com/entity: other
  tolerations:
  - key: werkint.com/entity
    operator: Equal
    value: other
    effect: NoSchedule

Loading

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