diff options
3 files changed, 7 insertions, 94 deletions
diff --git a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java index 38d2bbe6fc0..3960c90af59 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -193,7 +193,7 @@ public class Flags { TENANT_ID, ZONE_ID); public static final UnboundBooleanFlag CACHE_ACL = defineFeatureFlag( - "cache-acl", true, + "cache-acl", false, List.of("hakon"), "2021-04-26", "2021-05-26", "Whether host-admin should cache the ACL responses w/TTL 115s, or always ask config server.", "Takes effect on next host-admin tick.", 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) { |