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.

Sign up for GitHub

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

Request for feedback: Add new fixed-lifespan cloud retention strategy. #96

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Request for feedback: Add new fixed-lifespan cloud retention strategy. #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/main/java/org/jenkinsci/plugins/vSphereCloudSlaveTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@
import hudson.model.Node.Mode;
import hudson.model.labels.LabelAtom;
import hudson.plugins.sshslaves.SSHLauncher;
import hudson.slaves.NodeProperty;
import hudson.slaves.CommandLauncher;
import hudson.slaves.ComputerLauncher;
import hudson.slaves.JNLPLauncher;
import hudson.slaves.NodeProperty;
import hudson.slaves.RetentionStrategy;
import hudson.util.FormValidation;

Expand All @@ -51,6 +51,7 @@
import jenkins.model.Jenkins;
import jenkins.slaves.JnlpSlaveAgentProtocol;

import org.jenkinsci.plugins.vsphere.FixedLifespanCloudRetentionStrategy;
import org.jenkinsci.plugins.vsphere.RunOnceCloudRetentionStrategy;
import org.jenkinsci.plugins.vsphere.VSphereCloudRetentionStrategy;
import org.jenkinsci.plugins.vsphere.VSphereConnectionConfig;
Expand Down Expand Up @@ -497,6 +498,12 @@ private RetentionStrategy<?> determineRetention() {
templateStrategy.getIdleMinutes());
return cloneStrategy;
}
if (retentionStrategy instanceof FixedLifespanCloudRetentionStrategy) {
final FixedLifespanCloudRetentionStrategy templateStrategy = (FixedLifespanCloudRetentionStrategy) retentionStrategy;
final FixedLifespanCloudRetentionStrategy cloneStrategy = new FixedLifespanCloudRetentionStrategy(
templateStrategy.getLifespanMinutes());
return cloneStrategy;
}
throw new IllegalStateException(
"Unsupported retentionStrategy (" + retentionStrategy + ") in template configuration");
}
Expand Down Expand Up @@ -606,6 +613,7 @@ public static List<Descriptor<RetentionStrategy<?>>> getRetentionStrategyDescrip
final List<Descriptor<RetentionStrategy<?>>> result = new ArrayList<>();
result.add(RunOnceCloudRetentionStrategy.DESCRIPTOR);
result.add(VSphereCloudRetentionStrategy.DESCRIPTOR);
result.add(FixedLifespanCloudRetentionStrategy.DESCRIPTOR);
return result;
}
}
Expand Down
114 changes: 114 additions & 0 deletions src/main/java/org/jenkinsci/plugins/vsphere/FixedLifespanCloudRetentionStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
package org.jenkinsci.plugins.vsphere;

import hudson.model.Descriptor;
import hudson.model.InvisibleAction;
import hudson.slaves.AbstractCloudComputer;
import hudson.slaves.AbstractCloudSlave;
import hudson.slaves.CloudRetentionStrategy;
import hudson.slaves.RetentionStrategy;
import hudson.util.TimeUnit2;

import java.io.IOException;
import java.util.concurrent.ExecutionException;
import java.util.logging.Level;
import java.util.logging.Logger;

import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.stapler.DataBoundConstructor;

import static java.util.logging.Level.WARNING;

