1452694: Candlepin no longer deletes pools for custom subscriptions (2.0)

server/spec/import_spec.rb

@@ -120,7 +120,8 @@ def import_and_wait
     it 'can be undone' do
       # Make a custom pool so we can be sure it does not get wiped
       # out during either the undo or a subsequent re-import:
-      prod = create_product(random_string(), random_string(), {:owner => @import_owner['key']})
+      prod_name = random_string("custom_pool_prod-")
+      prod = create_product(prod_name, prod_name, {:owner => @import_owner['key']})
       custom_pool = @cp.create_pool(@import_owner['key'], prod['id'])
 
       job = @import_owner_client.undo_import(@import_owner['key'])

server/spec/rules_import_spec.rb

@@ -11,7 +11,7 @@
     # ones that may have been left in the database:
     @cp.delete_rules
 
-    sleep 6 #The status resource response is being cached for 5 seconds
+    sleep 7 # The status resource response is being cached for 5 seconds
     @orig_ver = @cp.get_status()['rulesVersion']
 
     rules_version_parts = @orig_ver.split(".")
@@ -44,7 +44,7 @@ def upload_dummy_rules
     fetched_rules = @cp.list_rules
     decoded_fetched_rules = Base64.decode64(fetched_rules)
     (decoded_fetched_rules == @rules).should be true
-    sleep 6 #The status resource response is being cached for 5 seconds
+    sleep 7 # The status resource response is being cached for 5 seconds
 
     @cp.get_status()['rulesVersion'].should == @new_ver
   end
@@ -60,7 +60,7 @@ def upload_dummy_rules
     rules = @cp.list_rules
 
     # Version should be back to original:
-    sleep 6 #The status resource response is being cached for 5 seconds
+    sleep 7 # The status resource response is being cached for 5 seconds
 
     @cp.get_status()['rulesVersion'].should == @orig_ver
 

server/src/main/java/org/candlepin/controller/CandlepinPoolManager.java

