Check if monitor thread has run before retrieving work

gocd · Oct 18, 2018 · e95e88b · e95e88b
1 parent cb97c16
commit e95e88b
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 12 deletions.
diff --git a/agent/src/main/java/com/thoughtworks/go/agent/AgentControllerFactory.java b/agent/src/main/java/com/thoughtworks/go/agent/AgentControllerFactory.java
@@ -26,6 +26,7 @@
 import com.thoughtworks.go.plugin.access.scm.SCMExtension;
 import com.thoughtworks.go.plugin.infra.PluginManager;
 import com.thoughtworks.go.plugin.infra.PluginRequestProcessorRegistry;
+import com.thoughtworks.go.plugin.infra.monitor.PluginJarLocationMonitor;
 import com.thoughtworks.go.publishers.GoArtifactsManipulator;
 import com.thoughtworks.go.remote.BuildRepositoryRemote;
 import com.thoughtworks.go.util.HttpService;
@@ -56,6 +57,7 @@ public class AgentControllerFactory {
     private final WebSocketSessionHandler sessionHandler;
     private static final Logger LOG = LoggerFactory.getLogger(AgentControllerFactory.class);
     private final AgentHealthHolder agentHealthHolder;
+    private final PluginJarLocationMonitor pluginJarLocationMonitor;
 
     @Autowired
     public AgentControllerFactory(
@@ -75,7 +77,8 @@ public AgentControllerFactory(
             HttpService httpService,
             WebSocketClientHandler webSocketClientHandler,
             WebSocketSessionHandler sessionHandler,
-            AgentHealthHolder agentHealthHolder) {
+            AgentHealthHolder agentHealthHolder,
+            PluginJarLocationMonitor pluginJarLocationMonitor) {
         this.server = server;
         this.manipulator = manipulator;
         this.pluginManager = pluginManager;
@@ -93,6 +96,7 @@ public AgentControllerFactory(
         this.webSocketClientHandler = webSocketClientHandler;
         this.sessionHandler = sessionHandler;
         this.agentHealthHolder = agentHealthHolder;
+        this.pluginJarLocationMonitor = pluginJarLocationMonitor;
     }
 
     public AgentController createInstance() {
@@ -132,7 +136,8 @@ public AgentController createInstance() {
                     taskExtension,
                     artifactExtension,
                     pluginRequestProcessorRegistry,
-                    agentHealthHolder);
+                    agentHealthHolder,
+                    pluginJarLocationMonitor);
         }
     }
 }
diff --git a/agent/src/main/java/com/thoughtworks/go/agent/AgentHTTPClientController.java b/agent/src/main/java/com/thoughtworks/go/agent/AgentHTTPClientController.java
@@ -27,6 +27,7 @@
 import com.thoughtworks.go.plugin.access.scm.SCMExtension;
 import com.thoughtworks.go.plugin.infra.PluginManager;
 import com.thoughtworks.go.plugin.infra.PluginRequestProcessorRegistry;
+import com.thoughtworks.go.plugin.infra.monitor.PluginJarLocationMonitor;
 import com.thoughtworks.go.publishers.GoArtifactsManipulator;
 import com.thoughtworks.go.remote.AgentIdentifier;
 import com.thoughtworks.go.remote.AgentInstruction;
