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

Adding basic retention #91

Merged
merged 8 commits into from Nov 30, 2016

Conversation

Projects
None yet
3 participants
@jeffersongirao

jeffersongirao commented Nov 14, 2016

  • The idea is to re-use slaves after successful builds, the slave is terminated after being idle for the time defined in idleMinutes. That is useful for builds that rely heavily on cached data for "speed".
Show outdated Hide outdated src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java
/**
* Kubernetes cloud provider.
*
*

This comment has been minimized.

@carlossg

carlossg Nov 20, 2016

it helps reviewing PRs if there are no spurious changes in imports or whitespace

@carlossg

carlossg Nov 20, 2016

it helps reviewing PRs if there are no spurious changes in imports or whitespace

This comment has been minimized.

@jeffersongirao

jeffersongirao Nov 21, 2016

My bad, I reverted such changes.

@jeffersongirao

jeffersongirao Nov 21, 2016

My bad, I reverted such changes.

Show outdated Hide outdated src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java
@@ -44,9 +38,11 @@
private transient final KubernetesCloud cloud;
@DataBoundConstructor
@Deprecated

This comment has been minimized.

@carlossg

carlossg Nov 20, 2016

I think there is no need to deprecate it
It should call the other constructor this(...) to avoid duplication

@carlossg

carlossg Nov 20, 2016

I think there is no need to deprecate it
It should call the other constructor this(...) to avoid duplication

This comment has been minimized.

@jeffersongirao
Show outdated Hide outdated ...sources/org/csanchez/jenkins/plugins/kubernetes/PodTemplate/config.jelly
@@ -27,6 +27,10 @@
<f:textbox/>
</f:entry>
<f:entry field="retainedAfterTask" title="${%Retain slave after task}">

This comment has been minimized.

@carlossg

carlossg Nov 20, 2016

instead of having a boolean, I think it's better to have idleMinutes here, like the docker plugin, so it can be set per template for how long it should be kept

@carlossg

carlossg Nov 20, 2016

instead of having a boolean, I think it's better to have idleMinutes here, like the docker plugin, so it can be set per template for how long it should be kept

This comment has been minimized.

@jeffersongirao
@jeffersongirao

This comment has been minimized.

Show comment
Hide comment
@jeffersongirao

jeffersongirao Nov 21, 2016

Thanks for the feedback, I've made the suggested changes. Cheers!

jeffersongirao commented Nov 21, 2016

Thanks for the feedback, I've made the suggested changes. Cheers!

Show outdated Hide outdated src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java
RetentionStrategy rs)
throws Descriptor.FormException, IOException {
this(template, nodeDescription, cloud, labelStr);

This comment has been minimized.

@carlossg

carlossg Nov 21, 2016

I think you'd better do it the other way around, call this(template, nodeDescription, cloud, labelStr, rs); in the first constructor, call super in the second one

@carlossg

carlossg Nov 21, 2016

I think you'd better do it the other way around, call this(template, nodeDescription, cloud, labelStr, rs); in the first constructor, call super in the second one

Show outdated Hide outdated src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java
@@ -49,6 +49,8 @@
private int instanceCap = Integer.MAX_VALUE;
private int idleMinutes = Integer.MIN_VALUE;

This comment has been minimized.

@carlossg

carlossg Nov 21, 2016

why not just use Integer and set it to null?

@carlossg

carlossg Nov 21, 2016

why not just use Integer and set it to null?

This comment has been minimized.

@jeffersongirao

jeffersongirao Nov 21, 2016

Just afraid of NullPointerException in general. I will change it.

@jeffersongirao

jeffersongirao Nov 21, 2016

Just afraid of NullPointerException in general. I will change it.

Show outdated Hide outdated src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java
@DataBoundSetter
public void setIdleMinutesStr(String idleMinutes) {
if ("".equals(idleMinutes)) {

This comment has been minimized.

@carlossg

carlossg Nov 21, 2016

you'd want to use StringUtils.isBlank() not just the empty string

@carlossg

carlossg Nov 21, 2016

you'd want to use StringUtils.isBlank() not just the empty string

This comment has been minimized.

@jeffersongirao

jeffersongirao Nov 21, 2016

Is it ok to introduce a dependency here? I was just trying to be consistent with what is implemented for setInstanceCapStr.

@jeffersongirao

jeffersongirao Nov 21, 2016

Is it ok to introduce a dependency here? I was just trying to be consistent with what is implemented for setInstanceCapStr.

Show outdated Hide outdated src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesCloud.java
if (t.getIdleMinutes() == Integer.MIN_VALUE) {
retentionStrategy = new OnceRetentionStrategy(cloud.getRetentionTimeout());
} else {
retentionStrategy = new CloudRetentionStrategy(cloud.getRetentionTimeout());

This comment has been minimized.

@carlossg

carlossg Nov 21, 2016

shouldn't be t.getIdleMinutes() to use the idle time for the template ?

@carlossg

carlossg Nov 21, 2016

shouldn't be t.getIdleMinutes() to use the idle time for the template ?

This comment has been minimized.

@jeffersongirao

jeffersongirao Nov 21, 2016

You are right, fixing it.

@jeffersongirao

jeffersongirao Nov 21, 2016

You are right, fixing it.

@jeffersongirao

This comment has been minimized.

Show comment
Hide comment
@jeffersongirao

jeffersongirao Nov 21, 2016

I did changes, please let me know if you prefer me to squash the commits in a single one. Cheers!

jeffersongirao commented Nov 21, 2016

I did changes, please let me know if you prefer me to squash the commits in a single one. Cheers!

@carlossg

Just one last comment, and then it needs rebase

Show outdated Hide outdated src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesSlave.java
@Deprecated
public KubernetesSlave(PodTemplate template, String nodeDescription, KubernetesCloud cloud, Label label)
throws Descriptor.FormException, IOException {

This comment has been minimized.

@carlossg

carlossg Nov 29, 2016

can you just call

this(template, nodeDescription, cloud, label, new OnceRetentionStrategy(cloud.getRetentionTimeout()))

to avoid duplicated code

@carlossg

carlossg Nov 29, 2016

can you just call

this(template, nodeDescription, cloud, label, new OnceRetentionStrategy(cloud.getRetentionTimeout()))

to avoid duplicated code

@jeffersongirao

This comment has been minimized.

Show comment
Hide comment
@jeffersongirao

jeffersongirao Nov 30, 2016

Fixed and rebased. Thanks!

jeffersongirao commented Nov 30, 2016

Fixed and rebased. Thanks!

@carlossg carlossg merged commit 78b1c25 into jenkinsci:master Nov 30, 2016

1 check passed

Jenkins This pull request looks good
Details
@carlossg

This comment has been minimized.

Show comment
Hide comment
@carlossg

carlossg commented Nov 30, 2016

thanks!

@JeanMertz

This comment has been minimized.

Show comment
Hide comment
@JeanMertz

JeanMertz Dec 2, 2016

I'm not sure if this is the PR that caused our issues, but it's closely related, so I'll put this here:

We use different docker images for each of our projects running on Jenkins using the kubernetes-plugin.

I've noticed recently that a lot of times a job of project A might end up on a pod/container of project B.

Because each pod/container setup is highly coupled to the different projects, a lot of tools are missing for project A to run inside a pod of project B, and thus the run fails.

We haven't set the idleMinutes value, so it should default to 0, and thus OnceRetentionStrategy, but that doesn't seem to be the case.

Something else that I noticed: since this PR(?) any custom pod template that we define in a Jenkinsfile gets automatically added to the list of global list of "Kubernetes Pod Template" configs in the global configuration page of Jenkins.

This means that after a while, we end up with thousands of templates on that page, grinding that page to a halt. Possibly, this is the cause of the shared pod templates we are seeing?

JeanMertz commented Dec 2, 2016

I'm not sure if this is the PR that caused our issues, but it's closely related, so I'll put this here:

We use different docker images for each of our projects running on Jenkins using the kubernetes-plugin.

I've noticed recently that a lot of times a job of project A might end up on a pod/container of project B.

Because each pod/container setup is highly coupled to the different projects, a lot of tools are missing for project A to run inside a pod of project B, and thus the run fails.

We haven't set the idleMinutes value, so it should default to 0, and thus OnceRetentionStrategy, but that doesn't seem to be the case.

Something else that I noticed: since this PR(?) any custom pod template that we define in a Jenkinsfile gets automatically added to the list of global list of "Kubernetes Pod Template" configs in the global configuration page of Jenkins.

This means that after a while, we end up with thousands of templates on that page, grinding that page to a halt. Possibly, this is the cause of the shared pod templates we are seeing?

@JeanMertz

This comment has been minimized.

Show comment
Hide comment
@JeanMertz

JeanMertz Dec 2, 2016

If I do a "reload from disk", the extra templates disappear from the global configuration (leaving only those that we actually defined in the global scope)

JeanMertz commented Dec 2, 2016

If I do a "reload from disk", the extra templates disappear from the global configuration (leaving only those that we actually defined in the global scope)

@jeffersongirao

This comment has been minimized.

Show comment
Hide comment
@jeffersongirao

jeffersongirao Dec 7, 2016

Hello @JeanMertz, I tried to reproduce your issue but I could not. Could you please, give more details about the version of Jenkins you are trying it out and example of the Jenkinsfile? Thanks!

jeffersongirao commented Dec 7, 2016

Hello @JeanMertz, I tried to reproduce your issue but I could not. Could you please, give more details about the version of Jenkins you are trying it out and example of the Jenkinsfile? Thanks!

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