@@ -342,23 +342,8 @@ void refreshPoolsWithRegeneration(SubscriptionServiceAdapter subAdapter, Owner o
         log.debug("Deleting pools for absent subscriptions...");
         List<Pool> poolsToDelete = new ArrayList<Pool>();
 
-        // BZ 1452694: Don't delete pools for custom subscriptions
-        // We need to verify that we aren't deleting pools that are created via the API.
-        // Unfortunately, we don't have a 100% reliable way of detecting such pools at this point,
-        // so we'll do the next best thing: In standalone, pools with an upstream pool ID are those
-        // we've received from an import (and, thus, are eligible for deletion). In hosted,
-        // however, we *are* the upstream source, so everything is eligible for removal.
-        // This is pretty hacky, so the way we go about doing this check should eventually be
-        // replaced with something more generic and reliable, and not dependent on the config.
-
-        // TODO:
-        // Remove the standalone config check and replace it with a check for whether or not the
-        // pool is "managed"  -- however we decide to implement that in the future.
-        boolean standalone = config.getBoolean(ConfigProperties.STANDALONE, true);
         for (Pool pool : poolCurator.getPoolsFromBadSubs(owner, subscriptionMap.keySet())) {
-            if ((!standalone || pool.getUpstreamPoolId() != null) && pool.getSourceSubscription() != null &&
-                !pool.getType().isDerivedType()) {
-
+            if (this.isManaged(pool)) {
                 poolsToDelete.add(pool);
             }
         }
@@ -2389,4 +2374,26 @@ public void recalculatePoolQuantitiesForOwner(Owner owner) {
         poolCurator.calculateConsumedForOwnersPools(owner);
         poolCurator.calculateExportedForOwnersPools(owner);
     }
+
+    /**
+     * @{inheritDocs}
+     */
+    @Override
+    public boolean isManaged(Pool pool) {
+        // BZ 1452694: Don't delete pools for custom subscriptions
+        // We need to verify that we aren't deleting pools that are created via the API.
+        // Unfortunately, we don't have a 100% reliable way of detecting such pools at this point,
+        // so we'll do the next best thing: In standalone, pools with an upstream pool ID are those
+        // we've received from an import (and, thus, are eligible for deletion). In hosted,
+        // however, we *are* the upstream source, so everything is eligible for removal.
+        // This is pretty hacky, so the way we go about doing this check should eventually be
+        // replaced with something more generic and reliable, and not dependent on the config.
+
+        // TODO:
+        // Remove the standalone config check and replace it with a check for whether or not the
+        // pool is non-custom  -- however we decide to implement that in the future.
+
+        return pool != null && pool.getSourceSubscription() != null && !pool.getType().isDerivedType() &&
+            (pool.getUpstreamPoolId() != null || !this.config.getBoolean(ConfigProperties.STANDALONE, true));
+    }
 }

server/src/main/java/org/candlepin/controller/PoolManager.java

@@ -331,4 +331,15 @@ List<Entitlement> entitleByProductsForHost(Consumer guest, Consumer host,
     void deletePools(Collection<Pool> pools);
 
     void deletePools(Collection<Pool> pools, Set<String> alreadyDeletedPools);
+
+    /**
+     * Checks whether or not the given pool is a managed (that is, non-custom) pool.
+     *
+     * @param pool
+     *  The pool to check
+     *
+     * @return
+     *  true if the pool is a managed pool; false otherwise
+     */
+    boolean isManaged(Pool pool);
 }

server/src/main/java/org/candlepin/liquibase/PerOrgProductsMigrationTask.java

@@ -737,5 +737,32 @@ protected void migrateRelatedData() throws DatabaseException, SQLException {
             "SELECT id, subscriptionid, subscriptionsubkey, pool_id, created, updated " +
             "FROM cp_pool_source_sub "
         );
+
+        // Migrate upstream tracking columns from subscription to pool
+        ResultSet subscriptionInfo = this.executeQuery(
+            "SELECT ss.pool_id, s.cdn_id, s.certificate_id, s.upstream_entitlement_id, " +
+            "  s.upstream_consumer_id, s.upstream_pool_id " +
+            "FROM cp_subscription s " +
+            "JOIN cp2_pool_source_sub ss ON s.id = ss.subscription_id"
+        );
+
+        // Update any pool referencing this subscription...
+        while (subscriptionInfo.next()) {
+            String poolId = subscriptionInfo.getString(1);
+            String cdnId = subscriptionInfo.getString(2);
+            String certId = subscriptionInfo.getString(3);
+            String upstreamEntitlementId = subscriptionInfo.getString(4);
+            String upstreamConsumerId = subscriptionInfo.getString(5);
+            String upstreamPoolId = subscriptionInfo.getString(6);
+
+            int updated = this.executeUpdate(
+                "UPDATE cp_pool SET cdn_id=?, certificate_id=?, upstream_entitlement_id=?, " +
+                "  upstream_consumer_id=?, upstream_pool_id=? " +
+                "WHERE id=?",
+                cdnId, certId, upstreamEntitlementId, upstreamConsumerId, upstreamPoolId, poolId
+            );
+        }
+
+        subscriptionInfo.close();
     }
 }

server/src/main/java/org/candlepin/pinsetter/tasks/UndoImportsJob.java

@@ -131,7 +131,7 @@ public void toExecute(JobExecutionContext context) throws JobExecutionException
 
             List<Pool> pools = this.poolManager.listPoolsByOwner(owner).list();
             for (Pool pool : pools) {
-                if (pool.getSourceSubscription() != null && !pool.getType().isDerivedType()) {
+                if (this.poolManager.isManaged(pool)) {
                     this.poolManager.deletePool(pool);
                 }
             }

server/src/main/java/org/candlepin/policy/js/JsRunnerProvider.java

@@ -112,7 +112,7 @@ public void compileRules(boolean forceRefresh) {
                 return;
             }
 
-            log.info("Recompiling rules with timestamp: " + newUpdated);
+            log.info("Recompiling rules with timestamp: {}", newUpdated);
 
             Context context = Context.enter();
             context.setOptimizationLevel(9);

server/src/main/java/org/candlepin/policy/js/pool/PoolHelper.java

@@ -241,6 +241,15 @@ public static Pool clonePool(Pool sourcePool, Product product, String quantity,
             pool.getBranding().add(new Branding(brand.getProductId(), brand.getType(), brand.getName()));
         }
 
+        // Copy upstream fields
+        // Impl note/TODO:
+        // We are only doing this to facilitate marking pools derived from an upstream source/manifest
+        // as also from that same upstream source. A proper pool hierarchy would be a better solution
+        // here, but this will work for the interim.
+        pool.setUpstreamPoolId(sourcePool.getUpstreamPoolId());
+        pool.setUpstreamEntitlementId(sourcePool.getUpstreamEntitlementId());
+        pool.setUpstreamConsumerId(sourcePool.getUpstreamConsumerId());
+
         return pool;
     }
 

server/src/main/java/org/candlepin/sync/Exporter.java

