Only add default JNLP container if pod template is empty

[clockworksoul]

Current logic is to add a default JNLP agent container if there is no container specifically named "jnlp" in the pod, without regard for -- and competing with -- any JNLP agents already in the pod. This is despite existing documentation that clearly states an agent must be present, but not what its name should be.

This current behavior cost me quite a number of hours of debug time when my custom JNLP refused to run. Much to my surprise, a kubectl describe on those pods showed there was a mystery interloper competing for ports with my own JNLP container!

This change puts the responsibility on the developer to follow the documentation and ensure that a JNLP agent is present; if no containers are presents at all, only then is a default agent added.

[iocanel]

Sorry to hear about your experience.

The ida of supporting multiple containers in to the slave pod is to allow the slave pod to have access to tooling or services that can serve the build.

Most of the time the user will just need to add the custom container to the pod and leave the jnlp as is.

With the suggested change, if the user adds any custom container, will have to also add a jnlp container, which is not ideal and will also break existing configs or pipelines that assume a default JNLP.

I'd say that we just need to make it crystal clear in the documentation, that custom JNLP containers need to be named accordingly.

[carlossg]

This has been clarified in a9bb356 and 8849676

[jgangemi]

i think a Use custom jnlp container checkbox would have been a better choice instead of requiring the container to be renamed.

[carlossg]

yeah, but now 0.10 is out of the door and there's no easy solution

[jgangemi]

it's fine - renaming the container wasn't that hard, although i do think @clockworksoul's solution should either be reconsidered or you push out a point release that triggers jenkins to issue a breaking change alert in the upgrade page.

not everyone checks the docs before updating and it would save the surprise of things no longer working.

[jgangemi]

one other thing i would suggest is adding some help on the container name field in the UI that also explicitly states to do this.

[iocanel]

How about configuring the jnlp container using a different field, that would have as default value our default jnlp container?

Pros:

1. Obvious to people who to configure/override jnlp, even without reading docs.
2. Doesn't break existing pipelines (if we keep but deprecate overriding the jnlp from the containers list).
3. Clean

Cons:

1. None

Yay!

[jgangemi]

i like your proposal provided it really is backwards compatible. when i went from 0.8 -> 0.9 my existing template stopped working (https://issues.jenkins-ci.org/browse/JENKINS-39077) and me and another co-worker spent a few hours trying to figure out why.

i would also support something that is not backwards compatible provided that warning shows up on the plugin upgrade page and the plugin docs reflect what the change is. i don't mind having to make changes provided i know they need to be made.

[bparees]

I would also like to see the jnlp container be configured separately, so users can choose what image they want to use as their jnlp container.

It would also be nice if there were a way to prevent the jnlp container from being added to the pod template, for cases where the user's image is also the jnlp image. Currently you can do that by naming your container "jnlp", but it would be nice if there were a more explicit way to say "do not add a jnlp container to this pod template" along with the more explicit way to say "when adding a jnlp container to the pod template, here is the image to add"

[iocanel]

Let me rework a little bit my proposal, so that it accomodates @bparees.

We add text field in kubernetes-plugin which allows us to set the name of the base pod template that will be used as the parent of all templates. There anyone can configure the defaults of 'jnlp' or anything else that needs to be applied to ALL templates (env vars, volumes, anything).

Now each pod template will have a separate field / group of fields for configuring the jnlp container so that its crystal clear to the user how to configure the jnlp.

Thoughts?

[bparees]

so every template other than the base template will always inherit from the same base pod template?

how does a template override content from the base pod template? can the child pod template define a container w/ the same name as the base pod template?

is there an implied base template if the user hasn't defined one?

[iocanel]

Yeah, all podTemplates will inherit the base template.

A child template can define a container that exists in the parent. In this case child container will inherit from the parent.

Currently there is no base template assumed, but we could change that.

Full doc on pod template inheritance: https://github.com/jenkinsci/kubernetes-plugin#pod-template-inheritance

[carlossg]

The problem is that in 0.10 we should have marked the existing container as the jnlp container

Now in a future 0.11 we can't really discern when users have one container if it is the jnlp one or another one and they expect the automatically created jnlp container

[clockworksoul]

It's true that this should be marked as a potentially breaking change, but I think it's important for this project to support the kind of flexibility that users will demand. As long as everything is clearly and thoroughly documented, I'm willing to accept the change.

Keep in mind, too, that not everybody has updated to version 0.10 yet. The 0.8 to 0.10 transition broke my system and brought my entire CI pipeline down for a full day; perhaps this change will make things more gentle for upgrades that skip the 0.10 version?

[iocanel]

The change is forward compatible. The current approach of configuring the jnlp container will still be available (so that current config.xml and pipelines can still work with future releases).

We just provide a better alternative and then we can deprecate and remove the current approach.