Code review fixes

3 files changed, 16 insertions, 10 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java
index fad87492f83..4748670a4be 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java
@@ -74,7 +74,9 @@ public class NodeAdminImpl implements NodeAdmin {
             }
         }, 0, 30, TimeUnit.SECONDS);
 
-        aclMaintainer.ifPresent(maintainer -> aclScheduler.scheduleAtFixedRate(maintainer, 30, 60, TimeUnit.SECONDS));
+        aclMaintainer.ifPresent(maintainer -> aclScheduler.scheduleAtFixedRate(() -> {
+            if (!isFrozen()) maintainer.run();
+        }, 30, 60, TimeUnit.SECONDS));
     }
 
     @Override
@@ -102,11 +104,16 @@ public class NodeAdminImpl implements NodeAdmin {
     }
 
     @Override
-    public boolean setFrozen(boolean frozen) {
+    public boolean setFrozen(boolean wantFrozen) {
+        // Use filter with count instead of allMatch() because allMatch() will short curcuit on first non-match
         boolean allNodeAgentsConverged = nodeAgents.values().stream()
-                .filter(nodeAgent -> !nodeAgent.setFrozen(frozen))
+                .filter(nodeAgent -> !nodeAgent.setFrozen(wantFrozen))
                 .count() == 0;
-        if (allNodeAgentsConverged) isFrozen = frozen;
+
+        if (wantFrozen) {
+            if (allNodeAgentsConverged) isFrozen = true;
+        } else isFrozen = false;
+
         return allNodeAgentsConverged;
     }
 
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java
index 201df8c5db2..d581a6fadc6 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java
@@ -130,7 +130,7 @@ public class NodeAdminStateUpdater extends AbstractComponent {
     }
 
     /**
-     * This method is attempts to converge NodeAgent's and NodeAdmin's frozen state with their orchestrator
+     * This method attempts to converge NodeAgent's and NodeAdmin's frozen state with their orchestrator
      * state. When trying to suspend node-admin, this method will first attempt to freeze all NodeAgents and
      * NodeAdmin, then asking orchestrator for permission to suspend node-admin app, and finally asking orchestrator
      * for permission to suspend all active nodes on this host, if either of the request is denied,
@@ -139,8 +139,7 @@ public class NodeAdminStateUpdater extends AbstractComponent {
     private void convergeState(State wantedState) {
         boolean wantFrozen = wantedState != RESUMED;
         if (!nodeAdmin.setFrozen(wantFrozen)) {
-            throw new RuntimeException("Wanted NodeAdmin to be " + (wantFrozen ? "frozen" : "unfrozen") +
-                    " but instead is " + (!wantFrozen ? "frozen" : "unfrozen"));
+            throw new RuntimeException("NodeAdmin has not yet converged to " + (wantFrozen ? "frozen" : "unfrozen"));
         }
 
         // To get to resumed state, we only need to converge NodeAdmins frozen state
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java
index 90a6a04c5a9..3594a16b182 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java
@@ -136,7 +136,7 @@ public class NodeAdminImplTest {
         nodeAdmin.synchronizeNodeSpecsToNodeAgents(nodeSpecs, existingContainers);
 
         mockNodeAgentSetFrozenResponse(nodeAgents, false, false, false);
-        assertFalse(nodeAdmin.setFrozen(true));
+        assertFalse(nodeAdmin.setFrozen(true)); // NodeAdmin freezes only when all the NodeAgents are frozen
 
         mockNodeAgentSetFrozenResponse(nodeAgents, false, true, true);
         assertFalse(nodeAdmin.setFrozen(true));
@@ -152,11 +152,11 @@ public class NodeAdminImplTest {
 
         mockNodeAgentSetFrozenResponse(nodeAgents, false, false, false);
         assertFalse(nodeAdmin.setFrozen(false));
-        assertTrue(nodeAdmin.isFrozen());
+        assertFalse(nodeAdmin.isFrozen()); // NodeAdmin unfreezes instantly
 
         mockNodeAgentSetFrozenResponse(nodeAgents, false, false, true);
         assertFalse(nodeAdmin.setFrozen(false));
-        assertTrue(nodeAdmin.isFrozen());
+        assertFalse(nodeAdmin.isFrozen());
 
         mockNodeAgentSetFrozenResponse(nodeAgents, true, true, true);
         assertTrue(nodeAdmin.setFrozen(false));