Tombstone tenants stuck in 'none' plan for a long time

4 files changed, 77 insertions, 36 deletions

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 59877fce634..384a5d0f1ac 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
@@ -171,7 +171,15 @@ public class TenantController {
                     throw new IllegalArgumentException("Could not delete tenant '" + tenant.value()
                             + "': This tenant has active applications");
 
-                credentials.ifPresent(creds -> accessControl.deleteTenant(tenant, creds));
+                if (oldTenant.type() == Tenant.Type.athenz) {
+                    credentials.ifPresent(creds -> accessControl.deleteTenant(tenant, creds));
+                } else if (oldTenant.type() == Tenant.Type.cloud) {
+                    accessControl.deleteTenant(tenant, null);
+                } else {
+                    throw new IllegalArgumentException("Could not delete tenant '" + tenant.value()
+                            + ": This tenant is of unhandled type " + oldTenant.type());
+                }
+
                 controller.notificationsDb().removeNotifications(NotificationSource.from(tenant));
             }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/CloudTrialExpirer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/CloudTrialExpirer.java
index c9310349b9b..50a73ce214d 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/CloudTrialExpirer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/CloudTrialExpirer.java
@@ -7,17 +7,24 @@ import com.yahoo.vespa.flags.ListFlag;
 import com.yahoo.vespa.flags.PermanentFlags;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.api.integration.billing.PlanId;
+import com.yahoo.vespa.hosted.controller.application.Deployment;
 import com.yahoo.vespa.hosted.controller.tenant.LastLoginInfo;
 import com.yahoo.vespa.hosted.controller.tenant.Tenant;
 
 import java.time.Duration;
+import java.time.Instant;
 import java.util.List;