@@ -213,8 +213,7 @@ private File makeArchive(Consumer consumer, File tempDir, File exportDir)
         log.info("Creating archive of " + exportDir.getAbsolutePath() + " in: " +
             exportFileName);
 
-        File archive = createZipArchiveWithDir(
-            tempDir, exportDir, "consumer_export.zip",
+        File archive = createZipArchiveWithDir(tempDir, exportDir, "consumer_export.zip",
             "Candlepin export for " + consumer.getUuid());
 
         InputStream archiveInputStream = null;

server/src/test/java/org/candlepin/controller/PoolManagerTest.java

@@ -78,15 +78,18 @@
 import org.candlepin.test.TestUtil;
 import org.candlepin.util.Util;
 
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.ArgumentCaptor;
 import org.mockito.Captor;
 import org.mockito.Mock;
 import org.mockito.Mockito;
+import org.mockito.MockitoAnnotations;
 import org.mockito.invocation.InvocationOnMock;
-import org.mockito.runners.MockitoJUnitRunner;
 import org.mockito.stubbing.Answer;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -111,7 +114,7 @@
 /**
  * PoolManagerTest
  */
-@RunWith(MockitoJUnitRunner.class)
+@RunWith(JUnitParamsRunner.class)
 public class PoolManagerTest {
     private static Logger log = LoggerFactory.getLogger(PoolManagerTest.class);
 
@@ -157,6 +160,8 @@ public class PoolManagerTest {
 
     @Before
     public void init() throws Exception {
+        MockitoAnnotations.initMocks(this);
+
         i18n = I18nFactory.getI18n(getClass(), Locale.US, I18nFactory.FALLBACK);
 
         owner = TestUtil.createOwner("key", "displayname");
@@ -1867,4 +1872,120 @@ private void mockOwner(Owner owner) {
         }
     }
 
+    // TODO:
+    // Refactor these tests when isManaged is refactored to not be reliant upon the config
+    public Object[][] getParametersForIsManagedTests() {
+        SourceSubscription srcSub = new SourceSubscription("test_sub_id", "test_sub_key");
+
+        return new Object[][] {
+            // Standalone tests
+            new Object[] { PoolType.NORMAL, null, null, false, false },
+            new Object[] { PoolType.ENTITLEMENT_DERIVED, null, null, false, false },
+            new Object[] { PoolType.STACK_DERIVED, null, null, false, false },
+            new Object[] { PoolType.BONUS, null, null, false, false },
+            new Object[] { PoolType.UNMAPPED_GUEST, null, null, false, false },
+            new Object[] { PoolType.DEVELOPMENT, null, null, false, false },
+
+            new Object[] { PoolType.NORMAL, srcSub, null, false, false },
+            new Object[] { PoolType.ENTITLEMENT_DERIVED, srcSub, null, false, false },
+            new Object[] { PoolType.STACK_DERIVED, srcSub, null, false, false },
+            new Object[] { PoolType.BONUS, srcSub, null, false, false },
+            new Object[] { PoolType.UNMAPPED_GUEST, srcSub, null, false, false },
+            new Object[] { PoolType.DEVELOPMENT, srcSub, null, false, false },
+
+            new Object[] { PoolType.NORMAL, null, "upstream_pool_id", false, false },
+            new Object[] { PoolType.ENTITLEMENT_DERIVED, null, "upstream_pool_id", false, false },
+            new Object[] { PoolType.STACK_DERIVED, null, "upstream_pool_id", false, false },
+            new Object[] { PoolType.BONUS, null, "upstream_pool_id", false, false },
+            new Object[] { PoolType.UNMAPPED_GUEST, null, "upstream_pool_id", false, false },
+            new Object[] { PoolType.DEVELOPMENT, null, "upstream_pool_id", false, false },
+
+            new Object[] { PoolType.NORMAL, srcSub, "upstream_pool_id", false, true },
+            new Object[] { PoolType.ENTITLEMENT_DERIVED, srcSub, "upstream_pool_id", false, false },
+            new Object[] { PoolType.STACK_DERIVED, srcSub, "upstream_pool_id", false, false },
+            new Object[] { PoolType.BONUS, srcSub, "upstream_pool_id", false, true },
+            new Object[] { PoolType.UNMAPPED_GUEST, srcSub, "upstream_pool_id", false, true },
+            new Object[] { PoolType.DEVELOPMENT, srcSub, "upstream_pool_id", false, true },
+
+            // Hosted tests
+            new Object[] { PoolType.NORMAL, null, null, true, false },
+            new Object[] { PoolType.ENTITLEMENT_DERIVED, null, null, true, false },
+            new Object[] { PoolType.STACK_DERIVED, null, null, true, false },
+            new Object[] { PoolType.BONUS, null, null, true, false },
+            new Object[] { PoolType.UNMAPPED_GUEST, null, null, true, false },
+            new Object[] { PoolType.DEVELOPMENT, null, null, true, false },
+
+            new Object[] { PoolType.NORMAL, srcSub, null, true, true },
+            new Object[] { PoolType.ENTITLEMENT_DERIVED, srcSub, null, true, false },
+            new Object[] { PoolType.STACK_DERIVED, srcSub, null, true, false },
+            new Object[] { PoolType.BONUS, srcSub, null, true, true },
+            new Object[] { PoolType.UNMAPPED_GUEST, srcSub, null, true, true },
+            new Object[] { PoolType.DEVELOPMENT, srcSub, null, true, true },
+
+            new Object[] { PoolType.NORMAL, null, "upstream_pool_id", true, false },
+            new Object[] { PoolType.ENTITLEMENT_DERIVED, null, "upstream_pool_id", true, false },
+            new Object[] { PoolType.STACK_DERIVED, null, "upstream_pool_id", true, false },
+            new Object[] { PoolType.BONUS, null, "upstream_pool_id", true, false },
+            new Object[] { PoolType.UNMAPPED_GUEST, null, "upstream_pool_id", true, false },
+            new Object[] { PoolType.DEVELOPMENT, null, "upstream_pool_id", true, false },
+
+            new Object[] { PoolType.NORMAL, srcSub, "upstream_pool_id", true, true },
+            new Object[] { PoolType.ENTITLEMENT_DERIVED, srcSub, "upstream_pool_id", true, false },
+            new Object[] { PoolType.STACK_DERIVED, srcSub, "upstream_pool_id", true, false },
+            new Object[] { PoolType.BONUS, srcSub, "upstream_pool_id", true, true },
+            new Object[] { PoolType.UNMAPPED_GUEST, srcSub, "upstream_pool_id", true, true },
+            new Object[] { PoolType.DEVELOPMENT, srcSub, "upstream_pool_id", true, true },
+        };
+    }
+
+    @Test
+    public void testIsManagedWithNullPool() {
+        assertFalse(manager.isManaged(null));
+    }
+
+    @Test
+    @Parameters(method = "getParametersForIsManagedTests")
+    public void testIsManaged(PoolType type, SourceSubscription srcSub, String upstreamPoolId, boolean hosted,
+        boolean expected) {
+
+        Pool pool = TestUtil.createPool(owner, product);
+        when(mockConfig.getBoolean(eq(ConfigProperties.STANDALONE))).thenReturn(!hosted);
+        when(mockConfig.getBoolean(eq(ConfigProperties.STANDALONE), anyBoolean())).thenReturn(!hosted);
+
+        pool.setSourceSubscription(srcSub);
+        pool.setUpstreamPoolId(upstreamPoolId);
+
+        switch (type) {
+            case UNMAPPED_GUEST:
+                pool.setAttribute(Pool.Attributes.DERIVED_POOL, "true");
+                pool.setAttribute(Pool.Attributes.UNMAPPED_GUESTS_ONLY, "true");
+                break;
+
+            case ENTITLEMENT_DERIVED:
+                pool.setAttribute(Pool.Attributes.DERIVED_POOL, "true");
+                pool.setSourceEntitlement(new Entitlement());
+                break;
+
+            case STACK_DERIVED:
+                pool.setAttribute(Pool.Attributes.DERIVED_POOL, "true");
+                pool.setSourceStack(new SourceStack());
+                break;
+
+            case BONUS:
+                pool.setAttribute(Pool.Attributes.DERIVED_POOL, "true");
+                break;
+
+            case DEVELOPMENT:
+                pool.setAttribute(Pool.Attributes.DEVELOPMENT_POOL, "true");
+                break;
+
+            case NORMAL:
+            default:
+                // Nothing to do here
+        }
+
+        boolean output = manager.isManaged(pool);
+        assertEquals(expected, output);
+    }
+
 }

server/src/test/java/org/candlepin/pinsetter/tasks/UndoImportsJobTest.java

@@ -209,6 +209,9 @@ protected Pool createPool(String name, Owner owner, boolean keepSourceSub, PoolT
         if (!keepSourceSub) {
             pool.setSourceSubscription(null);
         }
+        else {
+            pool.setUpstreamPoolId("upstream_pool_id");
+        }
 
         this.poolCurator.create(pool);