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

Limit pod name to 63 characters, and shorten randomly generated string #143

Merged
merged 1 commit into from Mar 1, 2017

Conversation

Projects
None yet
2 participants
@austinmoore-

austinmoore- commented Feb 28, 2017

This PR has two small changes. Hope that it's alright that they're both in one PR. They're closely related.

The first fixes a bug where the plugin fails to use a pod if the pod's name is over 63 characters. I believe this is due to the fact that the JNLP container has a max hostname of 64 bytes (including the null terminator).

$ getconf HOST_NAME_MAX
64

As as a result, when workspace.child(HOSTNAME_FILE).readToString() is executed, it returns the first 63 characters of the pod name, which causes the plugin to look for the wrong pod.

The second change makes the random string appended to the end of the pod name more like Kubernetes' string generation. With this PR, only 5 characters are appended (or replaced if it exceeds the max length) to the end of the string.

@iocanel iocanel merged commit 029f90b into jenkinsci:master Mar 1, 2017

1 check passed

Jenkins This pull request looks good
Details
@iocanel

This comment has been minimized.

Show comment
Hide comment
@iocanel

iocanel commented Mar 1, 2017

Thanks!

@iocanel

This comment has been minimized.

Show comment
Hide comment
@iocanel

iocanel Mar 17, 2017

@austinmoore-: I think that here lies an issue that I missed in my original review. The PodTemplate that we generate using the pipeline is added to the cloud configuration. This has the following side effects:

  • Two builds using podTemplates with the same name can get in each other way
  • It's possible to mess up with preconfigured podTemplate (found in config.xml)

So I am considering reverting just that part of the commit to how it used to be.

@austinmoore-: I think that here lies an issue that I missed in my original review. The PodTemplate that we generate using the pipeline is added to the cloud configuration. This has the following side effects:

  • Two builds using podTemplates with the same name can get in each other way
  • It's possible to mess up with preconfigured podTemplate (found in config.xml)

So I am considering reverting just that part of the commit to how it used to be.

This comment has been minimized.

Show comment
Hide comment
@iocanel

iocanel replied Mar 17, 2017

@carlossg: wdyt?

@austinmoore-

This comment has been minimized.

Show comment
Hide comment
@austinmoore-

austinmoore- Mar 21, 2017

@iocanel: Hey, sorry you're seeing problems with the PR. I'm happy to help fix these issues. I haven't had time to dive into code looking for you've said yet but wanted to get some clarification from things off the top of my head.

Two builds using podTemplates with the same name can get in each other way

I frequently launch jobs with the same name, but a different label. Also, if left unspecified the name "kubernetes" is chosen by default. As long as the label is unique it works. In what ways are you seeing the names conflict?

Also, what specific part of the commit are you thinking of reverting? Some context might help me understand these problems.

Thanks!

austinmoore- commented Mar 21, 2017

@iocanel: Hey, sorry you're seeing problems with the PR. I'm happy to help fix these issues. I haven't had time to dive into code looking for you've said yet but wanted to get some clarification from things off the top of my head.

Two builds using podTemplates with the same name can get in each other way

I frequently launch jobs with the same name, but a different label. Also, if left unspecified the name "kubernetes" is chosen by default. As long as the label is unique it works. In what ways are you seeing the names conflict?

Also, what specific part of the commit are you thinking of reverting? Some context might help me understand these problems.

Thanks!

@iocanel

This comment has been minimized.

Show comment
Hide comment
@iocanel

iocanel Mar 21, 2017

No worries. It was a step to the right direction, with just some minor details that needed to be addressed.

Instantiating the template is usually done via node that uses a label, so no issues there. But when we are nesting podTemplates or use the inheritFrom that is done by name. And here's where possible issues may spawn. Also for the duration of the podTemplate scope, the podTemplate is added to the Jenkins configuration (as if it was done by hand) and I think that there we could also have issues with name conflicts (not 100% sure about the last part).

Anyway, I pushed a fix into the master which I think resolves the issue: bf245fe

iocanel commented Mar 21, 2017

No worries. It was a step to the right direction, with just some minor details that needed to be addressed.

Instantiating the template is usually done via node that uses a label, so no issues there. But when we are nesting podTemplates or use the inheritFrom that is done by name. And here's where possible issues may spawn. Also for the duration of the podTemplate scope, the podTemplate is added to the Jenkins configuration (as if it was done by hand) and I think that there we could also have issues with name conflicts (not 100% sure about the last part).

Anyway, I pushed a fix into the master which I think resolves the issue: bf245fe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment