diff options
author | Jon Marius Venstad <jonmv@users.noreply.github.com> | 2021-05-20 19:13:35 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-05-20 19:13:35 +0200 |
commit | a64992fadc49e5b86ae88951c4bd207af63c556f (patch) | |
tree | e5885f553298ea6814b4157127bde0695d9175b8 | |
parent | 7ce8929ba45d58fa60ec833ef54dd7d0c67784b3 (diff) | |
parent | fe1a8b62d02e4fbf015aa10ff064b25f984f63d6 (diff) |
Merge pull request #17930 from vespa-engine/jonmv/revert-slobroks-only-on-ccs
Jonmv/revert slobroks only on ccs
9 files changed, 30 insertions, 91 deletions
diff --git a/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java b/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java index 2784c111019..602e0c80d4b 100644 --- a/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java +++ b/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java @@ -43,7 +43,7 @@ public abstract class AbstractConfigProducer<CHILD extends AbstractConfigProduce private final String subId; private String configId = null; - private final List<Service> descendantServices = new ArrayList<>(); + private List<Service> descendantServices = new ArrayList<>(); private AbstractConfigProducer parent = null; @@ -59,8 +59,8 @@ public abstract class AbstractConfigProducer<CHILD extends AbstractConfigProduce * Creates a new AbstractConfigProducer with the given parent and subId. * This constructor will add the resulting producer to the children of parent. * - * @param parent the parent of this ConfigProducer - * @param subId the fragment of the config id for the producer + * @param parent The parent of this ConfigProducer + * @param subId The fragment of the config id for the producer */ public AbstractConfigProducer(AbstractConfigProducer parent, String subId) { this(subId); @@ -69,13 +69,7 @@ public abstract class AbstractConfigProducer<CHILD extends AbstractConfigProduce } } - /** Removes this from the config model */ - protected void remove() { - if (parent != null) - parent.removeChild(this); - } - - protected final void setParent(AbstractConfigProducer<?> parent) { + protected final void setParent(AbstractConfigProducer parent) { this.parent = parent; computeConfigId(); } @@ -98,7 +92,7 @@ public abstract class AbstractConfigProducer<CHILD extends AbstractConfigProduce /** * Adds a child to this config producer. * - * @param child the child config producer to add + * @param child The child config producer to add. */ protected void addChild(CHILD child) { if (child == null) { diff --git a/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java b/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java index abd9623c1e9..6e16b1a120e 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java @@ -92,11 +92,10 @@ public abstract class AbstractService extends AbstractConfigProducer<AbstractCon * Preferred constructor when building from XML. Use this if you are building * in doBuild() in an AbstractConfigProducerBuilder. * build() will call initService() in that case, after setting hostalias and baseport. - * - * @param parent the parent config producer in the model tree - * @param name the name of this service + * @param parent Parent config producer in the model tree. + * @param name Name of this service. */ - public AbstractService(AbstractConfigProducer<?> parent, String name) { + public AbstractService(AbstractConfigProducer parent, String name) { super(parent, name); } @@ -108,13 +107,6 @@ public abstract class AbstractService extends AbstractConfigProducer<AbstractCon super(name); } - @Override - public void remove() { - super.remove(); - if (hostResource != null) - hostResource.deallocateService(this); - } - /** * Distribute affinity on a collection of services. Services that are located on the same host * will be assigned a specific cpu socket on that host. diff --git a/config-model/src/main/java/com/yahoo/vespa/model/HostPorts.java b/config-model/src/main/java/com/yahoo/vespa/model/HostPorts.java index 38b668f121e..a80982fe75b 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/HostPorts.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/HostPorts.java @@ -6,12 +6,9 @@ import com.yahoo.config.application.api.DeployLogger; import com.yahoo.config.provision.NetworkPorts; import java.util.ArrayList; -import java.util.Collection; import java.util.Collections; -import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; -import java.util.ListIterator; import java.util.Map; import java.util.Optional; import java.util.logging.Level; @@ -146,13 +143,6 @@ public class HostPorts { return allocator.result(); } - void deallocatePorts(NetworkPortRequestor service) { - if (networkPortsList.isPresent()) - throw new IllegalStateException("Cannot deallocate ports after calling flushPortReservations()"); - portDB.entrySet().removeIf(entry -> entry.getValue().getServiceName().equals(service.getServiceName())); - allocatedPorts--; - } - public void flushPortReservations() { this.networkPortsList = Optional.of(new NetworkPorts(portFinder.allocations())); } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/HostResource.java b/config-model/src/main/java/com/yahoo/vespa/model/HostResource.java index ef041d06978..3bc07db9507 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/HostResource.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/HostResource.java @@ -79,11 +79,6 @@ public class HostResource implements Comparable<HostResource> { return ports; } - void deallocateService(AbstractService service) { - hostPorts.deallocatePorts(service); - services.remove(service.getServiceName()); - } - /** * Returns the service with the given "sentinel name" on this Host, * or null if the name does not match any service. diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/Admin.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/Admin.java index e080ce43730..d1b9bbb4e58 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/admin/Admin.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/admin/Admin.java @@ -148,23 +148,15 @@ public class Admin extends AbstractConfigProducer<Admin> implements Serializable public void setClusterControllers(ClusterControllerContainerCluster clusterControllers, DeployLogger deployLogger) { this.clusterControllers = clusterControllers; - if (isHostedVespa) { - // Prefer to put Slobroks on the admin cluster running cluster controllers to avoid unnecessary - // movement of the slobroks when there are changes to the content cluster nodes - removeSlobroks(); + if (isHostedVespa) addSlobroks(createSlobroksOn(clusterControllers, deployLogger)); - } - } - - private void removeSlobroks() { - slobroks.forEach(Slobrok::remove); - slobroks.clear(); } private List<Slobrok> createSlobroksOn(ClusterControllerContainerCluster clusterControllers, DeployLogger deployLogger) { List<Slobrok> slobroks = new ArrayList<>(); + int index = this.slobroks.size(); for (ClusterControllerContainer clusterController : clusterControllers.getContainers()) { - Slobrok slobrok = new Slobrok(this, clusterController.index()); + Slobrok slobrok = new Slobrok(this, index++); slobrok.setHostResource(clusterController.getHostResource()); slobroks.add(slobrok); slobrok.initService(deployLogger); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/Slobrok.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/Slobrok.java index 00b6d872ddd..eed886b707f 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/admin/Slobrok.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/admin/Slobrok.java @@ -9,10 +9,9 @@ import com.yahoo.vespa.model.PortAllocBridge; /** * Represents a Slobrok service. * - * @author gjoranv + * @author gjoranv */ public class Slobrok extends AbstractService implements StateserverConfig.Producer { - private static final long serialVersionUID = 1L; public final static int BASEPORT = 19099; @@ -26,7 +25,7 @@ public class Slobrok extends AbstractService implements StateserverConfig.Produc * @param parent the parent ConfigProducer. * @param index unique index for all slobroks */ - public Slobrok(AbstractConfigProducer<?> parent, int index) { + public Slobrok(AbstractConfigProducer parent, int index) { super(parent, "slobrok." + index); portsMeta.on(0).tag("rpc").tag("admin").tag("status"); portsMeta.on(1).tag("http").tag("state"); diff --git a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomAdminV4Builder.java b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomAdminV4Builder.java index e27faa13000..a0673824907 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomAdminV4Builder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomAdminV4Builder.java @@ -67,7 +67,7 @@ public class DomAdminV4Builder extends DomAdminBuilderBase { admin, allocateHosts(admin.hostSystem(), "slobroks", nodesSpecification)); } - else { // These will be removed later, if an admin cluster (for cluster controllers) is assigned + else { // TODO: Remove createSlobroks(deployLogger, admin, pickContainerHostsForSlobrok(nodesSpecification.minResources().nodes(), 2)); diff --git a/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java b/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java index 79e3e869b52..326bf577acc 100644 --- a/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java +++ b/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java @@ -494,8 +494,9 @@ public class ModelProvisioningTest { Set<HostResource> clusterControllerHosts = admin.getClusterControllers().getContainers() .stream().map(cc -> cc.getHostResource()).collect(Collectors.toSet()); Set<HostResource> slobrokHosts = admin.getSlobroks().stream().map(Slobrok::getHost).collect(Collectors.toSet()); - assertEquals(3, slobrokHosts.size()); - assertTrue("Slobroks are assigned on cluster controller nodes", clusterControllerHosts.containsAll(slobrokHosts)); + assertEquals(6, slobrokHosts.size()); + assertTrue("Slobroks are assigned from container and cluster controller nodes", + union(containerHosts, clusterControllerHosts).containsAll(slobrokHosts)); assertTrue("Logserver is assigned from container nodes", containerHosts.contains(admin.getLogserver().getHost())); assertEquals("No in-cluster config servers in a hosted environment", 0, admin.getConfigservers().size()); assertEquals("Dedicated admin cluster controllers when hosted", 3, admin.getClusterControllers().getContainers().size()); @@ -559,40 +560,6 @@ public class ModelProvisioningTest { } @Test - public void testSlobroksOnContainersIfNoContentClusters() { - String services = - "<?xml version='1.0' encoding='utf-8' ?>\n" + - "<services>" + - " <admin version='4.0'/>" + - " <container version='1.0' id='foo'>" + - " <nodes count='10'/>" + - " </container>" + - "</services>"; - - int numberOfHosts = 10; - VespaModelTester tester = new VespaModelTester(); - tester.addHosts(numberOfHosts); - VespaModel model = tester.createModel(services, true); - assertEquals(numberOfHosts, model.getRoot().hostSystem().getHosts().size()); - - // Check container cluster - assertEquals(1, model.getContainerClusters().size()); - Set<HostResource> containerHosts = model.getContainerClusters().get("foo").getContainers().stream() - .map(Container::getHost) - .collect(Collectors.toSet()); - assertEquals(10, containerHosts.size()); - - // Check admin clusters - Admin admin = model.getAdmin(); - Set<HostResource> slobrokHosts = admin.getSlobroks().stream().map(Slobrok::getHost).collect(Collectors.toSet()); - assertEquals(3, slobrokHosts.size()); - assertTrue("Slobroks are assigned from container nodes", - containerHosts.containsAll(slobrokHosts)); - assertTrue("Logserver is assigned from container nodes", containerHosts.contains(admin.getLogserver().getHost())); - assertEquals("No in-cluster config servers in a hosted environment", 0, admin.getConfigservers().size()); - } - - @Test public void testUsingNodesAndGroupCountAttributesWithoutDedicatedClusterControllers() { String services = "<?xml version='1.0' encoding='utf-8' ?>\n" + @@ -635,8 +602,9 @@ public class ModelProvisioningTest { Set<HostResource> clusterControllerHosts = admin.getClusterControllers().getContainers() .stream().map(cc -> cc.getHostResource()).collect(Collectors.toSet()); Set<HostResource> slobrokHosts = admin.getSlobroks().stream().map(Slobrok::getHost).collect(Collectors.toSet()); - assertEquals(3, slobrokHosts.size()); - assertTrue("Slobroks are assigned on cluster controller nodes", clusterControllerHosts.containsAll(slobrokHosts)); + assertEquals(6, slobrokHosts.size()); + assertTrue("Slobroks are assigned from container and cluster controller nodes", + union(containerHosts, clusterControllerHosts).containsAll(slobrokHosts)); assertTrue("Logserver is assigned from container nodes", containerHosts.contains(admin.getLogserver().getHost())); assertEquals("No in-cluster config servers in a hosted environment", 0, admin.getConfigservers().size()); assertEquals(3, admin.getClusterControllers().getContainers().size()); @@ -1645,7 +1613,7 @@ public class ModelProvisioningTest { tester.addHosts(6); VespaModel model = tester.createModel(services, true); assertEquals(6, model.getRoot().hostSystem().getHosts().size()); - assertEquals(3, model.getAdmin().getSlobroks().size()); + assertEquals(5, model.getAdmin().getSlobroks().size()); assertEquals(2, model.getContainerClusters().get("foo").getContainers().size()); assertEquals(1, model.getContentClusters().get("bar").getRootGroup().countNodes(true)); } @@ -2170,4 +2138,11 @@ public class ModelProvisioningTest { assertProvisioned(nodeCount, id, null, type, model); } + private Set<HostResource> union(Set<HostResource> a, Set<HostResource> b) { + Set<HostResource> union = new HashSet<>(); + union.addAll(a); + union.addAll(b); + return union; + } + } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java index a79d705963b..6bda6dc8a25 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java @@ -57,9 +57,11 @@ public class DeploymentMetricsMaintainer extends ControllerMaintainer { for (Deployment deployment : instance.deployments().values()) { attempts.incrementAndGet(); try { + if (deployment.version().getMajor() < 7) continue; DeploymentId deploymentId = new DeploymentId(instance.id(), deployment.zone()); List<ClusterMetrics> clusterMetrics = controller().serviceRegistry().configServer().getDeploymentMetrics(deploymentId); Instant now = controller().clock().instant(); + applications.lockApplicationIfPresent(application.id(), locked -> { Deployment existingDeployment = locked.get().require(instance.name()).deployments().get(deployment.zone()); if (existingDeployment == null) return; // Deployment removed since we started collecting metrics |