diff options
30 files changed, 322 insertions, 352 deletions
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java index e3d41873a8e..854f72b27fb 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java @@ -130,13 +130,18 @@ public class ApplicationController { int count = 0; for (TenantAndApplicationId id: curator.readApplicationIds()) { lockApplicationIfPresent(id, application -> { - if (id.tenant().value().startsWith("by-")) - application = application.with(DeploymentSpec.empty); - else + if (id.tenant().value().startsWith("by-")) { // TODO jonmv: Remove after run once. + for (Instance instance : application.get().instances().values()) + for (ZoneId zone : instance.deployments().keySet()) + deactivate(instance.id(), zone); + curator.removeApplication(id); + } + else { for (InstanceName instance : application.get().deploymentSpec().instanceNames()) - if ( ! application.get().instances().containsKey(instance)) + if (!application.get().instances().containsKey(instance)) application = withNewInstance(application, id.instance(instance)); - store(application); + store(application); + } }); count++; } @@ -229,7 +234,7 @@ public class ApplicationController { * * @throws IllegalArgumentException if the application already exists */ - public Application createApplication(TenantAndApplicationId id, Optional<Credentials> credentials) { + public Application createApplication(TenantAndApplicationId id, Credentials credentials) { try (Lock lock = lock(id)) { if (getApplication(id).isPresent()) throw new IllegalArgumentException("Could not create '" + id + "': Application already exists"); @@ -238,14 +243,9 @@ public class ApplicationController { com.yahoo.vespa.hosted.controller.api.identifiers.ApplicationId.validate(id.application().value()); - Optional<Tenant> tenant = controller.tenants().get(id.tenant()); - if (tenant.isEmpty()) + if (controller.tenants().get(id.tenant()).isEmpty()) throw new IllegalArgumentException("Could not create '" + id + "': This tenant does not exist"); - if (tenant.get().type() != Tenant.Type.user) { - if (credentials.isEmpty()) - throw new IllegalArgumentException("Could not create '" + id + "': No credentials provided"); - accessControl.createApplication(id, credentials.get()); - } + accessControl.createApplication(id, credentials); LockedApplication locked = new LockedApplication(new Application(id, clock.instant()), lock); store(locked); @@ -296,10 +296,6 @@ public class ApplicationController { throw new IllegalArgumentException("'" + instanceId + "' is a tester application!"); TenantAndApplicationId applicationId = TenantAndApplicationId.from(instanceId); - if ( getApplication(applicationId).isEmpty() - && controller.tenants().require(instanceId.tenant()).type() == Tenant.Type.user) - createApplication(applicationId, Optional.empty()); - if (getInstance(instanceId).isEmpty()) createInstance(instanceId); @@ -508,11 +504,7 @@ public class ApplicationController { * * @throws IllegalArgumentException if the application has deployments or the caller is not authorized */ - public void deleteApplication(TenantAndApplicationId id, Optional<Credentials> credentials) { - Tenant tenant = controller.tenants().require(id.tenant()); - if (tenant.type() != Tenant.Type.user && credentials.isEmpty()) - throw new IllegalArgumentException("Could not delete application '" + id + "': No credentials provided"); - + public void deleteApplication(TenantAndApplicationId id, Credentials credentials) { lockApplicationOrThrow(id, application -> { var deployments = application.get().instances().values().stream() .filter(instance -> ! instance.deployments().isEmpty()) @@ -531,8 +523,7 @@ public class ApplicationController { applicationStore.removeAll(id.tenant(), id.application()); applicationStore.removeAllTesters(id.tenant(), id.application()); - if (tenant.type() != Tenant.Type.user) - accessControl.deleteApplication(id, credentials.get()); + accessControl.deleteApplication(id, credentials); curator.removeApplication(id); controller.jobController().collectGarbage(); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedTenant.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedTenant.java index 6caf716aed4..12b985d1812 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedTenant.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedTenant.java @@ -15,7 +15,6 @@ import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant; import com.yahoo.vespa.hosted.controller.api.integration.organization.BillingInfo; import com.yahoo.vespa.hosted.controller.tenant.CloudTenant; import com.yahoo.vespa.hosted.controller.tenant.Tenant; -import com.yahoo.vespa.hosted.controller.tenant.UserTenant; import java.security.Principal; import java.security.PublicKey; @@ -40,7 +39,6 @@ public abstract class LockedTenant { static LockedTenant of(Tenant tenant, Lock lock) { switch (tenant.type()) { case athenz: return new Athenz((AthenzTenant) tenant); - case user: return new User((UserTenant) tenant); case cloud: return new Cloud((CloudTenant) tenant); default: throw new IllegalArgumentException("Unexpected tenant type '" + tenant.getClass().getName() + "'."); } @@ -99,32 +97,6 @@ public abstract class LockedTenant { } - /** A locked UserTenant. */ - public static class User extends LockedTenant { - - private final Optional<Contact> contact; - - private User(TenantName name, Optional<Contact> contact) { - super(name); - this.contact = contact; - } - - private User(UserTenant tenant) { - this(tenant.name(), tenant.contact()); - } - - @Override - public UserTenant get() { - return new UserTenant(name, contact); - } - - public User with(Contact contact) { - return new User(name, Optional.of(contact)); - } - - } - - /** A locked CloudTenant. */ public static class Cloud extends LockedTenant { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/TenantController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/TenantController.java index e794334232f..ae905d2b209 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/TenantController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/TenantController.java @@ -10,7 +10,6 @@ import com.yahoo.vespa.hosted.controller.security.AccessControl; import com.yahoo.vespa.hosted.controller.security.Credentials; import com.yahoo.vespa.hosted.controller.security.TenantSpec; import com.yahoo.vespa.hosted.controller.tenant.Tenant; -import com.yahoo.vespa.hosted.controller.tenant.UserTenant; import java.time.Duration; import java.time.Instant; @@ -47,8 +46,13 @@ public class TenantController { Instant start = controller.clock().instant(); int count = 0; for (TenantName name : curator.readTenantNames()) { - lockIfPresent(name, LockedTenant.class, this::store); - count++; + if (name.value().startsWith(Tenant.userPrefix)) // TODO jonmv: Remove after run once. + + curator.removeTenant(name); + else { + lockIfPresent(name, LockedTenant.class, this::store); + count++; + } } log.log(Level.INFO, String.format("Wrote %d tenants in %s", count, Duration.between(start, controller.clock().instant()))); @@ -94,14 +98,6 @@ public class TenantController { curator.writeTenant(tenant.get()); } - /** Create an user tenant with given username */ - public void createUser(UserTenant tenant) { - try (Lock lock = lock(tenant.name())) { - requireNonExistent(tenant.name()); - curator.writeTenant(tenant); - } - } - /** Create a tenant, provided the given credentials are valid. */ public void create(TenantSpec tenantSpec, Credentials credentials) { try (Lock lock = lock(tenantSpec.tenant())) { @@ -141,13 +137,6 @@ public class TenantController { } } - /** Deletes the given user tenant. */ - public void deleteUser(UserTenant tenant) { - try (Lock lock = lock(tenant.name())) { - curator.removeTenant(tenant.name()); - } - } - private void requireNonExistent(TenantName name) { if ( "hosted-vespa".equals(name.value()) || get(name).isPresent() diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/impl/AthenzFacade.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/impl/AthenzFacade.java index 628d7f48c85..301dad94b7e 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/impl/AthenzFacade.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/impl/AthenzFacade.java @@ -29,7 +29,6 @@ import com.yahoo.vespa.hosted.controller.security.Credentials; import com.yahoo.vespa.hosted.controller.security.TenantSpec; import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant; import com.yahoo.vespa.hosted.controller.tenant.Tenant; -import com.yahoo.vespa.hosted.controller.tenant.UserTenant; import javax.ws.rs.ForbiddenException; import java.util.Arrays; @@ -186,8 +185,8 @@ public class AthenzFacade implements AccessControl { AthenzIdentity identity = ((AthenzPrincipal) credentials.user()).getIdentity(); List<AthenzDomain> userDomains = ztsClient.getTenantDomains(service, identity, "admin"); return tenants.stream() - .filter(tenant -> tenant.type() == Tenant.Type.user && ((UserTenant) tenant).is(identity.getName()) - || tenant.type() == Tenant.Type.athenz && userDomains.contains(((AthenzTenant) tenant).domain())) + .filter(tenant -> tenant.type() == Tenant.Type.athenz + && userDomains.contains(((AthenzTenant) tenant).domain())) .collect(Collectors.toUnmodifiableList()); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java index be189004f6d..6904bff8548 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java @@ -435,10 +435,6 @@ public class JobController { if ( ! type.environment().isManuallyDeployed()) throw new IllegalArgumentException("Direct deployments are only allowed to manually deployed environments."); - if ( controller.tenants().require(id.tenant()).type() == Tenant.Type.user - && controller.applications().getApplication(TenantAndApplicationId.from(id)).isEmpty()) - controller.applications().createApplication(TenantAndApplicationId.from(id), Optional.empty()); - controller.applications().lockApplicationOrThrow(TenantAndApplicationId.from(id), application -> { if ( ! application.get().instances().containsKey(id.instance())) application = controller.applications().withNewInstance(application, id); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java index c854c5b45bf..002ec1f4a05 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java @@ -14,7 +14,6 @@ import com.yahoo.vespa.hosted.controller.api.integration.organization.User; import com.yahoo.vespa.hosted.controller.application.ApplicationList; import com.yahoo.vespa.hosted.controller.application.TenantAndApplicationId; import com.yahoo.vespa.hosted.controller.tenant.Tenant; -import com.yahoo.vespa.hosted.controller.tenant.UserTenant; import com.yahoo.yolean.Exceptions; import java.time.Duration; @@ -57,11 +56,10 @@ public class ApplicationOwnershipConfirmer extends Maintainer { .filter(application -> application.createdAt().isBefore(controller().clock().instant().minus(Duration.ofDays(90)))) .forEach(application -> { try { - Tenant tenant = tenantOf(application.id()); - tenant.contact().ifPresent(contact -> { // TODO jvenstad: Makes sense to require, and run this only in main? + tenantOf(application.id()).contact().ifPresent(contact -> { // TODO jvenstad: Makes sense to require, and run this only in main? ownershipIssues.confirmOwnership(application.ownershipIssueId(), summaryOf(application.id()), - determineAssignee(tenant, application), + determineAssignee(application), contact) .ifPresent(newIssueId -> store(newIssueId, application.id())); }); @@ -94,7 +92,7 @@ public class ApplicationOwnershipConfirmer extends Maintainer { application.ownershipIssueId().ifPresent(issueId -> { try { Tenant tenant = tenantOf(application.id()); - ownershipIssues.ensureResponse(issueId, tenant.type() == Tenant.Type.athenz ? tenant.contact() : Optional.empty()); + ownershipIssues.ensureResponse(issueId, tenant.contact()); } catch (RuntimeException e) { log.log(Level.INFO, "Exception caught when attempting to escalate issue with id '" + issueId + "': " + Exceptions.toMessageString(e)); @@ -118,8 +116,8 @@ public class ApplicationOwnershipConfirmer extends Maintainer { }); } - private User determineAssignee(Tenant tenant, Application application) { - return application.owner().orElse(tenant instanceof UserTenant ? userFor(tenant) : null); + private User determineAssignee(Application application) { + return application.owner().orElse(null); } private Tenant tenantOf(TenantAndApplicationId applicationId) { 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 cbe9d8c70c1..5825285f9b0 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 @@ -40,9 +40,6 @@ public class ContactInformationMaintainer extends Maintainer { case athenz: tenants.lockIfPresent(tenant.name(), LockedTenant.Athenz.class, lockedTenant -> tenants.store(lockedTenant.with(contactRetriever.getContact(lockedTenant.get().propertyId())))); return; - case user: tenants.lockIfPresent(tenant.name(), LockedTenant.User.class, lockedTenant -> - tenants.store(lockedTenant.with(contactRetriever.getContact(Optional.empty())))); - return; case cloud: return; default: throw new IllegalArgumentException("Unexpected tenant type '" + tenant.type() + "'."); } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java index 3be15b67252..e7c0094b7ab 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java @@ -114,7 +114,7 @@ public class DeploymentIssueReporter extends Maintainer { try { Tenant tenant = ownerOf(application.id()); tenant.contact().ifPresent(contact -> { - User assignee = tenant.type() == Tenant.Type.user ? userFor(tenant) : application.owner().orElse(null); + User assignee = application.owner().orElse(null); Optional<IssueId> ourIssueId = application.deploymentIssueId(); IssueId issueId = deploymentIssues.fileUnlessOpen(ourIssueId, application.id().defaultInstance(), assignee, contact); store(application.id(), issueId); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/TenantSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/TenantSerializer.java index 7f938885cac..9df87ab4c12 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/TenantSerializer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/TenantSerializer.java @@ -19,7 +19,6 @@ import com.yahoo.vespa.hosted.controller.api.integration.organization.Contact; import com.yahoo.vespa.hosted.controller.api.integration.organization.BillingInfo; import com.yahoo.vespa.hosted.controller.tenant.CloudTenant; import com.yahoo.vespa.hosted.controller.tenant.Tenant; -import com.yahoo.vespa.hosted.controller.tenant.UserTenant; import java.net.URI; import java.security.Principal; @@ -68,7 +67,6 @@ public class TenantSerializer { switch (tenant.type()) { case athenz: toSlime((AthenzTenant) tenant, tenantObject); break; - case user: toSlime((UserTenant) tenant, tenantObject); break; case cloud: toSlime((CloudTenant) tenant, tenantObject); break; default: throw new IllegalArgumentException("Unexpected tenant type '" + tenant.type() + "'."); } @@ -85,13 +83,6 @@ public class TenantSerializer { }); } - private void toSlime(UserTenant tenant, Cursor tenantObject) { - tenant.contact().ifPresent(contact -> { - Cursor contactCursor = tenantObject.setObject(contactField); - writeContact(contact, contactCursor); - }); - } - private void toSlime(CloudTenant tenant, Cursor root) { developerKeysToSlime(tenant.developerKeys(), root.setArray(pemDeveloperKeysField)); toSlime(tenant.billingInfo(), root.setObject(billingInfoField)); @@ -117,7 +108,7 @@ public class TenantSerializer { switch (type) { case athenz: return athenzTenantFrom(tenantObject); - case user: return userTenantFrom(tenantObject); + case user: return null; // TODO jonmv: Remove when run once. case cloud: return cloudTenantFrom(tenantObject); default: throw new IllegalArgumentException("Unexpected tenant type '" + type + "'."); } @@ -132,12 +123,6 @@ public class TenantSerializer { return new AthenzTenant(name, domain, property, propertyId, contact); } - private UserTenant userTenantFrom(Inspector tenantObject) { - TenantName name = TenantName.from(tenantObject.field(nameField).asString()); - Optional<Contact> contact = contactFrom(tenantObject.field(contactField)); - return new UserTenant(name, contact); - } - private CloudTenant cloudTenantFrom(Inspector tenantObject) { TenantName name = TenantName.from(tenantObject.field(nameField).asString()); BillingInfo billingInfo = billingInfoFrom(tenantObject.field(billingInfoField)); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java index 94d7b120406..5e234d31322 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java @@ -90,7 +90,6 @@ import com.yahoo.vespa.hosted.controller.security.Credentials; import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant; import com.yahoo.vespa.hosted.controller.tenant.CloudTenant; import com.yahoo.vespa.hosted.controller.tenant.Tenant; -import com.yahoo.vespa.hosted.controller.tenant.UserTenant; import com.yahoo.vespa.hosted.controller.versions.VespaVersion; import com.yahoo.vespa.serviceview.bindings.ApplicationView; import com.yahoo.yolean.Exceptions; @@ -252,7 +251,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler { } private HttpResponse handlePUT(Path path, HttpRequest request) { - if (path.matches("/application/v4/user")) return createUser(request); + if (path.matches("/application/v4/user")) return new EmptyResponse(); if (path.matches("/application/v4/tenant/{tenant}")) return updateTenant(path.get("tenant"), request); if (path.matches("/application/v4/tenant/{tenant}/application/{application}/instance/{instance}/environment/{environment}/region/{region}/global-rotation/override")) return setGlobalRotationOverride(path.get("tenant"), path.get("application"), path.get("instance"), path.get("environment"), path.get("region"), false, request); if (path.matches("/application/v4/tenant/{tenant}/application/{application}/environment/{environment}/region/{region}/instance/{instance}/global-rotation/override")) return setGlobalRotationOverride(path.get("tenant"), path.get("application"), path.get("instance"), path.get("environment"), path.get("region"), false, request); @@ -335,11 +334,8 @@ public class ApplicationApiHandler extends LoggingRequestHandler { // TODO jonmv: Move to Athenz API. private HttpResponse authenticatedUser(HttpRequest request) { Principal user = requireUserPrincipal(request); - if (user == null) - throw new NotAuthorizedException("You must be authenticated."); String userName = user instanceof AthenzPrincipal ? ((AthenzPrincipal) user).getIdentity().getName() : user.getName(); - TenantName tenantName = TenantName.from(UserTenant.normalizeUser(userName)); List<Tenant> tenants = controller.tenants().asList(new Credentials(user)); Slime slime = new Slime(); @@ -348,7 +344,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler { Cursor tenantsArray = response.setArray("tenants"); for (Tenant tenant : tenants) tenantInTenantsListToSlime(tenant, request.getUri(), tenantsArray.addObject()); - response.setBool("tenantExists", tenants.stream().anyMatch(tenant -> tenant.name().equals(tenantName))); + response.setBool("tenantExists", true); return new SlimeJsonResponse(slime); } @@ -1423,26 +1419,6 @@ public class ApplicationApiHandler extends LoggingRequestHandler { return response; } - private HttpResponse createUser(HttpRequest request) { - String user = Optional.of(requireUserPrincipal(request)) - .filter(AthenzPrincipal.class::isInstance) - .map(AthenzPrincipal.class::cast) - .map(AthenzPrincipal::getIdentity) - .filter(AthenzUser.class::isInstance) - .map(AthenzIdentity::getName) - .map(UserTenant::normalizeUser) - .orElseThrow(() -> new ForbiddenException("Not authenticated or not a user.")); - - UserTenant tenant = UserTenant.create(user); - try { - controller.tenants().createUser(tenant); - return new MessageResponse("Created user '" + user + "'"); - } catch (AlreadyExistsException e) { - // Ok - return new MessageResponse("User '" + user + "' already exists"); - } - } - private HttpResponse updateTenant(String tenantName, HttpRequest request) { getTenantOrThrow(tenantName); TenantName tenant = TenantName.from(tenantName); @@ -1463,11 +1439,8 @@ public class ApplicationApiHandler extends LoggingRequestHandler { private HttpResponse createApplication(String tenantName, String applicationName, HttpRequest request) { Inspector requestObject = toSlime(request.getData()).get(); TenantAndApplicationId id = TenantAndApplicationId.from(tenantName, applicationName); - Optional<Credentials> credentials = controller.tenants().require(id.tenant()).type() == Tenant.Type.user - ? Optional.empty() - : Optional.of(accessControlRequests.credentials(id.tenant(), requestObject, request.getJDiscRequest())); + Credentials credentials = accessControlRequests.credentials(id.tenant(), requestObject, request.getJDiscRequest()); Application application = controller.applications().createApplication(id, credentials); - Slime slime = new Slime(); toSlime(id, slime.setObject(), request); return new SlimeJsonResponse(slime); @@ -1693,16 +1666,13 @@ public class ApplicationApiHandler extends LoggingRequestHandler { private HttpResponse deleteTenant(String tenantName, HttpRequest request) { Optional<Tenant> tenant = controller.tenants().get(tenantName); - if ( ! tenant.isPresent()) + if (tenant.isEmpty()) return ErrorResponse.notFoundError("Could not delete tenant '" + tenantName + "': Tenant not found"); - if (tenant.get().type() == Tenant.Type.user) - controller.tenants().deleteUser((UserTenant) tenant.get()); - else - controller.tenants().delete(tenant.get().name(), - accessControlRequests.credentials(tenant.get().name(), - toSlime(request.getData()).get(), - request.getJDiscRequest())); + controller.tenants().delete(tenant.get().name(), + accessControlRequests.credentials(tenant.get().name(), + toSlime(request.getData()).get(), + request.getJDiscRequest())); // TODO: Change to a message response saying the tenant was deleted return tenant(tenant.get(), request); @@ -1710,9 +1680,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler { private HttpResponse deleteApplication(String tenantName, String applicationName, HttpRequest request) { TenantAndApplicationId id = TenantAndApplicationId.from(tenantName, applicationName); - Optional<Credentials> credentials = controller.tenants().require(id.tenant()).type() == Tenant.Type.user - ? Optional.empty() - : Optional.of(accessControlRequests.credentials(id.tenant(), toSlime(request.getData()).get(), request.getJDiscRequest())); + Credentials credentials = accessControlRequests.credentials(id.tenant(), toSlime(request.getData()).get(), request.getJDiscRequest()); controller.applications().deleteApplication(id, credentials); return new MessageResponse("Deleted application " + id); } @@ -1721,9 +1689,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler { TenantAndApplicationId id = TenantAndApplicationId.from(tenantName, applicationName); controller.applications().deleteInstance(id.instance(instanceName)); if (controller.applications().requireApplication(id).instances().isEmpty()) { - Optional<Credentials> credentials = controller.tenants().require(id.tenant()).type() == Tenant.Type.user - ? Optional.empty() - : Optional.of(accessControlRequests.credentials(id.tenant(), toSlime(request.getData()).get(), request.getJDiscRequest())); + Credentials credentials = accessControlRequests.credentials(id.tenant(), toSlime(request.getData()).get(), request.getJDiscRequest()); controller.applications().deleteApplication(id, credentials); } return new MessageResponse("Deleted instance " + id.instance(instanceName).toFullString()); @@ -1797,7 +1763,6 @@ public class ApplicationApiHandler extends LoggingRequestHandler { }); }); break; - case user: break; case cloud: { CloudTenant cloudTenant = (CloudTenant) tenant; @@ -1836,7 +1801,6 @@ public class ApplicationApiHandler extends LoggingRequestHandler { metaData.setString("athensDomain", athenzTenant.domain().getName()); metaData.setString("property", athenzTenant.property().id()); break; - case user: break; case cloud: break; default: throw new IllegalArgumentException("Unexpected tenant type '" + tenant.type() + "'."); } @@ -2055,7 +2019,6 @@ public class ApplicationApiHandler extends LoggingRequestHandler { private static String tenantType(Tenant tenant) { switch (tenant.type()) { - case user: return "USER"; case athenz: return "ATHENS"; case cloud: return "CLOUD"; default: throw new IllegalArgumentException("Unknown tenant type: " + tenant.getClass().getSimpleName()); diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/tenant/Tenant.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/tenant/Tenant.java index e0c750dec80..55133aa06c8 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/tenant/Tenant.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/tenant/Tenant.java @@ -65,7 +65,7 @@ public abstract class Tenant { athenz, /** Tenant authenticated through Okta, as a user. */ - user, + user, // TODO jonmv: Remove. /** Tenant authenticated through some cloud identity provider. */ cloud; diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/tenant/UserTenant.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/tenant/UserTenant.java deleted file mode 100644 index a46d847f6f3..00000000000 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/tenant/UserTenant.java +++ /dev/null @@ -1,73 +0,0 @@ -// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.vespa.hosted.controller.tenant; - -import com.yahoo.config.provision.TenantName; -import com.yahoo.vespa.hosted.controller.api.integration.organization.Contact; - -import java.util.Optional; - -/** - * Represents an user tenant in hosted Vespa. - * - * @author mpolden - */ -public class UserTenant extends Tenant { - - /** - * This should only be used by serialization. - * Use {@link #create(String)}. - * */ - public UserTenant(TenantName name, Optional<Contact> contact) { - super(name, contact); - } - - @Override - public Type type() { - return Type.user; - } - - public UserTenant(TenantName name) { - super(name, Optional.empty()); - } - - /** Returns true if this is the tenant for the given user name */ - public boolean is(String username) { - return name().value().equals(normalizeUser(username)); - } - - @Override - public String toString() { - return "user tenant '" + name() + "'"; - } - - /** Create a new user tenant */ - public static UserTenant create(String username) { - TenantName name = TenantName.from(username); - return new UserTenant(requireName(requireUser(name))); - } - - public static UserTenant create(String username, Optional<Contact> contact) { - TenantName name = TenantName.from(username); - return new UserTenant(requireName(requireUser(name)), contact); - } - - /** Normalize given username. E.g. foo_bar becomes by-foo-bar */ - public static String normalizeUser(String username) { - int offset = 0; - if (username.startsWith(Tenant.userPrefix)) { - offset = Tenant.userPrefix.length(); - } - return Tenant.userPrefix + username.substring(offset).replace('_', '-'); - } - - private static TenantName requireUser(TenantName name) { - if (!name.value().startsWith(Tenant.userPrefix)) { - throw new IllegalArgumentException("User tenant must have prefix '" + Tenant.userPrefix + "'"); - } - if (name.value().substring(Tenant.userPrefix.length()).contains("_")) { - throw new IllegalArgumentException("User tenant cannot contain '_'"); - } - return name; - } - -} diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java index a816f82c044..b5796bb4ecc 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java @@ -327,20 +327,20 @@ public final class ControllerTester { return tenant; } - public Optional<Credentials> credentialsFor(TenantName tenantName) { + public Credentials credentialsFor(TenantName tenantName) { Tenant tenant = controller().tenants().require(tenantName); switch (tenant.type()) { case athenz: - return Optional.of(new AthenzCredentials(new AthenzPrincipal(new AthenzUser("user")), + return new AthenzCredentials(new AthenzPrincipal(new AthenzUser("user")), ((AthenzTenant) tenant).domain(), new OktaIdentityToken("okta-identity-token"), - new OktaAccessToken("okta-access-token"))); + new OktaAccessToken("okta-access-token")); case cloud: - return Optional.of(new Credentials(new SimplePrincipal("dev"))); + return new Credentials(new SimplePrincipal("dev")); default: - return Optional.empty(); + throw new IllegalArgumentException("Unexpected tenant type '" + tenant.type() + "'"); } } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmerTest.java index 8997f34fb98..e7e508f1789 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmerTest.java @@ -10,7 +10,6 @@ import com.yahoo.vespa.hosted.controller.api.integration.organization.OwnershipI import com.yahoo.vespa.hosted.controller.api.integration.organization.User; import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester; import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb; -import com.yahoo.vespa.hosted.controller.tenant.UserTenant; import org.junit.Before; import org.junit.Test; @@ -42,70 +41,70 @@ public class ApplicationOwnershipConfirmerTest { @Test public void testConfirmation() { Optional<Contact> contact = Optional.of(tester.controllerTester().serviceRegistry().contactRetrieverMock().contact()); - var propertyApp = tester.newDeploymentContext(); + var app = tester.newDeploymentContext(); tester.controller().tenants().lockOrThrow(appId.tenant(), LockedTenant.Athenz.class, tenant -> tester.controller().tenants().store(tenant.with(contact.get()))); - propertyApp.submit().deploy(); + app.submit().deploy(); - UserTenant user = UserTenant.create("by-user", contact); - tester.controller().tenants().createUser(user); - var userApp = tester.newDeploymentContext("by-user", "application", "default"); - userApp.submit().deploy(); + var appWithoutContact = tester.newDeploymentContext("other", "application", "default"); + appWithoutContact.submit().deploy(); - assertFalse("No issue is initially stored for a new application.", propertyApp.application().ownershipIssueId().isPresent()); - assertFalse("No issue is initially stored for a new application.", userApp.application().ownershipIssueId().isPresent()); - assertFalse("No escalation has been attempted for a new application", issues.escalatedToContact || issues.escalatedToTerminator); + assertFalse("No issue is initially stored for a new application.", app.application().ownershipIssueId().isPresent()); + assertFalse("No issue is initially stored for a new application.", appWithoutContact.application().ownershipIssueId().isPresent()); + assertFalse("No escalation has been attempted for a new application", issues.escalated); // Set response from the issue mock, which will be obtained by the maintainer on issue filing. Optional<IssueId> issueId = Optional.of(IssueId.from("1")); issues.response = issueId; confirmer.maintain(); - assertFalse("No issue is stored for an application newer than 3 months.", propertyApp.application().ownershipIssueId().isPresent()); - assertFalse("No issue is stored for an application newer than 3 months.", userApp.application().ownershipIssueId().isPresent()); + assertFalse("No issue is stored for an application newer than 3 months.", app.application().ownershipIssueId().isPresent()); + assertFalse("No issue is stored for an application newer than 3 months.", appWithoutContact.application().ownershipIssueId().isPresent()); tester.clock().advance(Duration.ofDays(91)); confirmer.maintain(); - assertEquals("Confirmation issue has been filed for property owned application.", issueId, propertyApp.application().ownershipIssueId()); - assertEquals("Confirmation issue has been filed for user owned application.", issueId, userApp.application().ownershipIssueId()); - assertTrue(issues.escalatedToTerminator); - assertTrue("Both applications have had their responses ensured.", issues.escalatedToContact); + assertEquals("Confirmation issue has been filed for application with contact.", issueId, app.application().ownershipIssueId()); + assertTrue("The confirmation issue response has been ensured.", issues.escalated); + assertEquals("No confirmation issue has been filed for application without contact.", Optional.empty(), appWithoutContact.application().ownershipIssueId()); // No new issue is created, so return empty now. issues.response = Optional.empty(); confirmer.maintain(); - assertEquals("Confirmation issue reference is not updated when no issue id is returned.", issueId, propertyApp.application().ownershipIssueId()); - assertEquals("Confirmation issue reference is not updated when no issue id is returned.", issueId, userApp.application().ownershipIssueId()); - - // The user deletes all production deployments — see that the issue is forgotten. - assertEquals("Confirmation issue for user is still open.", issueId, userApp.application().ownershipIssueId()); - userApp.application().productionDeployments().values().stream().flatMap(List::stream) - .forEach(deployment -> tester.controller().applications().deactivate(userApp.instanceId(), deployment.zone())); - assertTrue("No production deployments are listed for user.", userApp.application().require(InstanceName.defaultName()).productionDeployments().isEmpty()); - confirmer.maintain(); + assertEquals("Confirmation issue reference is not updated when no issue id is returned.", issueId, app.application().ownershipIssueId()); // Time has passed, and a new confirmation issue is in order for the property which is still in production. Optional<IssueId> issueId2 = Optional.of(IssueId.from("2")); issues.response = issueId2; confirmer.maintain(); - assertEquals("A new confirmation issue id is stored when something is returned to the maintainer.", issueId2, propertyApp.application().ownershipIssueId()); - assertEquals("Confirmation issue for application without production deployments has not been filed.", issueId, userApp.application().ownershipIssueId()); + assertEquals("A new confirmation issue id is stored when something is returned to the maintainer.", issueId2, app.application().ownershipIssueId()); - assertFalse("No owner is stored for application", propertyApp.application().owner().isPresent()); + assertFalse("No owner is stored for application", app.application().owner().isPresent()); issues.owner = Optional.of(User.from("username")); confirmer.maintain(); - assertEquals("Owner has been added to application", propertyApp.application().owner().get().username(), "username"); + assertEquals("Owner has been added to application", app.application().owner().get().username(), "username"); + + // The app deletes all production deployments — see that the issue is forgotten. + assertEquals("Confirmation issue for application is still open.", issueId2, app.application().ownershipIssueId()); + app.application().productionDeployments().values().stream().flatMap(List::stream) + .forEach(deployment -> tester.controller().applications().deactivate(app.instanceId(), deployment.zone())); + assertTrue("No production deployments are listed for user.", app.application().require(InstanceName.defaultName()).productionDeployments().isEmpty()); + confirmer.maintain(); + + // Time has passed, and a new confirmation issue is in order for the property which is still in production. + Optional<IssueId> issueId3 = Optional.of(IssueId.from("3")); + issues.response = issueId3; + confirmer.maintain(); + assertEquals("Confirmation issue for application without production deployments has not been filed.", issueId2, app.application().ownershipIssueId()); } private class MockOwnershipIssues implements OwnershipIssues { private Optional<IssueId> response; - private boolean escalatedToContact = false; - private boolean escalatedToTerminator = false; + private boolean escalated = false; private Optional<User> owner = Optional.empty(); @Override @@ -115,8 +114,7 @@ public class ApplicationOwnershipConfirmerTest { @Override public void ensureResponse(IssueId issueId, Optional<Contact> contact) { - if (contact.isPresent()) escalatedToContact = true; - else escalatedToTerminator = true; + escalated = true; } @Override diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/TenantSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/TenantSerializerTest.java index ff1c952c2a5..9c085bb72f7 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/TenantSerializerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/TenantSerializerTest.java @@ -12,7 +12,6 @@ import com.yahoo.vespa.hosted.controller.api.integration.organization.Contact; import com.yahoo.vespa.hosted.controller.api.role.SimplePrincipal; import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant; import com.yahoo.vespa.hosted.controller.tenant.CloudTenant; -import com.yahoo.vespa.hosted.controller.tenant.UserTenant; import org.junit.Test; import java.net.URI; @@ -77,14 +76,6 @@ public class TenantSerializerTest { } @Test - public void user_tenant() { - UserTenant tenant = UserTenant.create("by-foo", Optional.of(contact())); - UserTenant serialized = (UserTenant) serializer.tenantFrom(serializer.toSlime(tenant)); - assertEquals(tenant.name(), serialized.name()); - assertEquals(contact(), serialized.contact().get()); - } - - @Test public void cloud_tenant() { CloudTenant tenant = new CloudTenant(TenantName.from("elderly-lady"), new BillingInfo("old cat lady", "vespa"), diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java index 442770ba23e..cfde999973f 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java @@ -182,15 +182,16 @@ public class ApplicationApiTest extends ControllerContainerTest { // GET the authenticated user (with associated tenants) tester.assertResponse(request("/application/v4/user", GET).userIdentity(USER_ID), new File("user.json")); - // PUT a user tenant + // TODO jonmv: Remove when dashboard is gone. + // PUT a user tenant — does nothing tester.assertResponse(request("/application/v4/user", PUT).userIdentity(USER_ID), - "{\"message\":\"Created user 'by-myuser'\"}"); + ""); // GET the authenticated user which now exists (with associated tenants) tester.assertResponse(request("/application/v4/user", GET).userIdentity(USER_ID), - new File("user-which-exists.json")); - // DELETE the user + new File("user.json")); + // DELETE the user — it doesn't exist, so access control fails tester.assertResponse(request("/application/v4/tenant/by-myuser", DELETE).userIdentity(USER_ID), - "{\"tenant\":\"by-myuser\",\"type\":\"USER\",\"applications\":[]}"); + "{\n \"code\" : 403,\n \"message\" : \"Access denied\"\n}", 403); // GET all tenants tester.assertResponse(request("/application/v4/tenant/", GET).userIdentity(USER_ID), new File("tenant-list.json")); @@ -753,17 +754,10 @@ public class ApplicationApiTest extends ControllerContainerTest { .userIdentity(USER_ID), "{\"message\":\"Aborting run 2 of staging-test for tenant1.application1.instance1\"}"); - // PUT (create) the authenticated user - byte[] data = new byte[0]; - tester.assertResponse(request("/application/v4/user?user=new_user&domain=by", PUT) - .data(data) - .userIdentity(new UserId("new_user")), // Normalized to by-new-user by API - new File("create-user-response.json")); - // GET user lists only tenants for the authenticated user tester.assertResponse(request("/application/v4/user", GET) .userIdentity(new UserId("other_user")), - "{\"user\":\"other_user\",\"tenants\":[],\"tenantExists\":false}"); + "{\"user\":\"other_user\",\"tenants\":[],\"tenantExists\":true}"); // OPTIONS return 200 OK tester.assertResponse(request("/application/v4/", Request.Method.OPTIONS) @@ -1374,22 +1368,22 @@ public class ApplicationApiTest extends ControllerContainerTest { // PUT (create) the authenticated user tester.assertResponse(request("/application/v4/user?user=new_user&domain=by", PUT) .userIdentity(userId), // Normalized to by-new-user by API - new File("create-user-response.json")); + ""); ApplicationPackage applicationPackage = new ApplicationPackageBuilder() .athenzIdentity(com.yahoo.config.provision.AthenzDomain.from("domain1"), com.yahoo.config.provision.AthenzService.from("service")) .build(); - // POST (deploy) an application to a dev zone + // POST (deploy) an application to a dev zone fails because user tenant is used — these do not exist. MultiPartStreamer entity = createApplicationDeployData(applicationPackage, true); tester.assertResponse(request("/application/v4/tenant/by-new-user/application/application1/environment/dev/region/us-west-1/instance/default", POST) .data(entity) .userIdentity(userId), - "{\"error-code\":\"BAD_REQUEST\",\"message\":\"User user.new-user is not allowed to launch service domain1.service. Please reach out to the domain admin.\"}", - 400); + "{\n \"code\" : 403,\n \"message\" : \"Access denied\"\n}", + 403); createTenantAndApplication(); - // POST (deploy) an application to dev through a deployment job + // POST (deploy) an application to dev through a deployment job, with user instance and a proper tenant tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/new-user/deploy/dev-us-east-1", POST) .data(entity) .userIdentity(userId), @@ -1401,11 +1395,12 @@ public class ApplicationApiTest extends ControllerContainerTest { .domains.get(ATHENZ_TENANT_DOMAIN) .admin(HostedAthenzIdentities.from(userId)); - // POST (deploy) an application to a dev zone - tester.assertResponse(request("/application/v4/tenant/by-new-user/application/application1/environment/dev/region/us-east-1/instance/default", POST) + // POST (deploy) an application to a dev zone fails because user tenant is used — these do not exist. + tester.assertResponse(request("/application/v4/tenant/by-new-user/application/application1/environment/dev/region/us-west-1/instance/default", POST) .data(entity) .userIdentity(userId), - new File("deploy-result.json")); + "{\n \"code\" : 403,\n \"message\" : \"Access denied\"\n}", + 403); // POST (deploy) an application to dev through a deployment job tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/instance/new-user/deploy/dev-us-east-1", POST) @@ -1434,7 +1429,7 @@ public class ApplicationApiTest extends ControllerContainerTest { AthenzCredentials credentials = new AthenzCredentials( new AthenzPrincipal(new AthenzUser(developer.id())), sandboxDomain, OKTA_IT, OKTA_AT); tester.controller().tenants().create(tenantSpec, credentials); - tester.controller().applications().createApplication(TenantAndApplicationId.from("sandbox", "myapp"), Optional.of(credentials)); + tester.controller().applications().createApplication(TenantAndApplicationId.from("sandbox", "myapp"), credentials); // Create an application package referencing the service from the other domain ApplicationPackage applicationPackage = new ApplicationPackageBuilder() diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/user.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/user.json index 79b9a785801..9902267dbb5 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/user.json +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/user.json @@ -1,5 +1,5 @@ { "user": "myuser", "tenants": @include(tenant-list.json), - "tenantExists": false + "tenantExists": true }
\ No newline at end of file diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/user/UserApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/user/UserApiTest.java index d70a09414bb..93d88ff8abd 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/user/UserApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/user/UserApiTest.java @@ -71,10 +71,10 @@ public class UserApiTest extends ControllerContainerCloudTest { .data("{\"token\":\"hello\"}"), new File("tenant-without-applications.json")); - // PUT a tenant is not available to anyone. + // PUT a tenant is ignored. tester.assertResponse(request("/application/v4/user/", PUT) .roles(operator), - "{\"error-code\":\"FORBIDDEN\",\"message\":\"Not authenticated or not a user.\"}", 403); + "", 200); // GET at user/v1 root fails as no access control is defined there. tester.assertResponse(request("/user/v1/"), 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 bb4e281a85f..a97b21bc2f4 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -67,6 +67,12 @@ public class Flags { "Takes effect on next host admin tick.", HOSTNAME); + public static final UnboundBooleanFlag SERVICE_MODEL_CACHE = defineFeatureFlag( + "service-model-cache", true, + "Whether the service model is cached.", + "Takes effect on restart of config server.", + HOSTNAME); + public static final UnboundListFlag<String> DISABLED_HOST_ADMIN_TASKS = defineListFlag( "disabled-host-admin-tasks", List.of(), String.class, "List of host-admin task names (as they appear in the log, e.g. root>main>UpgradeTask) that should be skipped", diff --git a/jrt/src/com/yahoo/jrt/Connector.java b/jrt/src/com/yahoo/jrt/Connector.java index 57fad5a163d..10f2b3742f2 100644 --- a/jrt/src/com/yahoo/jrt/Connector.java +++ b/jrt/src/com/yahoo/jrt/Connector.java @@ -1,44 +1,67 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.jrt; -import com.yahoo.concurrent.ThreadFactoryFactory; +import com.yahoo.concurrent.CachedThreadPoolWithFallback; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.Executor; import java.util.concurrent.RejectedExecutionException; -import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; -class Connector { +class Connector implements AutoCloseable { - private final ExecutorService executor = new ThreadPoolExecutor(1, 64, 1L, TimeUnit.SECONDS, - new LinkedBlockingQueue<>(), - ThreadFactoryFactory.getDaemonThreadFactory("jrt.connector")); + private static final Object globalLock = new Object(); + private static CachedThreadPoolWithFallback globalExecutor = null; + private static long usages = 0; - private void connect(Connection conn) { - conn.transportThread().addConnection(conn.connect()); + private static CachedThreadPoolWithFallback acquire() { + synchronized (globalLock) { + if (globalExecutor == null) { + globalExecutor = new CachedThreadPoolWithFallback("jrt.connector", 1, 64, 1L, TimeUnit.SECONDS); + } + usages++; + return globalExecutor; + } } - public void connectLater(Connection conn) { - try { - executor.execute(() -> connect(conn)); - } catch (RejectedExecutionException e) { - conn.transportThread().addConnection(conn); + private static void release(CachedThreadPoolWithFallback executor) { + synchronized (globalLock) { + assert executor == globalExecutor; + usages--; + if (usages == 0) { + globalExecutor.close(); + globalExecutor = null; + } } } - public Connector shutdown() { - executor.shutdown(); - return this; + private final AtomicReference<CachedThreadPoolWithFallback> executor; + + Connector() { + executor = new AtomicReference<>(acquire()); } - public void join() { - while (true) { + private void connect(Connection conn) { + conn.transportThread().addConnection(conn.connect()); + } + + public void connectLater(Connection conn) { + Executor executor = this.executor.get(); + if (executor != null) { try { - if (executor.awaitTermination(60, TimeUnit.SECONDS)) { - return; - } - } catch (InterruptedException e) {} + executor.execute(() -> connect(conn)); + return; + } catch (RejectedExecutionException ignored) { + } + } + conn.transportThread().addConnection(conn); + } + + @Override + public void close() { + CachedThreadPoolWithFallback toShutdown = executor.getAndSet(null); + if (toShutdown != null) { + release(toShutdown); } } } diff --git a/jrt/src/com/yahoo/jrt/Supervisor.java b/jrt/src/com/yahoo/jrt/Supervisor.java index 09360c2da7b..d4168e97743 100644 --- a/jrt/src/com/yahoo/jrt/Supervisor.java +++ b/jrt/src/com/yahoo/jrt/Supervisor.java @@ -1,7 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.jrt; - import java.util.HashMap; import java.util.concurrent.atomic.AtomicReference; @@ -103,19 +102,6 @@ public class Supervisor { } /** - * Remove a method from the set of methods held by this Supervisor - * - * @param methodName name of the method to remove - **/ - public void removeMethod(String methodName) { - synchronized (methodMapLock) { - HashMap<String, Method> newMap = new HashMap<>(methodMap()); - newMap.remove(methodName); - methodMap.setRelease(newMap); - } - } - - /** * Remove a method from the set of methods held by this * Supervisor. Use this if you know exactly which method to remove * and not only the name. diff --git a/jrt/src/com/yahoo/jrt/Transport.java b/jrt/src/com/yahoo/jrt/Transport.java index 6f5a381fd6b..8abd3942a39 100644 --- a/jrt/src/com/yahoo/jrt/Transport.java +++ b/jrt/src/com/yahoo/jrt/Transport.java @@ -4,7 +4,6 @@ package com.yahoo.jrt; import java.nio.channels.SocketChannel; import java.util.ArrayList; -import java.util.Iterator; import java.util.Random; import java.util.concurrent.atomic.AtomicInteger; import java.util.logging.Level; @@ -19,7 +18,7 @@ import java.util.logging.Logger; **/ public class Transport { - private static Logger log = Logger.getLogger(Transport.class.getName()); + private static final Logger log = Logger.getLogger(Transport.class.getName()); private final FatalErrorHandler fatalHandler; // NB: this must be set first private final CryptoEngine cryptoEngine; @@ -28,7 +27,7 @@ public class Transport { private final AtomicInteger runCnt; private final TransportMetrics metrics = TransportMetrics.getInstance(); - private final ArrayList<TransportThread> threads = new ArrayList<TransportThread>(); + private final ArrayList<TransportThread> threads = new ArrayList<>(); private final Random rnd = new Random(); /** @@ -174,7 +173,7 @@ public class Transport { * @return this object, to enable chaining with join **/ public Transport shutdown() { - connector.shutdown().join(); + connector.close(); for (TransportThread thread: threads) { thread.shutdown(); } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java index 7c0e0e7868b..efb2a71264a 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java @@ -430,7 +430,11 @@ public final class Node { /** Returns whether this is a state where the node is assigned to an application */ public boolean isAllocated() { - return this == reserved || this == active || this == inactive || this == failed || this == parked; + return allocatedStates().contains(this); + } + + public static Set<State> allocatedStates() { + return Set.of(reserved, active, inactive, failed, parked); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java index 72be68a7ee3..91c15cdb61b 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java @@ -121,8 +121,7 @@ class Preparer { */ private int findHighestIndex(ApplicationId application, ClusterSpec cluster) { int highestIndex = -1; - for (Node node : nodeRepository.getNodes(application, - Node.State.active, Node.State.inactive, Node.State.parked, Node.State.failed)) { + for (Node node : nodeRepository.getNodes(application, Node.State.allocatedStates().toArray(new Node.State[0]))) { ClusterSpec nodeCluster = node.allocation().get().membership().cluster(); if ( ! nodeCluster.id().equals(cluster.id())) continue; if ( ! nodeCluster.type().equals(cluster.type())) continue; diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisionTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisionTest.java index 2b770625060..677aaf93336 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisionTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisionTest.java @@ -6,10 +6,13 @@ import com.yahoo.component.Version; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.Capacity; import com.yahoo.config.provision.ClusterSpec; +import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.Flavor; import com.yahoo.config.provision.HostSpec; import com.yahoo.config.provision.NodeResources; import com.yahoo.config.provision.NodeType; +import com.yahoo.config.provision.RegionName; +import com.yahoo.config.provision.Zone; import com.yahoo.vespa.flags.Flags; import com.yahoo.vespa.flags.InMemoryFlagSource; import com.yahoo.vespa.hosted.provision.Node; @@ -18,11 +21,15 @@ import com.yahoo.vespa.hosted.provision.node.IP; import com.yahoo.vespa.hosted.provision.testutils.MockNameResolver; import org.junit.Test; +import java.time.Instant; import java.util.List; import java.util.Set; +import java.util.function.Function; import java.util.stream.Collectors; +import java.util.stream.IntStream; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; @@ -103,6 +110,41 @@ public class DynamicDockerProvisionTest { assertEquals(4, tester.nodeRepository().getNodes(NodeType.tenant, Node.State.reserved).size()); } + @Test + public void node_indices_are_unique_even_when_a_node_is_left_in_reserved_state() { + ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east"))).build(); + NodeResources resources = new NodeResources(10, 10, 10, 10); + ApplicationId app = tester.makeApplicationId(); + + Function<Node, Node> retireNode = node -> + tester.nodeRepository().write(node.withWantToRetire(true, Agent.system, Instant.now()), () -> {}); + Function<Integer, Node> getNodeInGroup = group -> tester.nodeRepository().getNodes(app).stream() + .filter(node -> node.allocation().get().membership().cluster().group().get().index() == group) + .findAny().orElseThrow(); + + // Allocate 10 hosts + tester.makeReadyNodes(10, resources, NodeType.host, 1); + tester.deployZoneApp(); + + // Prepare & activate an application with 8 nodes and 2 groups + tester.activate(app, tester.prepare(app, clusterSpec("content"), 8, 2, resources)); + + // Retire a node in group 1 and prepare the application + retireNode.apply(getNodeInGroup.apply(1)); + tester.prepare(app, clusterSpec("content"), 8, 2, resources); + // App is not activated, to leave node '8' in reserved state + + // Retire a node in group 0 and prepare the application + retireNode.apply(getNodeInGroup.apply(0)); + tester.prepare(app, clusterSpec("content"), 8, 2, resources); + + // Verify that nodes have unique indices from 0..9 + var indices = tester.nodeRepository().getNodes(app).stream() + .map(node -> node.allocation().get().membership().index()) + .collect(Collectors.toSet()); + assertTrue(indices.containsAll(IntStream.range(0, 10).boxed().collect(Collectors.toList()))); + } + private static void deployZoneApp(ProvisioningTester tester) { ApplicationId applicationId = tester.makeApplicationId(); List<HostSpec> list = tester.prepare(applicationId, diff --git a/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceModelCache.java b/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceModelCache.java index c50f5e6c2d5..1b37555a554 100644 --- a/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceModelCache.java +++ b/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceModelCache.java @@ -17,6 +17,7 @@ public class ServiceModelCache implements Supplier<ServiceModel> { private final Supplier<ServiceModel> expensiveSupplier; private final Timer timer; + private final boolean useCache; private volatile ServiceModel snapshot; private boolean updatePossiblyInProgress = false; @@ -24,13 +25,18 @@ public class ServiceModelCache implements Supplier<ServiceModel> { private final Object updateMonitor = new Object(); private long snapshotMillis; - public ServiceModelCache(Supplier<ServiceModel> expensiveSupplier, Timer timer) { + public ServiceModelCache(Supplier<ServiceModel> expensiveSupplier, Timer timer, boolean useCache) { this.expensiveSupplier = expensiveSupplier; this.timer = timer; + this.useCache = useCache; } @Override public ServiceModel get() { + if (!useCache) { + return expensiveSupplier.get(); + } + if (snapshot == null) { synchronized (updateMonitor) { if (snapshot == null) { diff --git a/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceMonitorImpl.java b/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceMonitorImpl.java index 0a40555036c..67b4e890c29 100644 --- a/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceMonitorImpl.java +++ b/service-monitor/src/main/java/com/yahoo/vespa/service/model/ServiceMonitorImpl.java @@ -5,16 +5,12 @@ import com.google.inject.Inject; import com.yahoo.config.provision.Zone; import com.yahoo.jdisc.Metric; import com.yahoo.jdisc.Timer; -import com.yahoo.vespa.applicationmodel.ApplicationInstance; -import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; +import com.yahoo.vespa.flags.FlagSource; +import com.yahoo.vespa.flags.Flags; import com.yahoo.vespa.service.duper.DuperModelManager; -import com.yahoo.vespa.service.health.HealthMonitorManager; import com.yahoo.vespa.service.manager.UnionMonitorManager; import com.yahoo.vespa.service.monitor.ServiceModel; import com.yahoo.vespa.service.monitor.ServiceMonitor; -import com.yahoo.vespa.service.slobrok.SlobrokMonitorManagerImpl; - -import java.util.Map; public class ServiceMonitorImpl implements ServiceMonitor { @@ -25,7 +21,8 @@ public class ServiceMonitorImpl implements ServiceMonitor { UnionMonitorManager monitorManager, Metric metric, Timer timer, - Zone zone) { + Zone zone, + FlagSource flagSource) { duperModelManager.registerListener(monitorManager); ServiceModelProvider uncachedServiceModelProvider = new ServiceModelProvider( @@ -34,7 +31,8 @@ public class ServiceMonitorImpl implements ServiceMonitor { duperModelManager, new ModelGenerator(), zone); - serviceModelProvider = new ServiceModelCache(uncachedServiceModelProvider, timer); + boolean cache = Flags.SERVICE_MODEL_CACHE.bindTo(flagSource).value(); + serviceModelProvider = new ServiceModelCache(uncachedServiceModelProvider, timer, cache); } @Override diff --git a/service-monitor/src/test/java/com/yahoo/vespa/service/model/ServiceModelCacheTest.java b/service-monitor/src/test/java/com/yahoo/vespa/service/model/ServiceModelCacheTest.java index 2d6921df374..c2314be1e0f 100644 --- a/service-monitor/src/test/java/com/yahoo/vespa/service/model/ServiceModelCacheTest.java +++ b/service-monitor/src/test/java/com/yahoo/vespa/service/model/ServiceModelCacheTest.java @@ -18,7 +18,7 @@ public class ServiceModelCacheTest { @SuppressWarnings("unchecked") private final Supplier<ServiceModel> rawSupplier = mock(Supplier.class); private final Timer timer = mock(Timer.class); - private final ServiceModelCache cache = new ServiceModelCache(rawSupplier, timer); + private final ServiceModelCache cache = new ServiceModelCache(rawSupplier, timer, true); @Test public void sanityCheck() { diff --git a/vespajlib/src/main/java/com/yahoo/concurrent/CachedThreadPoolWithFallback.java b/vespajlib/src/main/java/com/yahoo/concurrent/CachedThreadPoolWithFallback.java new file mode 100644 index 00000000000..42e86aad1ba --- /dev/null +++ b/vespajlib/src/main/java/com/yahoo/concurrent/CachedThreadPoolWithFallback.java @@ -0,0 +1,63 @@ +// Copyright 2020 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +package com.yahoo.concurrent; + +import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.RejectedExecutionException; +import java.util.concurrent.SynchronousQueue; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; + +/** + * An executor that will first try a bounded cached threadpool before falling back to a unbounded + * single threaded threadpool that will take over dispatching to the primary pool. + * + */ +public class CachedThreadPoolWithFallback implements AutoCloseable, Executor { + private final ExecutorService primary; + private final ExecutorService secondary; + public CachedThreadPoolWithFallback(String baseName, int corePoolSize, int maximumPoolSize, long keepAlimeTime, TimeUnit timeUnit) { + primary = new ThreadPoolExecutor(corePoolSize, maximumPoolSize, keepAlimeTime, timeUnit, + new SynchronousQueue<>(), ThreadFactoryFactory.getDaemonThreadFactory(baseName + ".primary")); + secondary = Executors.newSingleThreadExecutor(ThreadFactoryFactory.getDaemonThreadFactory(baseName + ".secondary")); + } + @Override + public void execute(Runnable command) { + try { + primary.execute(command); + } catch (RejectedExecutionException e1) { + secondary.execute(() -> retryForever(command)); + } + } + private void retryForever(Runnable command) { + while (true) { + try { + primary.execute(command); + return; + } catch (RejectedExecutionException rejected) { + try { + Thread.sleep(1); + } catch (InterruptedException silenced) { } + } + } + } + + @Override + public void close() { + secondary.shutdown(); + join(secondary); + primary.shutdown(); + join(primary); + } + private static void join(ExecutorService executor) { + while (true) { + try { + if (executor.awaitTermination(60, TimeUnit.SECONDS)) { + return; + } + } catch (InterruptedException e) {} + } + } +} diff --git a/vespajlib/src/test/java/com/yahoo/concurrent/CachedThreadPoolWithFallbackTest.java b/vespajlib/src/test/java/com/yahoo/concurrent/CachedThreadPoolWithFallbackTest.java new file mode 100644 index 00000000000..52e17631a34 --- /dev/null +++ b/vespajlib/src/test/java/com/yahoo/concurrent/CachedThreadPoolWithFallbackTest.java @@ -0,0 +1,43 @@ +// Copyright 2020 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +package com.yahoo.concurrent; + +import org.junit.Test; + +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; + +import static org.junit.Assert.assertEquals; + +public class CachedThreadPoolWithFallbackTest { + private static void countAndBlock(AtomicLong counter, long waitLimit) { + counter.incrementAndGet(); + try { + synchronized (counter) { + while (counter.get() < waitLimit) { + counter.wait(); + } + } + } catch (InterruptedException e) {} + } + + @Test + public void testThatTaskAreQueued() throws InterruptedException { + CachedThreadPoolWithFallback executor = new CachedThreadPoolWithFallback("test", 1, 30, 1, TimeUnit.SECONDS); + AtomicLong counter = new AtomicLong(0); + for (int i = 0; i < 1000; i++) { + executor.execute(() -> countAndBlock(counter, 100)); + } + while (counter.get() < 30) { + Thread.sleep(1); + } + Thread.sleep(1); + assertEquals(30L, counter.get()); + counter.set(100); + synchronized (counter) { + counter.notifyAll(); + } + executor.close(); + assertEquals(1070L, counter.get()); + } +} |