public final class FixedLifespanCloudRetentionStrategy extends RetentionStrategy<AbstractCloudComputer> {

private static final Logger LOGGER = Logger.getLogger(CloudRetentionStrategy.class.getName());

private final int lifespanMinutes;

@DataBoundConstructor
public FixedLifespanCloudRetentionStrategy(int lifespanMinutes) {
this.lifespanMinutes = lifespanMinutes;
}

public int getLifespanMinutes() {
return lifespanMinutes;
}

@Override
public long check(AbstractCloudComputer c) {
final long creationTimeMillis = c.getAction(CloudComputerCreatedOnInvisibleAction.class).getCreationTimeMillis();
final long ageMillis = System.currentTimeMillis() - creationTimeMillis;
final String cname = c.getName();
if (!isAtEndOfLife() && ageMillis > TimeUnit2.MINUTES.toMillis(lifespanMinutes)) {
LOGGER.log(Level.FINE, "Will terminate {0} once idle - lifespan of {1} minutes reached.", new Object[] { cname, lifespanMinutes });
setAtEndOfLife();
final VSphereOfflineCause cause = new VSphereOfflineCause(Messages._fixedLifespanCloudRetentionStrategy_OfflineReason_LifespanReached(String.valueOf(lifespanMinutes)));
try {
c.disconnect(cause).get();
} catch (InterruptedException | ExecutionException e) {
LOGGER.log(WARNING, "Failed to disconnect " + cname, e);
Copy link
Member

Choose a reason for hiding this comment

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

If this fails to disconnect, it looks like setAtEndOfLife() will still be set so that no further attempts will be made to disconnect the slave (until Jenkins gets restarted).
That doesn't seem right to me.

}
}
if (isAtEndOfLife() && c.isIdle()) {
final AbstractCloudSlave computerNode = c.getNode();
if (computerNode != null) {
try {
LOGGER.log(Level.FINER, "Initiating termination of {0}.", cname);
computerNode.terminate();
} catch (InterruptedException | IOException e) {
LOGGER.log(WARNING, "Failed to terminate " + cname, e);
}
}
}
return 1; // re-check in 1 minute
Copy link
Member

Choose a reason for hiding this comment

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

Once a machine has reached end-of-life then re-checking every minute is good.
However, before it's reached end-of-life, we don't need to check every minute - we should return "number of minutes until we reach end of line".

}

@Override
public DescriptorImpl getDescriptor() {
return DESCRIPTOR;
}

@Override
public void start(AbstractCloudComputer c) {
c.addAction(new CloudComputerCreatedOnInvisibleAction(System.currentTimeMillis()));
super.start(c);
c.connect(false);
}

@Override
public boolean isAcceptingTasks(AbstractCloudComputer c) {
return !isAtEndOfLife();
}

private transient boolean atEndOfLife;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a big fan of having class fields scattered throughout the code - I much prefer to see all the fields at the top.


private synchronized boolean isAtEndOfLife() {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that synchronized here is going to achieve thread-safety.
e.g. the check method calls isAtEndOfLife() twice, so if you only lock in here, check might get two different values when it calls the method if another thread has called setAtEndOfLife() in between.
You probably want to make check() be synchronized (or add a synchronized section within it) so that you can ensure that you get a consistent and controlled logic flow within that method.
...also, if you put it there, you can reduce the number of synchronized blocks to just one - thread synchronization is expensive so we don't want to do that any more than we absolutely have to.
...and I'd suggest that you check with other retention strategies (in this and other plugins) and verify that it's needed at all because, if we don't need to make this code thread-safe, we shouldn't add code making it look like it is.

return atEndOfLife;
}

private synchronized void setAtEndOfLife() {
atEndOfLife = true;
}

@Restricted(NoExternalUse.class)
public static final DescriptorImpl DESCRIPTOR = new DescriptorImpl();

public static final class DescriptorImpl extends Descriptor<RetentionStrategy<?>> {
@Override
public String getDisplayName() {
return "vSphere Fixed Lifespan Retention Strategy";
}
}

public static class CloudComputerCreatedOnInvisibleAction extends InvisibleAction {
private final long creationTimeMillis;

CloudComputerCreatedOnInvisibleAction(long creationTimeMillis) {
this.creationTimeMillis = creationTimeMillis;
}

long getCreationTimeMillis() {
return creationTimeMillis;
}
}
}
6 changes: 6 additions & 0 deletions .../resources/org/jenkinsci/plugins/vsphere/FixedLifespanCloudRetentionStrategy/config.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
<f:entry title="${%Lifespan Timeout}" field="lifespanMinutes">
<f:number default="60"/>
</f:entry>
</j:jelly>
Copy link
Member

Choose a reason for hiding this comment

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

Please add a linefeed to the end of the file.

1 change: 1 addition & 0 deletions src/main/resources/org/jenkinsci/plugins/vsphere/Messages.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
runOnceCloudRetentionStrategy.OfflineReason.BuildHasRun=VSphere Cloud Slave configured was to run one build only
fixedLifespanCloudRetentionStrategy.OfflineReason.LifespanReached=VSphere Cloud Agent has reached the end of its configured lifespan of {0} minutes
Copy link
Member

Choose a reason for hiding this comment

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

Please add a linefeed to the end of the file.