+import java.util.Optional;
+import java.util.function.Consumer;
+import java.util.function.Predicate;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
 /**
  * Expires unused tenants from Vespa Cloud.
  *
+ * TODO: Should support sending notifications some time before the various expiry events happen.
+ *
  * @author ogronnesby
  */
 public class CloudTrialExpirer extends ControllerMaintainer {
@@ -33,39 +40,56 @@ public class CloudTrialExpirer extends ControllerMaintainer {
 
     @Override
     protected double maintain() {
-        var expiredTenants = controller().tenants().asList().stream()
-                .filter(this::tenantIsCloudTenant)           // only valid for cloud tenants
-                .filter(this::tenantHasTrialPlan)            // only valid to expire actual trial tenants
-                .filter(this::tenantIsNotExemptFromExpiry)   // feature flag might exempt tenant from expiry
-                .filter(this::tenantReadersNotLoggedIn)      // no user logged in last 14 days
-                .filter(this::tenantHasNoDeployments)        // no running deployments active
-                .collect(Collectors.toList());
+        if (controller().system().equals(SystemName.PublicCd)) {
+            tombstoneNonePlanTenants();
+        }
+        moveInactiveTenantsToNonePlan();
+        return 1;
+    }
 
-        if (! expiredTenants.isEmpty()) {
-            var expiredTenantNames = expiredTenants.stream()
-                    .map(Tenant::name)
-                    .map(TenantName::value)
-                    .collect(Collectors.joining(", "));
+    private void moveInactiveTenantsToNonePlan() {
+        var predicate = tenantReadersNotLoggedIn(loginExpiry)
+                .and(this::tenantHasTrialPlan)
+                .and(this::tenantHasNoDeployments);
 
-            log.info("Moving expired tenants to 'none' plan: " + expiredTenantNames);
-        }
+        forTenant("'none' plan", predicate, this::setPlanNone);
+    }
 
-        expireTenants(expiredTenants);
+    private void tombstoneNonePlanTenants() {
+        // tombstone tenants that are inactive 14 days after being set as 'none'
+        var expiry = loginExpiry.plus(loginExpiry);
+        var predicate = tenantReadersNotLoggedIn(expiry).and(this::tenantHasNonePlan);
+        forTenant("tombstoned", predicate, this::tombstoneTenants);
+    }
 
-        return 1;
+    private void forTenant(String name, Predicate<Tenant> p, Consumer<List<Tenant>> c) {
+        var predicate = ((Predicate<Tenant>) this::tenantIsCloudTenant)
+                .and(this::tenantIsNotExemptFromExpiry);
+
+        var tenants = controller().tenants().asList().stream()
+                .filter(predicate.and(p))
+                .collect(Collectors.toList());
+
+        if (! tenants.isEmpty()) {
+            var tenantNames = tenants.stream().map(Tenant::name).map(TenantName::value).collect(Collectors.joining(", "));
+            log.info("Setting tenants as " + name + ": " + tenantNames);
+        }
+
+        c.accept(tenants);
     }
 
     private boolean tenantIsCloudTenant(Tenant tenant) {
         return tenant.type() == Tenant.Type.cloud;
     }
 
-    private boolean tenantReadersNotLoggedIn(Tenant tenant) {
-        return tenant.lastLoginInfo().get(LastLoginInfo.UserLevel.user)
-                .map(instant -> {
-                    var sinceLastLogin = Duration.between(instant, controller().clock().instant());
-                    return sinceLastLogin.compareTo(loginExpiry) > 0;
-                })
-                .orElse(false);
+    private Predicate<Tenant> tenantReadersNotLoggedIn(Duration duration) {
+        // returns true if no user has logged in to the tenant after (now - duration)
+        return (Tenant tenant) -> {
+            var timeLimit = controller().clock().instant().minus(duration);
+            return tenant.lastLoginInfo().get(LastLoginInfo.UserLevel.user)
+                    .map(instant -> instant.isBefore(timeLimit))
+                    .orElse(false);
+        };
     }
 
     private boolean tenantHasTrialPlan(Tenant tenant) {
@@ -73,6 +97,11 @@ public class CloudTrialExpirer extends ControllerMaintainer {
         return "trial".equals(planId.value());
     }
 
+    private boolean tenantHasNonePlan(Tenant tenant) {
+        var planId = controller().serviceRegistry().billingController().getPlan(tenant.name());
+        return "none".equals(planId.value());
+    }
+
     private boolean tenantIsNotExemptFromExpiry(Tenant tenant) {
         return ! extendedTrialTenants.value().contains(tenant.name().value());
     }
@@ -84,9 +113,15 @@ public class CloudTrialExpirer extends ControllerMaintainer {
                 .sum() == 0;
     }
 
-    private void expireTenants(List<Tenant> tenants) {
+    private void setPlanNone(List<Tenant> tenants) {
         tenants.forEach(tenant -> {
             controller().serviceRegistry().billingController().setPlan(tenant.name(), PlanId.from("none"), false);
         });
     }
+
+    private void tombstoneTenants(List<Tenant> tenants) {
+        tenants.forEach(tenant -> {
+            controller().tenants().delete(tenant.name(), Optional.empty(), false);
+        });
+    }
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/security/CloudAccessControl.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/security/CloudAccessControl.java
index de70fc54d3b..39ee2a6ce44 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/security/CloudAccessControl.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/security/CloudAccessControl.java
@@ -117,7 +117,6 @@ public class CloudAccessControl implements AccessControl {
 
     @Override
     public void deleteTenant(TenantName tenant, Credentials credentials) {
-        deleteBillingInfo(tenant, credentials);
         for (TenantRole role : Roles.tenantRoles(tenant))
             userManagement.deleteRole(role);
     }
@@ -134,13 +133,4 @@ public class CloudAccessControl implements AccessControl {
             userManagement.deleteRole(role);
     }
 
-    private void deleteBillingInfo(TenantName tenant, Credentials credentials) {
-        var users = Roles.tenantRoles(tenant)
-                .stream()
-                .flatMap(role -> userManagement.listUsers(role).stream())
-                .collect(Collectors.toSet());
-        var isPrivileged = allowedByPrivilegedRole((Auth0Credentials) credentials);
-        billingController.deleteBillingInfo(tenant, users, isPrivileged);
-    }
-
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/CloudTrialExpirerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/CloudTrialExpirerTest.java
index c772ca8b8f7..ed00e7f5473 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/CloudTrialExpirerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/CloudTrialExpirerTest.java
@@ -18,12 +18,13 @@ import java.time.Duration;
 import java.util.List;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 /**
  * @author ogronnesby
  */
 public class CloudTrialExpirerTest {
-    private final ControllerTester tester = new ControllerTester(SystemName.Public);
+    private final ControllerTester tester = new ControllerTester(SystemName.PublicCd);
     private final DeploymentTester deploymentTester = new DeploymentTester(tester);
     private final CloudTrialExpirer expirer = new CloudTrialExpirer(tester.controller(), Duration.ofMinutes(5));
 
@@ -35,6 +36,13 @@ public class CloudTrialExpirerTest {
     }
 
     @Test
+    public void tombstone_inactive_none() {
+        registerTenant("none-tenant", "none", Duration.ofDays(28).plusMillis(1));
+        expirer.maintain();
+        assertEquals(Tenant.Type.deleted, tester.controller().tenants().get(TenantName.from("none-tenant"), true).get().type());
+    }
+
+    @Test
     public void keep_inactive_nontrial_tenants() {
         registerTenant("not-a-trial-tenant", "pay-as-you-go", Duration.ofDays(30));
         expirer.maintain();