Only support cache-acl flag is false code branch

2 files changed, 6 insertions, 93 deletions

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 772e17291ef..58ca4ae3f41 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
@@ -3,9 +3,7 @@ package com.yahoo.vespa.hosted.node.admin.nodeadmin;
 
 import com.yahoo.concurrent.ThreadFactoryFactory;
 import com.yahoo.config.provision.HostName;
-import com.yahoo.vespa.flags.BooleanFlag;
 import com.yahoo.vespa.flags.FlagSource;
-import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.Acl;
 import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeRepository;
 import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec;
@@ -17,18 +15,14 @@ import com.yahoo.yolean.Exceptions;
 
 import java.time.Clock;
 import java.time.Duration;
-import java.time.Instant;
 import java.util.ArrayList;
 import java.util.EnumSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
-import java.util.function.Function;
-import java.util.function.Supplier;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
@@ -50,13 +44,11 @@ public class NodeAdminStateUpdater {
     private final ScheduledExecutorService metricsScheduler =
             Executors.newScheduledThreadPool(1, ThreadFactoryFactory.getDaemonThreadFactory("metricsscheduler"));
 
-    private final CachedSupplier<Map<String, Acl>> cachedAclSupplier;
     private final NodeAgentContextFactory nodeAgentContextFactory;
     private final NodeRepository nodeRepository;
     private final Orchestrator orchestrator;
     private final NodeAdmin nodeAdmin;
     private final String hostHostname;
-    private final BooleanFlag cacheAclFlag;
 
     public enum State { TRANSITIONING, RESUMED, SUSPENDED_NODE_ADMIN, SUSPENDED }
 
@@ -75,8 +67,6 @@ public class NodeAdminStateUpdater {
         this.orchestrator = orchestrator;
         this.nodeAdmin = nodeAdmin;
         this.hostHostname = hostHostname.value();
-        this.cachedAclSupplier = new CachedSupplier<>(clock, Duration.ofSeconds(115), () -> nodeRepository.getAcls(this.hostHostname));
-        this.cacheAclFlag = Flags.CACHE_ACL.bindTo(flagSource);
     }
 
     public void start() {
@@ -172,18 +162,10 @@ public class NodeAdminStateUpdater {
 
     void adjustNodeAgentsToRunFromNodeRepository() {
         try {
-            Map<String, NodeSpec> nodeSpecByHostname = nodeRepository.getNodes(hostHostname).stream()
-                    .collect(Collectors.toMap(NodeSpec::hostname, Function.identity()));
-            Map<String, Acl> aclByHostname = cacheAclFlag.value() ?
-                    Optional.of(cachedAclSupplier.get())
-                            .filter(acls -> acls.keySet().containsAll(nodeSpecByHostname.keySet()))
-                            .orElseGet(cachedAclSupplier::invalidateAndGet) :
-                    cachedAclSupplier.invalidateAndGet();
-
-            Set<NodeAgentContext> nodeAgentContexts = nodeSpecByHostname.keySet().stream()
-                    .map(hostname -> nodeAgentContextFactory.create(
-                            nodeSpecByHostname.get(hostname),
-                            aclByHostname.getOrDefault(hostname, Acl.EMPTY)))
+            Map<String, Acl> aclByHostname = nodeRepository.getAcls(hostHostname);
+
+            Set<NodeAgentContext> nodeAgentContexts = nodeRepository.getNodes(hostHostname).stream()
+                    .map(node -> nodeAgentContextFactory.create(node, aclByHostname.getOrDefault(node.hostname(), Acl.EMPTY)))
                     .collect(Collectors.toSet());
             nodeAdmin.refreshContainersToRun(nodeAgentContexts);
         } catch (ConvergenceException e) {
@@ -200,33 +182,4 @@ public class NodeAdminStateUpdater {
                              .map(NodeSpec::hostname)
                              .collect(Collectors.toList());
     }
-
-    private static class CachedSupplier<T> implements Supplier<T> {
-        private final Clock clock;
-        private final Duration expiration;
-        private final Supplier<T> supplier;
-        private Instant refreshAt;
-        private T cachedValue;
-
-        private CachedSupplier(Clock clock, Duration expiration, Supplier<T> supplier) {
-            this.clock = clock;
-            this.expiration = expiration;
-            this.supplier = supplier;
-            this.refreshAt = Instant.MIN;
-        }
-
-        @Override
-        public T get() {
-            if (! clock.instant().isBefore(refreshAt)) {
-                cachedValue = supplier.get();
-                refreshAt = clock.instant().plus(expiration);
-            }
-            return cachedValue;
-        }
-
-        private T invalidateAndGet() {
-            refreshAt = Instant.MIN;
-            return get();
-        }
-    }
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdaterTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdaterTest.java
index d348d2b74e9..4a678597e41 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdaterTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdaterTest.java
@@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.node.admin.nodeadmin;
 
 import com.yahoo.config.provision.HostName;
 import com.yahoo.test.ManualClock;
-import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.Acl;
 import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeRepository;
@@ -179,26 +178,6 @@ public class NodeAdminStateUpdaterTest {
     }
 
     @Test
-    public void uses_cached_acl() {
-        mockNodeRepo(NodeState.active, 1);
-        mockAcl(Acl.EMPTY, 1);
-
-        updater.adjustNodeAgentsToRunFromNodeRepository();
-        verify(nodeRepository, times(1)).getAcls(any());
-        clock.advance(Duration.ofSeconds(30));
-
-        updater.adjustNodeAgentsToRunFromNodeRepository();
-        clock.advance(Duration.ofSeconds(30));
-        updater.adjustNodeAgentsToRunFromNodeRepository();
-        clock.advance(Duration.ofSeconds(30));
-        verify(nodeRepository, times(1)).getAcls(any());
-
-        clock.advance(Duration.ofSeconds(30));
-        updater.adjustNodeAgentsToRunFromNodeRepository();
-        verify(nodeRepository, times(2)).getAcls(any());
-    }
-
-    @Test
     public void node_spec_and_acl_aligned() {
         Acl acl = new Acl.Builder().withTrustedPorts(22).build();
         mockNodeRepo(NodeState.active, 3);
@@ -212,25 +191,6 @@ public class NodeAdminStateUpdaterTest {
         verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.hostname().equals("host2.yahoo.com")), eq(acl));
         verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.hostname().equals("host3.yahoo.com")), eq(acl));
         verify(nodeRepository, times(3)).getNodes(eq(hostHostname.value()));
-        verify(nodeRepository, times(1)).getAcls(eq(hostHostname.value()));
-    }
-
-    @Test
-    public void node_spec_and_acl_aligned_with_acl_cache_disabled() {
-        flagSource.withBooleanFlag(Flags.CACHE_ACL.id(), false);
-
-        Acl acl = new Acl.Builder().withTrustedPorts(22).build();
-        mockNodeRepo(NodeState.active, 3);
-        mockAcl(acl, 1, 2, 3);
-
-        updater.adjustNodeAgentsToRunFromNodeRepository();
-        updater.adjustNodeAgentsToRunFromNodeRepository();
-        updater.adjustNodeAgentsToRunFromNodeRepository();
-
-        verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.hostname().equals("host1.yahoo.com")), eq(acl));
-        verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.hostname().equals("host2.yahoo.com")), eq(acl));
-        verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.hostname().equals("host3.yahoo.com")), eq(acl));
-        verify(nodeRepository, times(3)).getNodes(eq(hostHostname.value()));
         verify(nodeRepository, times(3)).getAcls(eq(hostHostname.value()));
     }
 
