From 08f512b1c9317cd09e621cad3446e97d6ff79bfb Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Fri, 17 Apr 2020 10:34:01 +0200 Subject: Reduce initial maintainer delay in controller --- .../vespa/hosted/controller/maintenance/Maintainer.java | 2 +- .../hosted/controller/maintenance/MaintainerTest.java | 17 +++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java index cec44c5d3a0..779b8dcdfcc 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java @@ -116,7 +116,7 @@ public abstract class Maintainer extends AbstractComponent implements Runnable { long offset = cluster.indexOf(host) * interval.toMillis() / cluster.size(); long timeUntilNextRun = Math.floorMod(offset - now.toEpochMilli(), interval.toMillis()); - return timeUntilNextRun + interval.toMillis() / cluster.size(); + return timeUntilNextRun; } } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MaintainerTest.java index 3aa1f2b5af2..efd5d61ce56 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MaintainerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/MaintainerTest.java @@ -38,17 +38,22 @@ public class MaintainerTest { @Test public void staggering() { List cluster = List.of(HostName.from("cfg1"), HostName.from("cfg2"), HostName.from("cfg3")); - Instant now = Instant.ofEpochMilli(1001); Duration interval = Duration.ofMillis(300); - assertEquals(299, Maintainer.staggeredDelay(cluster, HostName.from("cfg1"), now, interval)); - assertEquals(399, Maintainer.staggeredDelay(cluster, HostName.from("cfg2"), now, interval)); - assertEquals(199, Maintainer.staggeredDelay(cluster, HostName.from("cfg3"), now, interval)); + Instant now = Instant.ofEpochMilli(1000); + assertEquals(200, Maintainer.staggeredDelay(cluster, HostName.from("cfg1"), now, interval)); + assertEquals( 0, Maintainer.staggeredDelay(cluster, HostName.from("cfg2"), now, interval)); + assertEquals(100, Maintainer.staggeredDelay(cluster, HostName.from("cfg3"), now, interval)); - now = Instant.ofEpochMilli(1101); + now = Instant.ofEpochMilli(1001); assertEquals(199, Maintainer.staggeredDelay(cluster, HostName.from("cfg1"), now, interval)); assertEquals(299, Maintainer.staggeredDelay(cluster, HostName.from("cfg2"), now, interval)); - assertEquals(399, Maintainer.staggeredDelay(cluster, HostName.from("cfg3"), now, interval)); + assertEquals( 99, Maintainer.staggeredDelay(cluster, HostName.from("cfg3"), now, interval)); + + now = Instant.ofEpochMilli(1101); + assertEquals( 99, Maintainer.staggeredDelay(cluster, HostName.from("cfg1"), now, interval)); + assertEquals(199, Maintainer.staggeredDelay(cluster, HostName.from("cfg2"), now, interval)); + assertEquals(299, Maintainer.staggeredDelay(cluster, HostName.from("cfg3"), now, interval)); assertEquals(300, Maintainer.staggeredDelay(cluster, HostName.from("cfg0"), now, interval)); } -- cgit v1.2.3 From 3a6fbf844732dd41ad2fc2ef6b597a281808868d Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Fri, 17 Apr 2020 11:15:22 +0200 Subject: Reduce initial delay of config server maintainers too --- .../hosted/provision/maintenance/Maintainer.java | 2 +- .../hosted/provision/maintenance/MaintainerTest.java | 19 +++++++++++++------ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java index e01f7ea7bf5..63e2721582a 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java @@ -114,7 +114,7 @@ public abstract class Maintainer extends AbstractComponent implements Runnable { long offset = cluster.indexOf(host) * interval.toMillis() / cluster.size(); long timeUntilNextRun = Math.floorMod(offset - now.toEpochMilli(), interval.toMillis()); - return timeUntilNextRun + interval.toMillis() / cluster.size(); + return timeUntilNextRun; } } diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MaintainerTest.java index c19b54a8cab..7ce64093491 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MaintainerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/MaintainerTest.java @@ -19,15 +19,22 @@ public class MaintainerTest { @Test public void staggering() { List cluster = Arrays.asList(HostName.from("cfg1"), HostName.from("cfg2"), HostName.from("cfg3")); - Instant now = Instant.ofEpochMilli(1001); Duration interval = Duration.ofMillis(300); - assertEquals(299, Maintainer.staggeredDelay(cluster, HostName.from("cfg1"), now, interval)); - assertEquals(399, Maintainer.staggeredDelay(cluster, HostName.from("cfg2"), now, interval)); - assertEquals(199, Maintainer.staggeredDelay(cluster, HostName.from("cfg3"), now, interval)); - now = Instant.ofEpochMilli(1101); + Instant now = Instant.ofEpochMilli(1000); + assertEquals(200, Maintainer.staggeredDelay(cluster, HostName.from("cfg1"), now, interval)); + assertEquals( 0, Maintainer.staggeredDelay(cluster, HostName.from("cfg2"), now, interval)); + assertEquals(100, Maintainer.staggeredDelay(cluster, HostName.from("cfg3"), now, interval)); + + now = Instant.ofEpochMilli(1001); assertEquals(199, Maintainer.staggeredDelay(cluster, HostName.from("cfg1"), now, interval)); assertEquals(299, Maintainer.staggeredDelay(cluster, HostName.from("cfg2"), now, interval)); - assertEquals(399, Maintainer.staggeredDelay(cluster, HostName.from("cfg3"), now, interval)); + assertEquals( 99, Maintainer.staggeredDelay(cluster, HostName.from("cfg3"), now, interval)); + + now = Instant.ofEpochMilli(1101); + assertEquals( 99, Maintainer.staggeredDelay(cluster, HostName.from("cfg1"), now, interval)); + assertEquals(199, Maintainer.staggeredDelay(cluster, HostName.from("cfg2"), now, interval)); + assertEquals(299, Maintainer.staggeredDelay(cluster, HostName.from("cfg3"), now, interval)); + assertEquals(300, Maintainer.staggeredDelay(cluster, HostName.from("cfg0"), now, interval)); } } -- cgit v1.2.3 From 5644d4216c1ca3c2ec48a666db915ca02dd21259 Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Fri, 17 Apr 2020 11:16:35 +0200 Subject: Used named threads in maintainers --- .../java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java | 2 +- .../java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java index 779b8dcdfcc..03348833891 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java @@ -52,7 +52,7 @@ public abstract class Maintainer extends AbstractComponent implements Runnable { this.name = name; this.activeSystems = Set.copyOf(activeSystems); - service = new ScheduledThreadPoolExecutor(1); + service = new ScheduledThreadPoolExecutor(1, r -> new Thread(r, getClass().getSimpleName() + "-worker")); long delay = staggeredDelay(controller.curator().cluster(), controller.hostname(), controller.clock().instant(), interval); service.scheduleAtFixedRate(this, delay, interval.toMillis(), TimeUnit.MILLISECONDS); jobControl.started(name()); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java index 63e2721582a..2a87dd1d55a 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java @@ -42,7 +42,7 @@ public abstract class Maintainer extends AbstractComponent implements Runnable { HostName hostname = HostName.from(com.yahoo.net.HostName.getLocalhost()); long delay = staggeredDelay(nodeRepository.database().cluster(), hostname, nodeRepository.clock().instant(), interval); - service = new ScheduledThreadPoolExecutor(1); + service = new ScheduledThreadPoolExecutor(1, r -> new Thread(r, getClass().getSimpleName() + "-worker")); service.scheduleAtFixedRate(this, delay, interval.toMillis(), TimeUnit.MILLISECONDS); jobControl.started(name(), this); } -- cgit v1.2.3 From 1b536d61af0be52103fdb83e0fb6f2b6947b57c7 Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Fri, 17 Apr 2020 12:38:04 +0200 Subject: Inline variables --- .../java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java | 3 +-- .../java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java index 03348833891..252df554e22 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java @@ -115,8 +115,7 @@ public abstract class Maintainer extends AbstractComponent implements Runnable { return interval.toMillis(); long offset = cluster.indexOf(host) * interval.toMillis() / cluster.size(); - long timeUntilNextRun = Math.floorMod(offset - now.toEpochMilli(), interval.toMillis()); - return timeUntilNextRun; + return Math.floorMod(offset - now.toEpochMilli(), interval.toMillis()); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java index 2a87dd1d55a..1608bf7511a 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java @@ -113,8 +113,7 @@ public abstract class Maintainer extends AbstractComponent implements Runnable { return interval.toMillis(); long offset = cluster.indexOf(host) * interval.toMillis() / cluster.size(); - long timeUntilNextRun = Math.floorMod(offset - now.toEpochMilli(), interval.toMillis()); - return timeUntilNextRun; + return Math.floorMod(offset - now.toEpochMilli(), interval.toMillis()); } } -- cgit v1.2.3 From 564ea2ed446154746bfb23a29aa8f1a414de3c9b Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Fri, 17 Apr 2020 12:38:33 +0200 Subject: Expand ContactInformationMaintainer unit test to verify exception handling --- .../organization/MockContactRetriever.java | 11 ++++++--- .../ContactInformationMaintainerTest.java | 26 +++++++++++----------- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/MockContactRetriever.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/MockContactRetriever.java index dd0c143ae28..ab39d8e5660 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/MockContactRetriever.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/organization/MockContactRetriever.java @@ -8,23 +8,28 @@ import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.Optional; +import java.util.function.Supplier; /** * @author olaa */ public class MockContactRetriever implements ContactRetriever{ - private final Map contacts = new HashMap<>(); + private final Map> contacts = new HashMap<>(); @Override public Contact getContact(Optional propertyId) { - return contacts.getOrDefault(propertyId.get(), contact()); + return contacts.getOrDefault(propertyId.get(), this::contact).get(); } - public void addContact(PropertyId propertyId, Contact contact) { + public void addContact(PropertyId propertyId, Supplier contact) { contacts.put(propertyId, contact); } + public void addContact(PropertyId propertyId, Contact contact) { + contacts.put(propertyId, () -> contact); + } + public Contact contact() { return new Contact(URI.create("contacts.tld"), URI.create("properties.tld"), URI.create("issues.tld"), Collections.emptyList(), "queue", Optional.of("component")); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ContactInformationMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ContactInformationMaintainerTest.java index 0db718080c2..32d2e2f35f4 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ContactInformationMaintainerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ContactInformationMaintainerTest.java @@ -37,22 +37,22 @@ public class ContactInformationMaintainerTest { @Test public void updates_contact_information() { - long propertyId = 1; - TenantName name = tester.createTenant("tenant1", "domain1", propertyId); - Supplier tenant = () -> (AthenzTenant) tester.controller().tenants().require(name); - assertFalse("No contact information initially", tenant.get().contact().isPresent()); + PropertyId propertyId1 = new PropertyId("1"); + PropertyId propertyId2 = new PropertyId("2"); + TenantName name1 = tester.createTenant("tenant1", "domain1", 1L); + TenantName name2 = tester.createTenant("zenant1", "domain2", 2L); + Supplier tenant1 = () -> (AthenzTenant) tester.controller().tenants().require(name1); + Supplier tenant2 = () -> (AthenzTenant) tester.controller().tenants().require(name2); + assertFalse("No contact information initially", tenant1.get().contact().isPresent()); + assertFalse("No contact information initially", tenant2.get().contact().isPresent()); Contact contact = testContact(); - registerContact(propertyId, contact); - maintainer.run(); + tester.serviceRegistry().contactRetriever().addContact(propertyId1, () -> { throw new RuntimeException("ERROR"); }); + tester.serviceRegistry().contactRetriever().addContact(propertyId2, () -> contact); + maintainer.maintain(); - assertTrue("Contact information added", tenant.get().contact().isPresent()); - assertEquals(contact, tenant.get().contact().get()); - } - - private void registerContact(long propertyId, Contact contact) { - PropertyId p = new PropertyId(String.valueOf(propertyId)); - tester.serviceRegistry().contactRetrieverMock().addContact(p, contact); + assertEquals("No contact information added due to error", Optional.empty(), tenant1.get().contact()); + assertEquals("Contact information added", Optional.of(contact), tenant2.get().contact()); } private static Contact testContact() { -- cgit v1.2.3 From cf3a25d6906c976fbe12b4eb686c916a383e3bd6 Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Fri, 17 Apr 2020 12:49:33 +0200 Subject: Log process during contact information updates --- .../maintenance/ContactInformationMaintainer.java | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ContactInformationMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ContactInformationMaintainer.java index 7e354a02832..6b2daeb1323 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ContactInformationMaintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ContactInformationMaintainer.java @@ -6,14 +6,18 @@ import com.yahoo.log.LogLevel; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.LockedTenant; import com.yahoo.vespa.hosted.controller.TenantController; +import com.yahoo.vespa.hosted.controller.api.integration.organization.Contact; import com.yahoo.vespa.hosted.controller.api.integration.organization.ContactRetriever; import com.yahoo.vespa.hosted.controller.tenant.Tenant; import com.yahoo.yolean.Exceptions; import java.time.Duration; +import java.util.Optional; import java.util.function.Predicate; import java.util.logging.Logger; +import static java.util.logging.Level.INFO; + /** * Periodically fetch and store contact information for tenants. * @@ -34,13 +38,21 @@ public class ContactInformationMaintainer extends Maintainer { protected void maintain() { TenantController tenants = controller().tenants(); for (Tenant tenant : tenants.asList()) { + log.log(INFO, "Updating contact information for " + tenant); try { switch (tenant.type()) { - case athenz: tenants.lockIfPresent(tenant.name(), LockedTenant.Athenz.class, lockedTenant -> - tenants.store(lockedTenant.with(contactRetriever.getContact(lockedTenant.get().propertyId())))); + case athenz: + tenants.lockIfPresent(tenant.name(), LockedTenant.Athenz.class, lockedTenant -> { + Contact contact = contactRetriever.getContact(lockedTenant.get().propertyId()); + log.log(INFO, "Contact found for " + tenant + " was " + + (Optional.of(contact).equals(tenant.contact()) ? "un" : "") + "changed"); + tenants.store(lockedTenant.with(contact)); + }); + return; + case cloud: return; - case cloud: return; - default: throw new IllegalArgumentException("Unexpected tenant type '" + tenant.type() + "'."); + default: + throw new IllegalArgumentException("Unexpected tenant type '" + tenant.type() + "'."); } } catch (Exception e) { log.log(LogLevel.WARNING, "Failed to update contact information for " + tenant + ": " + -- cgit v1.2.3 From b98bf882f2d90808349d0ae482c5694aa08e3b2c Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Fri, 17 Apr 2020 14:16:16 +0200 Subject: break, not return, from switch statement --- .../hosted/controller/maintenance/ContactInformationMaintainer.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ContactInformationMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ContactInformationMaintainer.java index 6b2daeb1323..d1f2087ce12 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ContactInformationMaintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ContactInformationMaintainer.java @@ -48,9 +48,9 @@ public class ContactInformationMaintainer extends Maintainer { (Optional.of(contact).equals(tenant.contact()) ? "un" : "") + "changed"); tenants.store(lockedTenant.with(contact)); }); - return; + break; case cloud: - return; + break; default: throw new IllegalArgumentException("Unexpected tenant type '" + tenant.type() + "'."); } -- cgit v1.2.3 From c16e37ada9df0cca4e686ee546442e02a3cd1ed9 Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Fri, 17 Apr 2020 21:21:47 +0200 Subject: Apply suggestions from code review --- .../java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java | 2 +- .../java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java index 252df554e22..3e283d6c54a 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Maintainer.java @@ -52,7 +52,7 @@ public abstract class Maintainer extends AbstractComponent implements Runnable { this.name = name; this.activeSystems = Set.copyOf(activeSystems); - service = new ScheduledThreadPoolExecutor(1, r -> new Thread(r, getClass().getSimpleName() + "-worker")); + service = new ScheduledThreadPoolExecutor(1, r -> new Thread(r, name() + "-worker")); long delay = staggeredDelay(controller.curator().cluster(), controller.hostname(), controller.clock().instant(), interval); service.scheduleAtFixedRate(this, delay, interval.toMillis(), TimeUnit.MILLISECONDS); jobControl.started(name()); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java index 1608bf7511a..17c73282b4b 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Maintainer.java @@ -42,7 +42,7 @@ public abstract class Maintainer extends AbstractComponent implements Runnable { HostName hostname = HostName.from(com.yahoo.net.HostName.getLocalhost()); long delay = staggeredDelay(nodeRepository.database().cluster(), hostname, nodeRepository.clock().instant(), interval); - service = new ScheduledThreadPoolExecutor(1, r -> new Thread(r, getClass().getSimpleName() + "-worker")); + service = new ScheduledThreadPoolExecutor(1, r -> new Thread(r, name() + "-worker")); service.scheduleAtFixedRate(this, delay, interval.toMillis(), TimeUnit.MILLISECONDS); jobControl.started(name(), this); } -- cgit v1.2.3