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

Use AbstractCloudSlave and AbstractCloudComputer #1015

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

Use AbstractCloudSlave and AbstractCloudComputer #1015

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
22 changes: 13 additions & 9 deletions src/main/java/hudson/plugins/ec2/EC2AbstractSlave.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@
import hudson.model.Descriptor;
import hudson.model.Descriptor.FormException;
import hudson.model.Node;
import hudson.model.Slave;
import hudson.plugins.ec2.util.AmazonEC2Factory;
import hudson.plugins.ec2.util.ResettableCountDownLatch;
import hudson.slaves.AbstractCloudSlave;
import hudson.slaves.ComputerLauncher;
import hudson.slaves.NodeProperty;
import hudson.slaves.RetentionStrategy;
Expand Down Expand Up @@ -72,7 +72,7 @@
* @author Kohsuke Kawaguchi
*/
@SuppressWarnings("serial")
public abstract class EC2AbstractSlave extends Slave {
public abstract class EC2AbstractSlave extends AbstractCloudSlave {
public static final Boolean DEFAULT_METADATA_SUPPORTED = Boolean.TRUE;
public static final Boolean DEFAULT_METADATA_ENDPOINT_ENABLED = Boolean.TRUE;
public static final Boolean DEFAULT_METADATA_TOKENS_REQUIRED = Boolean.TRUE;
Expand Down Expand Up @@ -664,7 +664,7 @@ public String getInstanceId() {
}

@Override
public Computer createComputer() {
public EC2Computer createComputer() {
return new EC2Computer(this);
}

Expand All @@ -685,10 +685,6 @@ public static Instance getInstance(String instanceId, EC2Cloud cloud) {
}
return i;
}
/**
* Terminates the instance in EC2.
*/
public abstract void terminate();

void stop() {
try {
Expand Down Expand Up @@ -748,15 +744,23 @@ public boolean isAcceptingTasks() {
void idleTimeout() {
LOGGER.info("EC2 instance idle time expired: " + getInstanceId());
if (!stopOnTerminate) {
terminate();
try {
terminate();
} catch (InterruptedException | IOException e) {
LOGGER.log(Level.WARNING, "Failed to terminate EC2 instance: " + getInstanceId(), e);
}
} else {
stop();
}
}

void launchTimeout() {
LOGGER.info("EC2 instance failed to launch: " + getInstanceId());
terminate();
try {
terminate();
} catch (InterruptedException | IOException e) {
LOGGER.log(Level.WARNING, "Failed to terminate EC2 instance: " + getInstanceId(), e);
}
}

public long getLaunchTimeoutInMillis() {
Expand Down
10 changes: 7 additions & 3 deletions src/main/java/hudson/plugins/ec2/EC2Computer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import edu.umd.cs.findbugs.annotations.CheckForNull;
import hudson.Util;
import hudson.model.Node;
import hudson.slaves.SlaveComputer;
import hudson.slaves.AbstractCloudComputer;
import java.io.IOException;
import java.util.Collections;
import java.util.logging.Level;
Expand All @@ -41,7 +41,7 @@
/**
* @author Kohsuke Kawaguchi
*/
public class EC2Computer extends SlaveComputer {
public class EC2Computer extends AbstractCloudComputer<EC2AbstractSlave> {

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

Expand Down Expand Up @@ -220,7 +220,11 @@ public HttpResponse doDoDelete() throws IOException {
checkPermission(DELETE);
EC2AbstractSlave node = getNode();
if (node != null) {
node.terminate();
try {
node.terminate();
} catch (InterruptedException | IOException e) {
LOGGER.log(Level.WARNING, "Failed to terminate EC2 instance: " + getInstanceId(), e);
}
}
return new HttpRedirect("..");
}
Expand Down
18 changes: 16 additions & 2 deletions src/main/java/hudson/plugins/ec2/EC2ComputerLauncher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,14 @@ public void launch(SlaveComputer slaveComputer, TaskListener listener) {
e);
EC2AbstractSlave ec2AbstractSlave = (EC2AbstractSlave) slaveComputer.getNode();
if (ec2AbstractSlave != null) {
ec2AbstractSlave.terminate();
try {
ec2AbstractSlave.terminate();
} catch (InterruptedException | IOException e1) {
LOGGER.log(
Level.WARNING,
"Failed to terminate EC2 instance: " + ec2AbstractSlave.getInstanceId(),
e1);
}
}
}
} catch (InterruptedException e) {
Expand All @@ -70,7 +77,14 @@ public void launch(SlaveComputer slaveComputer, TaskListener listener) {
e);
EC2AbstractSlave ec2AbstractSlave = (EC2AbstractSlave) slaveComputer.getNode();
if (ec2AbstractSlave != null) {
ec2AbstractSlave.terminate();
try {
ec2AbstractSlave.terminate();
} catch (InterruptedException | IOException e1) {
LOGGER.log(
Level.WARNING,
"Failed to terminate EC2 instance: " + ec2AbstractSlave.getInstanceId(),
e1);
}
}
}
}
Expand Down
7 changes: 5 additions & 2 deletions src/main/java/hudson/plugins/ec2/EC2OndemandSlave.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
import com.amazonaws.services.ec2.AmazonEC2;
import com.amazonaws.services.ec2.model.*;
import hudson.Extension;
import hudson.Functions;
import hudson.model.Computer;
import hudson.model.Descriptor.FormException;
import hudson.model.Node;
import hudson.model.TaskListener;
import hudson.plugins.ec2.ssh.EC2MacLauncher;
import hudson.plugins.ec2.ssh.EC2UnixLauncher;
import hudson.plugins.ec2.win.EC2WindowsLauncher;
Expand Down Expand Up @@ -435,7 +437,7 @@ public EC2OndemandSlave(String instanceId) throws FormException, IOException {
* Terminates the instance in EC2.
*/
@Override
public void terminate() {
protected void _terminate(TaskListener listener) throws IOException, InterruptedException {
if (terminateScheduled.getCount() == 0) {
synchronized (terminateScheduled) {
if (terminateScheduled.getCount() == 0) {
Expand All @@ -456,7 +458,8 @@ public void terminate() {
Jenkins.get().removeNode(this);
Copy link
Member

Choose a reason for hiding this comment

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

ditto here

LOGGER.info("Removed EC2 instance from jenkins controller: " + getInstanceId());
Copy link
Member

Choose a reason for hiding this comment

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

Some of these INFO log messages could probably be replaced with messages to the TaskListener.

} catch (AmazonClientException | IOException e) {
LOGGER.log(Level.WARNING, "Failed to terminate EC2 instance: " + getInstanceId(), e);
Functions.printStackTrace(
e, listener.error("Failed to terminate EC2 instance: " + getInstanceId()));
} finally {
synchronized (terminateScheduled) {
terminateScheduled.countDown();
Expand Down
7 changes: 6 additions & 1 deletion src/main/java/hudson/plugins/ec2/EC2RetentionStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import hudson.model.Queue;
import hudson.plugins.ec2.util.MinimumInstanceChecker;
import hudson.slaves.RetentionStrategy;
import java.io.IOException;
import java.time.Clock;
import java.util.Objects;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -363,7 +364,11 @@ private void postJobAction(Executor executor) {
if (computer.countBusy() <= 1 && !computer.isAcceptingTasks()) {
LOGGER.info("Agent " + slaveNode.instanceId + " is terminated due to maxTotalUses ("
+ slaveNode.maxTotalUses + ")");
slaveNode.terminate();
try {
slaveNode.terminate();
} catch (InterruptedException | IOException e) {
LOGGER.log(Level.WARNING, "Failed to terminate EC2 instance: " + slaveNode.getInstanceId(), e);
}
} else {
if (slaveNode.maxTotalUses == 1) {
LOGGER.info("Agent " + slaveNode.instanceId + " is still in use by more than one ("
Expand Down
7 changes: 6 additions & 1 deletion src/main/java/hudson/plugins/ec2/EC2SlaveMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,12 @@ private void removeDeadNodes() {
try {
if (!ec2Slave.isAlive(true)) {
LOGGER.info("EC2 instance is dead: " + ec2Slave.getInstanceId());
ec2Slave.terminate();
try {
ec2Slave.terminate();
} catch (InterruptedException | IOException e) {
LOGGER.log(
Level.WARNING, "Failed to terminate EC2 instance: " + ec2Slave.getInstanceId(), e);
}
}
} catch (AmazonClientException e) {
if (e instanceof AmazonEC2Exception
Expand Down
16 changes: 9 additions & 7 deletions src/main/java/hudson/plugins/ec2/EC2SpotSlave.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@
import com.amazonaws.services.ec2.model.TerminateInstancesRequest;
import edu.umd.cs.findbugs.annotations.CheckForNull;
import hudson.Extension;
import hudson.Functions;
import hudson.model.Computer;
import hudson.model.Descriptor.FormException;
import hudson.model.TaskListener;
import hudson.plugins.ec2.ssh.EC2UnixLauncher;
import hudson.plugins.ec2.win.EC2WindowsLauncher;
import hudson.slaves.NodeProperty;
Expand Down Expand Up @@ -178,7 +180,7 @@ protected boolean isAlive(boolean force) {
* Cancel the spot request for the instance. Terminate the instance if it is up. Remove the agent from Jenkins.
*/
@Override
public void terminate() {
protected void _terminate(TaskListener listener) throws IOException, InterruptedException {
Copy link
Member

Choose a reason for hiding this comment

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

The impl seems mostly the same as in EC2OndemandSlave. Is there a reason the method cannot simply be pulled up into EC2AbstractSlave.

if (terminateScheduled.getCount() == 0) {
synchronized (terminateScheduled) {
if (terminateScheduled.getCount() == 0) {
Expand All @@ -196,7 +198,8 @@ public void terminate() {
LOGGER.info("Cancelled Spot request: " + spotInstanceRequestId);
Copy link
Member

Choose a reason for hiding this comment

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

ditto here

} catch (AmazonClientException e) {
// Spot request is no longer valid
LOGGER.log(Level.WARNING, "Failed to cancel Spot request: " + spotInstanceRequestId, e);
Functions.printStackTrace(
e, listener.error("Failed to cancel Spot request: " + spotInstanceRequestId));
}

// Terminate the agent if it is running
Expand All @@ -214,15 +217,14 @@ public void terminate() {
LOGGER.info("Terminated EC2 instance (terminated): " + instanceId);
} catch (AmazonClientException e) {
// Spot request is no longer valid
LOGGER.log(
Level.WARNING,
"Failed to terminate the Spot instance: " + instanceId,
e);
Functions.printStackTrace(
e,
listener.error("Failed to terminate the Spot instance: " + instanceId));
}
}
}
} catch (Exception e) {
LOGGER.log(Level.WARNING, "Failed to remove agent: ", e);
Functions.printStackTrace(e, listener.error("Failed to remove agent"));
} finally {
// Remove the instance even if deletion failed, otherwise it will hang around forever in
// the nodes page. One way for this to occur is that an instance was terminated
Copy link
Member

Choose a reason for hiding this comment

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

Pay attention to https://github.com/jenkinsci/jenkins/blob/4248fc0ca6ce108364ed23208063ff20753f5e70/core/src/main/java/hudson/slaves/AbstractCloudSlave.java#L88-L94 which means that after switching fromterminate to _terminate it is no longer your responsibility to removeNode.

Expand Down
9 changes: 4 additions & 5 deletions src/test/java/hudson/plugins/ec2/EC2AbstractSlaveTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@

import com.amazonaws.services.ec2.model.InstanceType;
import hudson.model.Node;
import hudson.model.TaskListener;
import hudson.slaves.NodeProperty;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import org.junit.Rule;
Expand Down Expand Up @@ -56,10 +58,7 @@ public void testGetLaunchTimeoutInMillisShouldNotOverflow() throws Exception {
DEFAULT_METADATA_SUPPORTED) {

@Override
public void terminate() {
// To change body of implemented methods use File | Settings |
// File Templates.
}
protected void _terminate(TaskListener listener) throws IOException, InterruptedException {}

@Override
public String getEc2Type() {
Expand Down Expand Up @@ -150,7 +149,7 @@ public void testMaxUsesBackwardCompat() throws Exception {
ConnectionStrategy.PRIVATE_IP,
0) {
@Override
public void terminate() {}
protected void _terminate(TaskListener listener) throws IOException, InterruptedException {}

@Override
public String getEc2Type() {
Expand Down
15 changes: 9 additions & 6 deletions src/test/java/hudson/plugins/ec2/EC2RetentionStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import hudson.model.Queue.Executable;
import hudson.model.Queue.Task;
import hudson.model.ResourceList;
import hudson.model.TaskListener;
import hudson.model.queue.CauseOfBlockage;
import hudson.plugins.ec2.util.AmazonEC2FactoryMockImpl;
import hudson.plugins.ec2.util.MinimumInstanceChecker;
Expand All @@ -29,6 +30,7 @@
import hudson.security.Permission;
import hudson.slaves.NodeProperty;
import hudson.slaves.OfflineCause;
import java.io.IOException;
import java.time.Clock;
import java.time.Duration;
import java.time.Instant;
Expand Down Expand Up @@ -244,7 +246,7 @@ private EC2Computer computerWithIdleTime(
ConnectionStrategy.PRIVATE_IP,
-1) {
@Override
public void terminate() {}
protected void _terminate(TaskListener listener) throws IOException, InterruptedException {}

@Override
public String getEc2Type() {
Expand Down Expand Up @@ -369,7 +371,7 @@ private EC2Computer computerWithUpTime(
ConnectionStrategy.PRIVATE_IP,
-1) {
@Override
public void terminate() {}
protected void _terminate(TaskListener listener) throws IOException, InterruptedException {}

@Override
public String getEc2Type() {
Expand Down Expand Up @@ -515,9 +517,7 @@ private EC2Computer computerWithUsageLimit(final int usageLimit) throws Exceptio
ConnectionStrategy.PRIVATE_IP,
usageLimit) {
@Override
public void terminate() {
terminateCalled.set(true);
}
protected void _terminate(TaskListener listener) throws IOException, InterruptedException {}

@Override
public String getEc2Type() {
Expand Down Expand Up @@ -1148,6 +1148,9 @@ public void testRetentionStopsAfterActiveRangeEnds() throws Exception {
private static void checkRetentionStrategy(EC2RetentionStrategy rs, EC2Computer c) throws InterruptedException {
rs.check(c);
EC2AbstractSlave node = c.getNode();
assertTrue(node.terminateScheduled.await(10, TimeUnit.SECONDS));
// checks if node has been terminated within timeout. if node is null, it has already been terminated
if (node != null) {
assertTrue(node.terminateScheduled.await(10, TimeUnit.SECONDS));
}
}
}
3 changes: 2 additions & 1 deletion src/test/java/hudson/plugins/ec2/ssh/verifiers/SshHostKeyVerificationStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import com.trilead.ssh2.Connection;
import com.trilead.ssh2.ServerHostKeyVerifier;
import hudson.model.Node;
import hudson.model.TaskListener;
import hudson.plugins.ec2.ConnectionStrategy;
import hudson.plugins.ec2.EC2AbstractSlave;
import hudson.plugins.ec2.EC2Computer;
Expand Down Expand Up @@ -378,7 +379,7 @@ private static MockEC2Computer createComputer(String suffix) throws Exception {
ConnectionStrategy.PRIVATE_IP,
-1) {
@Override
public void terminate() {}
protected void _terminate(TaskListener listener) throws IOException, InterruptedException {}

@Override
public String getEc2Type() {
Expand Down
5 changes: 2 additions & 3 deletions src/test/java/hudson/plugins/ec2/util/EC2AgentFactoryMockImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import com.amazonaws.AmazonClientException;
import hudson.Extension;
import hudson.model.Computer;
import hudson.model.Descriptor;
import hudson.plugins.ec2.*;
import hudson.plugins.ec2.Tenancy;
Expand Down Expand Up @@ -182,7 +181,7 @@ private MockEC2OndemandSlave(
}

@Override
public Computer createComputer() {
public EC2Computer createComputer() {
return new MockEC2Computer(this);
}
}
Expand Down Expand Up @@ -236,7 +235,7 @@ private MockEC2SpotSlave(
}

@Override
public Computer createComputer() {
public EC2Computer createComputer() {
return new MockEC2Computer(this);
}
}
Expand Down
Loading