@@ -249,7 +209,7 @@ public class NodeAdminStateUpdaterTest {
         verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.hostname().equals("host2.yahoo.com")), eq(acl));
         verify(nodeAgentContextFactory, times(1)).create(argThat(spec -> spec.hostname().equals("host3.yahoo.com")), eq(Acl.EMPTY));
         verify(nodeRepository, times(3)).getNodes(eq(hostHostname.value()));
-        verify(nodeRepository, times(2)).getAcls(eq(hostHostname.value())); // During the first tick, the cache is invalidated and retried
+        verify(nodeRepository, times(3)).getAcls(eq(hostHostname.value()));
     }
 
     @Test
@@ -265,7 +225,7 @@ public class NodeAdminStateUpdaterTest {
         verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.hostname().equals("host1.yahoo.com")), eq(acl));
         verify(nodeAgentContextFactory, times(3)).create(argThat(spec -> spec.hostname().equals("host2.yahoo.com")), eq(acl));
         verify(nodeRepository, times(3)).getNodes(eq(hostHostname.value()));
-        verify(nodeRepository, times(1)).getAcls(eq(hostHostname.value()));
+        verify(nodeRepository, times(3)).getAcls(eq(hostHostname.value()));
     }
 
     private void assertConvergeError(NodeAdminStateUpdater.State targetState, String reason) {