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

handle null retention policy resulting from direct xml pod template i… #381

Merged
merged 2 commits into from
Oct 17, 2018

Conversation

gabemontero
Copy link
Contributor

…njection (seen during agent termination)

Fixes https://issues.jenkins-ci.org/browse/JENKINS-53260

@carlossg ptal

@bparees @adambkaplan fyi

@bparees
Copy link
Contributor

bparees commented Oct 1, 2018

lgtm

Copy link
Member

@Vlatombe Vlatombe left a comment

Choose a reason for hiding this comment

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

I think a better fix would be to add the default pod retention policy upon deserialization in

protected Object readResolve() {
if (containers == null) {
// upgrading from 0.8
containers = new ArrayList<>();
ContainerTemplate containerTemplate = new ContainerTemplate(KubernetesCloud.JNLP_NAME, this.image);
containerTemplate.setCommand(command);
containerTemplate.setArgs(Strings.isNullOrEmpty(args) ? FALLBACK_ARGUMENTS : args);
containerTemplate.setPrivileged(privileged);
containerTemplate.setAlwaysPullImage(alwaysPullImage);
containerTemplate.setEnvVars(envVars);
containerTemplate.setResourceRequestMemory(resourceRequestMemory);
containerTemplate.setResourceLimitCpu(resourceLimitCpu);
containerTemplate.setResourceLimitMemory(resourceLimitMemory);
containerTemplate.setResourceRequestCpu(resourceRequestCpu);
containerTemplate.setWorkingDir(remoteFs);
containers.add(containerTemplate);
}

@bparees
Copy link
Contributor

bparees commented Oct 10, 2018

@adambkaplan did @gabemontero catch you up on this?

@gabemontero
Copy link
Contributor Author

gabemontero commented Oct 10, 2018

I did @bparees though we had left it that this was done ... I'll see about driving this to closure

At first blush @Vlatombe 's suggestion should work as well

I'll verify why we presumably wait on @carlossg to decide which way he wants to go

@gabemontero
Copy link
Contributor Author

Confirmed updating the deserialization path also works

pushed a second comment with that in place and the prior fix commented out

I'll squash / clean up / uncomment code as needed based on where the consensus lands after @carlossg chimes in.

thanks

@gabemontero
Copy link
Contributor Author

bump @carlossg ... see #381 (comment) for latest status

@carlossg carlossg added bug Bug Fixes and removed bug Bug Fixes labels Oct 17, 2018
@carlossg carlossg merged commit 9661613 into jenkinsci:master Oct 17, 2018
@gabemontero gabemontero deleted the fix-pod-retention-npe branch October 17, 2018 14:01
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.

4 participants