@@ -47,6 +48,7 @@ public class AgentHTTPClientController extends AgentController {
     private SslInfrastructureService sslInfrastructureService;
     private final ArtifactExtension artifactExtension;
     private final PluginRequestProcessorRegistry pluginRequestProcessorRegistry;
+    private final PluginJarLocationMonitor pluginJarLocationMonitor;
 
     private JobRunner runner;
     private PackageRepositoryExtension packageRepositoryExtension;
@@ -65,7 +67,10 @@ public AgentHTTPClientController(BuildRepositoryRemote server,
                                      PackageRepositoryExtension packageRepositoryExtension,
                                      SCMExtension scmExtension,
                                      TaskExtension taskExtension,
-                                     ArtifactExtension artifactExtension, PluginRequestProcessorRegistry pluginRequestProcessorRegistry, AgentHealthHolder agentHealthHolder) {
+                                     ArtifactExtension artifactExtension,
+                                     PluginRequestProcessorRegistry pluginRequestProcessorRegistry,
+                                     AgentHealthHolder agentHealthHolder,
+                                     PluginJarLocationMonitor pluginJarLocationMonitor) {
         super(sslInfrastructureService, systemEnvironment, agentRegistry, pluginManager, subprocessLogger, agentUpgradeService, agentHealthHolder);
         this.packageRepositoryExtension = packageRepositoryExtension;
         this.scmExtension = scmExtension;
@@ -75,6 +80,7 @@ public AgentHTTPClientController(BuildRepositoryRemote server,
         this.sslInfrastructureService = sslInfrastructureService;
         this.artifactExtension = artifactExtension;
         this.pluginRequestProcessorRegistry = pluginRequestProcessorRegistry;
+        this.pluginJarLocationMonitor = pluginJarLocationMonitor;
     }
 
     @Override
@@ -105,9 +111,13 @@ public void execute() {
     @Override
     public void work() {
         LOG.debug("[Agent Loop] Trying to retrieve work.");
-        retrieveCookieIfNecessary();
-        retrieveWork();
-        LOG.debug("[Agent Loop] Successfully retrieved work.");
+        if (pluginJarLocationMonitor.getLastRun() > 0) {
+            retrieveCookieIfNecessary();
+            retrieveWork();
+            LOG.debug("[Agent Loop] Successfully retrieved work.");
+        } else {
+            LOG.debug("[Agent Loop] PluginLocationMonitor has not yet run. Not retrieving work since plugins may not be initialized.");
+        }
     }
 
     private void retrieveCookieIfNecessary() {

diff --git a/agent/src/test/java/com/thoughtworks/go/agent/AgentHTTPClientControllerTest.java b/agent/src/test/java/com/thoughtworks/go/agent/AgentHTTPClientControllerTest.java
@@ -26,6 +26,7 @@
 import com.thoughtworks.go.plugin.access.scm.SCMExtension;
 import com.thoughtworks.go.plugin.infra.PluginManager;
 import com.thoughtworks.go.plugin.infra.PluginManagerReference;
+import com.thoughtworks.go.plugin.infra.monitor.PluginJarLocationMonitor;
 import com.thoughtworks.go.publishers.GoArtifactsManipulator;
 import com.thoughtworks.go.remote.AgentIdentifier;
 import com.thoughtworks.go.remote.BuildRepositoryRemote;
@@ -44,6 +45,8 @@
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnitRunner;
 
+import java.io.IOException;
+
 import static com.thoughtworks.go.util.SystemUtil.getFirstLocalNonLoopbackIpAddress;
 import static com.thoughtworks.go.util.SystemUtil.getLocalhostName;
 import static org.hamcrest.Matchers.is;
@@ -80,6 +83,8 @@ public class AgentHTTPClientControllerTest {
     private TaskExtension taskExtension;
     @Mock
     private ArtifactExtension artifactExtension;
+    @Mock
+    private PluginJarLocationMonitor pluginJarLocationMonitor;
     private String agentUuid = "uuid";
     private AgentIdentifier agentIdentifier;
     private AgentHTTPClientController agentController;
@@ -104,14 +109,26 @@ public void shouldSetPluginManagerReference() throws Exception {
     public void shouldRetrieveWorkFromServerAndDoIt() throws Exception {
         when(loopServer.getWork(any(AgentRuntimeInfo.class))).thenReturn(work);
         when(agentRegistry.uuid()).thenReturn(agentUuid);
+        when(pluginJarLocationMonitor.getLastRun()).thenReturn(System.currentTimeMillis());
         agentController = createAgentController();
         agentController.init();
         agentController.ping();
-        agentController.retrieveWork();
+        agentController.work();
         verify(work).doWork(any(EnvironmentVariableContext.class), any(AgentWorkContext.class));
         verify(sslInfrastructureService).createSslInfrastructure();
     }
 
+    @Test
+    public void shouldNotRetrieveWorkIfPluginMonitorHasNotRun() throws IOException {
+        when(agentRegistry.uuid()).thenReturn(agentUuid);
+        when(pluginJarLocationMonitor.getLastRun()).thenReturn(0L);
+        agentController = createAgentController();
+        agentController.init();
+        agentController.ping();
+        agentController.work();
+        verifyZeroInteractions(work);
+    }
+
     @Test
     public void shouldRetrieveCookieIfNotPresent() throws Exception {
         agentController = createAgentController();
@@ -121,6 +138,7 @@ public void shouldRetrieveCookieIfNotPresent() throws Exception {
         when(sslInfrastructureService.isRegistered()).thenReturn(true);
         when(loopServer.getWork(agentController.getAgentRuntimeInfo())).thenReturn(work);
         when(agentRegistry.uuid()).thenReturn(agentUuid);
+        when(pluginJarLocationMonitor.getLastRun()).thenReturn(System.currentTimeMillis());
         agentController.loop();
         verify(work).doWork(any(EnvironmentVariableContext.class), any(AgentWorkContext.class));
     }
@@ -140,7 +158,9 @@ public void shouldNotTellServerWorkIsCompletedWhenThereIsNoWork() throws Excepti
     public void shouldRegisterSubprocessLoggerAtExit() throws Exception {
         SslInfrastructureService sslInfrastructureService = mock(SslInfrastructureService.class);
         AgentRegistry agentRegistry = mock(AgentRegistry.class);
-        agentController = new AgentHTTPClientController(loopServer, artifactsManipulator, sslInfrastructureService, agentRegistry, agentUpgradeService, subprocessLogger, systemEnvironment, pluginManager, packageRepositoryExtension, scmExtension, taskExtension, artifactExtension, null, null);
+        agentController = new AgentHTTPClientController(loopServer, artifactsManipulator, sslInfrastructureService,
+                agentRegistry, agentUpgradeService, subprocessLogger, systemEnvironment, pluginManager,
+                packageRepositoryExtension, scmExtension, taskExtension, artifactExtension, null, null, pluginJarLocationMonitor);
         agentController.init();
         verify(subprocessLogger).registerAsExitHook("Following processes were alive at shutdown: ");
     }
@@ -181,6 +201,6 @@ private AgentHTTPClientController createAgentController() {
                 packageRepositoryExtension,
                 scmExtension,
                 taskExtension,
-                artifactExtension, null, null);
+                artifactExtension, null, null, pluginJarLocationMonitor);
     }
 }
diff --git a/...c/main/java/com/thoughtworks/go/plugin/infra/monitor/DefaultPluginJarLocationMonitor.java b/...c/main/java/com/thoughtworks/go/plugin/infra/monitor/DefaultPluginJarLocationMonitor.java
@@ -80,7 +80,8 @@ public void start() {
         monitorThread.start();
     }
 
-    long getLastRun() {
+    @Override
+    public long getLastRun() {
         if (monitorThread == null) {
             return 0;
         }
@@ -186,8 +187,6 @@ public void run() {
 
         //Added synchronized because the compiler can change the order of instructions, meaning that the lastRun can be
         //updated before the listeners are notified.
-        //lastRun is needed only for tests. It may be a bit excessive to synchronize methods just for tests. So this can
-        //definitely be improved in the future
         public synchronized void oneShot() {
             knownBundledPluginFileDetails = loadAndNotifyPluginsFrom(bundledPluginDirectory, knownBundledPluginFileDetails, true);
             knownExternalPluginFileDetails = loadAndNotifyPluginsFrom(externalPluginDirectory, knownExternalPluginFileDetails, false);

diff --git a/...nfra/src/main/java/com/thoughtworks/go/plugin/infra/monitor/PluginJarLocationMonitor.java b/...nfra/src/main/java/com/thoughtworks/go/plugin/infra/monitor/PluginJarLocationMonitor.java
@@ -27,4 +27,6 @@ public interface PluginJarLocationMonitor {
     void stop();
 
     void oneShot();
+
+    long getLastRun();
 }