From 9c9acdbdbac9c863cb9b302bd6039836fb4e1743 Mon Sep 17 00:00:00 2001 From: zaro0508 Date: Fri, 22 Mar 2013 15:09:20 -0700 Subject: [PATCH] fix executor names, clean up, remove code duplication src/main/java/hudson/plugins/gearman/AbstractWorkerThread.java Removed Id field, it was initially added because I thought it was the plugin's responsibility to cancel jobs that are on the gearman queue. We've decided that it will be the client (zuul or otherwise) responsibility to cancel jobs from the gearman queue. The gearman plugin will cancel jobs that have already been put on the jenkins queue. src/main/java/hudson/plugins/gearman/ComputerListenerImpl.java src/main/java/hudson/plugins/gearman/ExecutorWorkerThread.java src/main/java/hudson/plugins/gearman/GearmanPluginConfig.java src/main/java/hudson/plugins/gearman/GearmanProxy.java src/main/java/hudson/plugins/gearman/ManagementWorkerThread.java src/main/java/hudson/plugins/gearman/StartJobWorker.java Refactor to reduce code duplication. Consolidated creation of management worker and executor workers. Added a fix so that executors spawned on master node is named 'master-manager' for the manager and 'master-exec-#' for executors src/test/java/hudson/plugins/gearman/ManagementWorkerThreadTest.java Added test to make sure worker name is set correctly src/main/java/hudson/plugins/gearman/GearmanPluginUtil.java src/test/java/hudson/plugins/gearman/GearmanPluginUtilTest.java Useful utils for the gearman plugin with tests Change-Id: I96e097dc0dbf5cd78e5e82af584976085aee61b3 --- .../plugins/gearman/AbstractWorkerThread.java | 18 +---- .../plugins/gearman/ComputerListenerImpl.java | 35 +-------- .../plugins/gearman/ExecutorWorkerThread.java | 5 -- .../plugins/gearman/GearmanPluginConfig.java | 2 +- .../plugins/gearman/GearmanPluginUtil.java | 58 ++++++++++++++ .../hudson/plugins/gearman/GearmanProxy.java | 65 ++++++++++------ .../gearman/ManagementWorkerThread.java | 6 +- .../plugins/gearman/StartJobWorker.java | 7 +- .../gearman/GearmanPluginUtilTest.java | 75 +++++++++++++++++++ .../gearman/ManagementWorkerThreadTest.java | 9 ++- 10 files changed, 191 insertions(+), 89 deletions(-) create mode 100644 src/main/java/hudson/plugins/gearman/GearmanPluginUtil.java create mode 100644 src/test/java/hudson/plugins/gearman/GearmanPluginUtilTest.java diff --git a/src/main/java/hudson/plugins/gearman/AbstractWorkerThread.java b/src/main/java/hudson/plugins/gearman/AbstractWorkerThread.java index 53580b0..3992fa8 100644 --- a/src/main/java/hudson/plugins/gearman/AbstractWorkerThread.java +++ b/src/main/java/hudson/plugins/gearman/AbstractWorkerThread.java @@ -19,7 +19,6 @@ package hudson.plugins.gearman; import java.util.Date; -import java.util.UUID; import org.gearman.common.GearmanNIOJobServerConnection; import org.gearman.worker.GearmanWorker; @@ -47,7 +46,6 @@ public abstract class AbstractWorkerThread implements Runnable { protected GearmanWorker worker; private final GearmanNIOJobServerConnection conn; private Thread thread; - private UUID id; public AbstractWorkerThread(String host, int port) { this(host, port, DEFAULT_EXECUTOR_NAME); @@ -57,7 +55,6 @@ public abstract class AbstractWorkerThread implements Runnable { setHost(host); setPort(port); setName(name); - setId(UUID.randomUUID()); worker = new GearmanWorkerImpl(); conn = new GearmanNIOJobServerConnection(host, port); } @@ -86,15 +83,6 @@ public abstract class AbstractWorkerThread implements Runnable { this.name = name; } - public UUID getId() { - return id; - } - - public void setId(UUID id) { - this.id = id; - } - - /* * Register jobs with the gearman worker. * This method should be overriden. @@ -122,8 +110,7 @@ public abstract class AbstractWorkerThread implements Runnable { if (worker.isRunning()) { try { - logger.info("---- Stopping " + getName() + ":" + getId().toString() + - " (" + new Date().toString() + ")"); + logger.info("---- Stopping " + getName() +" (" + new Date().toString() + ")"); worker.unregisterAll(); worker.shutdown(); } catch (Exception e) { @@ -154,8 +141,7 @@ public abstract class AbstractWorkerThread implements Runnable { public void run() { if (!worker.isRunning()) { - logger.info("---- Starting Worker "+ getName() + ":" + getId().toString() + - " ("+new Date().toString()+")"); + logger.info("---- Starting Worker "+ getName() +" ("+new Date().toString()+")"); worker.setWorkerID(name); worker.setJobUniqueIdRequired(true); worker.addServer(conn); diff --git a/src/main/java/hudson/plugins/gearman/ComputerListenerImpl.java b/src/main/java/hudson/plugins/gearman/ComputerListenerImpl.java index ac51316..771f9f6 100644 --- a/src/main/java/hudson/plugins/gearman/ComputerListenerImpl.java +++ b/src/main/java/hudson/plugins/gearman/ComputerListenerImpl.java @@ -20,7 +20,6 @@ package hudson.plugins.gearman; import hudson.Extension; import hudson.model.TaskListener; import hudson.model.Computer; -import hudson.model.Node; import hudson.slaves.ComputerListener; import java.io.IOException; @@ -104,49 +103,21 @@ public class ComputerListenerImpl extends ComputerListener { * Spawn management executor worker. This worker does not need any * executors. It only needs to work with gearman. */ - String host = GearmanPluginConfig.get().getHost(); - int port = GearmanPluginConfig.get().getPort(); - - ManagementWorkerThread mwt = new ManagementWorkerThread(host, port, host); - mwt.registerJobs(); - mwt.start(); - GearmanProxy.getGmwtHandles().add(mwt); + GearmanProxy.createManagementWorker(); /* * Spawn executors for the jenkins master Need to treat the master * differently than slaves because the master is not the same as a * slave */ - Node masterNode = c.getNode(); - int executors = c.getExecutors().size(); - for (int i = 0; i < executors; i++) { - // create a gearman worker for every executor on the master - ExecutorWorkerThread ewt = new ExecutorWorkerThread(GearmanPluginConfig.get().getHost(), - GearmanPluginConfig.get().getPort(), - "master-exec"+ Integer.toString(i), - masterNode); - ewt.registerJobs(); - ewt.start(); - GearmanProxy.getGewtHandles().add(ewt); - } + GearmanProxy.createExecutorWorkersOnNode(c); } // on creation of new slave if (Computer.currentComputer() != c && !GearmanProxy.getGewtHandles().contains(c)) { - Node node = c.getNode(); - int slaveExecutors = c.getExecutors().size(); - // create a gearman worker for every executor on the slave - for (int i = 0; i < slaveExecutors; i++) { - ExecutorWorkerThread gwt = new ExecutorWorkerThread( - GearmanPluginConfig.get().getHost(), - GearmanPluginConfig.get().getPort(), node.getNodeName() - + "-exec" + Integer.toString(i), node); - gwt.registerJobs(); - gwt.start(); - GearmanProxy.getGewtHandles().add(gwt); - } + GearmanProxy.createExecutorWorkersOnNode(c); } } } diff --git a/src/main/java/hudson/plugins/gearman/ExecutorWorkerThread.java b/src/main/java/hudson/plugins/gearman/ExecutorWorkerThread.java index fe7760c..b5581d9 100644 --- a/src/main/java/hudson/plugins/gearman/ExecutorWorkerThread.java +++ b/src/main/java/hudson/plugins/gearman/ExecutorWorkerThread.java @@ -48,11 +48,6 @@ public class ExecutorWorkerThread extends AbstractWorkerThread{ private final Node node; // constructor - public ExecutorWorkerThread(String host, int port, Node node) { - super(host, port); - this.node = node; - } - public ExecutorWorkerThread(String host, int port, String name, Node node) { super(host, port, name); this.node = node; diff --git a/src/main/java/hudson/plugins/gearman/GearmanPluginConfig.java b/src/main/java/hudson/plugins/gearman/GearmanPluginConfig.java index 01c4769..77926d0 100644 --- a/src/main/java/hudson/plugins/gearman/GearmanPluginConfig.java +++ b/src/main/java/hudson/plugins/gearman/GearmanPluginConfig.java @@ -109,7 +109,7 @@ public class GearmanPluginConfig extends GlobalConfiguration { "host"); } - gearmanProxy.init_worker(host, port); + gearmanProxy.init_worker(); } else { gearmanProxy.stop_all(); diff --git a/src/main/java/hudson/plugins/gearman/GearmanPluginUtil.java b/src/main/java/hudson/plugins/gearman/GearmanPluginUtil.java new file mode 100644 index 0000000..8e08c41 --- /dev/null +++ b/src/main/java/hudson/plugins/gearman/GearmanPluginUtil.java @@ -0,0 +1,58 @@ +/* + * + * Copyright 2013 Hewlett-Packard Development Company, L.P. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package hudson.plugins.gearman; + +import hudson.model.Computer; +import hudson.model.Node; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * This class contains some useful utilities for this plugin + * + * @author Khai Do + */ +public class GearmanPluginUtil { + + private static final Logger logger = LoggerFactory + .getLogger(Constants.PLUGIN_LOGGER_NAME); + + /* + * This method returns the real node name. Master node + * by default has an empty string for the name. But you + * need to use "master" to tell jenkins to do stuff, + * namely like schedule a build. + * + * @param Node + * The node to lookup + * + * @return + * "master" for the master node or assigned name of the slave node + */ + public static String getRealName(Node node) { + + if (Computer.currentComputer() == node.toComputer()) { + return "master"; + } else { + return node.getNodeName(); + } + } + +} diff --git a/src/main/java/hudson/plugins/gearman/GearmanProxy.java b/src/main/java/hudson/plugins/gearman/GearmanProxy.java index 72fdcfb..9d5c5f1 100644 --- a/src/main/java/hudson/plugins/gearman/GearmanProxy.java +++ b/src/main/java/hudson/plugins/gearman/GearmanProxy.java @@ -62,7 +62,7 @@ public class GearmanProxy { * @param port the host port * */ - public void init_worker(String host, int port) { + public void init_worker() { /* * Purpose here is to create a 1:1 mapping of 'gearman worker':'jenkins @@ -75,10 +75,7 @@ public class GearmanProxy { * Spawn management executor worker. This worker does not need any * executors. It only needs to work with gearman. */ - ManagementWorkerThread gwt = new ManagementWorkerThread(host, port, host); - gwt.registerJobs(); - gwt.start(); - gmwtHandles.add(gwt); + createManagementWorker(); /* * Spawn executors for the jenkins master Need to treat the master @@ -99,15 +96,7 @@ public class GearmanProxy { if (masterNode != null) { Computer computer = masterNode.toComputer(); if (computer != null) { - int executors = computer.getExecutors().size(); - for (int i = 0; i < executors; i++) { - // create a gearman worker for every executor on the master - ExecutorWorkerThread ewt = new ExecutorWorkerThread(host, port, "master-exec" - + Integer.toString(i), masterNode); - ewt.registerJobs(); - ewt.start(); - gewtHandles.add(ewt); - } + createExecutorWorkersOnNode(computer); } } @@ -120,15 +109,7 @@ public class GearmanProxy { Computer computer = node.toComputer(); if (computer != null) { // create a gearman worker for every executor on the slave - int slaveExecutors = computer.getExecutors().size(); - for (int i = 0; i < slaveExecutors; i++) { - ExecutorWorkerThread ewt = new ExecutorWorkerThread(host, port, - node.getNodeName() + "-exec" - + Integer.toString(i), node); - ewt.registerJobs(); - ewt.start(); - gewtHandles.add(ewt); - } + createExecutorWorkersOnNode(computer); } } } @@ -137,6 +118,44 @@ public class GearmanProxy { logger.info("---- Num of executors running = " + getNumExecutors()); } + /* + * Spawn management executor worker. This worker does not need any + * executors. It only needs to work with gearman. + */ + public static void createManagementWorker() { + + ManagementWorkerThread gwt = new ManagementWorkerThread( + GearmanPluginConfig.get().getHost(), + GearmanPluginConfig.get().getPort(), + "master-manager"); + gwt.registerJobs(); + gwt.start(); + gmwtHandles.add(gwt); + + } + + /* + * Spawn workers for each executor on a node. + */ + public static void createExecutorWorkersOnNode(Computer computer) { + + Node node = computer.getNode(); + + int executors = computer.getExecutors().size(); + for (int i = 0; i < executors; i++) { + ExecutorWorkerThread ewt; + + ewt = new ExecutorWorkerThread(GearmanPluginConfig.get().getHost(), + GearmanPluginConfig.get().getPort(), + GearmanPluginUtil.getRealName(node)+"-exec-"+Integer.toString(i), + node); + + ewt.registerJobs(); + ewt.start(); + gewtHandles.add(ewt); + + } + } /* * This method stops all gearman workers diff --git a/src/main/java/hudson/plugins/gearman/ManagementWorkerThread.java b/src/main/java/hudson/plugins/gearman/ManagementWorkerThread.java index 6e28012..1c576bb 100644 --- a/src/main/java/hudson/plugins/gearman/ManagementWorkerThread.java +++ b/src/main/java/hudson/plugins/gearman/ManagementWorkerThread.java @@ -31,8 +31,6 @@ import org.slf4j.LoggerFactory; public class ManagementWorkerThread extends AbstractWorkerThread{ -// private static final Logger logger = LoggerFactory -// .getLogger(AbstractWorkerThread.class); private static final Logger logger = LoggerFactory .getLogger(Constants.PLUGIN_LOGGER_NAME); @@ -50,11 +48,9 @@ public class ManagementWorkerThread extends AbstractWorkerThread{ @Override public void registerJobs(){ String jobFunctionName = "stop:"+host; - logger.info("Registering job "+jobFunctionName+" on "+host); + logger.info("---- Registering job "+jobFunctionName+" on "+host); worker.registerFunctionFactory(new DefaultGearmanFunctionFactory(jobFunctionName, StopJobWorker.class.getName())); - - } } diff --git a/src/main/java/hudson/plugins/gearman/StartJobWorker.java b/src/main/java/hudson/plugins/gearman/StartJobWorker.java index d4a0639..b483c20 100644 --- a/src/main/java/hudson/plugins/gearman/StartJobWorker.java +++ b/src/main/java/hudson/plugins/gearman/StartJobWorker.java @@ -111,12 +111,7 @@ public class StartJobWorker extends AbstractGearmanFunction { /* * make this node build this project with unique id and build params from the client */ - - // set the name of the node to execute build on - String runNodeName = node.getNodeName(); - if (runNodeName.isEmpty()) { // master node name is "" - runNodeName = "master"; - } + String runNodeName = GearmanPluginUtil.getRealName(node); // create action to run on a specified node Action runNode = new NodeAssignmentAction(runNodeName); diff --git a/src/test/java/hudson/plugins/gearman/GearmanPluginUtilTest.java b/src/test/java/hudson/plugins/gearman/GearmanPluginUtilTest.java new file mode 100644 index 0000000..2445100 --- /dev/null +++ b/src/test/java/hudson/plugins/gearman/GearmanPluginUtilTest.java @@ -0,0 +1,75 @@ +/* + * + * Copyright 2013 Hewlett-Packard Development Company, L.P. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package hudson.plugins.gearman; + +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import hudson.model.Computer; +import hudson.slaves.DumbSlave; + +import org.gearman.common.GearmanNIOJobServerConnection; +import org.gearman.worker.GearmanWorker; +import org.gearman.worker.GearmanWorkerImpl; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.jvnet.hudson.test.HudsonTestCase; +import org.powermock.core.classloader.annotations.PrepareForTest; + +/** + * Test for the {@link GearmanPluginUtil} class. + * + * @author Khai Do + */ +@PrepareForTest(GearmanWorkerImpl.class) +public class GearmanPluginUtilTest extends HudsonTestCase { + + /** + */ + @Before + public void setUpTest() { + GearmanWorker gearmanWorker = mock(GearmanWorker.class); + GearmanNIOJobServerConnection conn = new GearmanNIOJobServerConnection("localhost", 4730); + doNothing().when(gearmanWorker).work(); + when(gearmanWorker.addServer(conn)).thenReturn(true); + } + + @After + public void tearDownTest() throws Exception { + } + + @Test + public void testGetRealNameSlave() throws Exception { + + DumbSlave slave = createSlave(); + slave.setNodeName("oneiric-10"); + + hudson.removeNode(slave); + + assertEquals("oneiric-10", GearmanPluginUtil.getRealName(slave)); + } + + @Test + public void testGetRealNameMaster() throws Exception { + + assertEquals("master", GearmanPluginUtil.getRealName(Computer.currentComputer().getNode())); + } + +} diff --git a/src/test/java/hudson/plugins/gearman/ManagementWorkerThreadTest.java b/src/test/java/hudson/plugins/gearman/ManagementWorkerThreadTest.java index 7faa35e..463ffad 100644 --- a/src/test/java/hudson/plugins/gearman/ManagementWorkerThreadTest.java +++ b/src/test/java/hudson/plugins/gearman/ManagementWorkerThreadTest.java @@ -56,9 +56,16 @@ public class ManagementWorkerThreadTest { @Test public void testRegisterJobs() { - AbstractWorkerThread manager = new ManagementWorkerThread("GearmanServer", 4730, "MyWorker" ); + AbstractWorkerThread manager = new ManagementWorkerThread("GearmanServer", 4730, "manager"); manager.registerJobs(); Set functions = manager.worker.getRegisteredFunctions(); assertEquals("stop:GearmanServer", functions.toArray()[0]); } + + @Test + public void testManagerId() { + AbstractWorkerThread manager = new ManagementWorkerThread("GearmanServer", 4730, "manager"); + assertEquals("manager", manager.getName()); + } + }