You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by is...@apache.org on 2019/12/15 05:25:10 UTC

[lucene-solr] branch branch_8x updated: SOLR-13662: Fixes to package manager

This is an automated email from the ASF dual-hosted git repository.

ishan pushed a commit to branch branch_8x
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git


The following commit(s) were added to refs/heads/branch_8x by this push:
     new de39472  SOLR-13662: Fixes to package manager
de39472 is described below

commit de39472f0766170f7a39f45dd4c5396efef96ad4
Author: Ishan Chattopadhyaya <is...@apache.org>
AuthorDate: Sun Dec 15 10:44:02 2019 +0530

    SOLR-13662: Fixes to package manager
    
    * Better logging and error reporting
    * Fixing deploy command to handle previously undeployed packages
    * Test now uses @LogLevel annotation
    * Deploy command had a hard coded collection name by mistake, fix it
---
 .../packagemanager/DefaultPackageRepository.java   |  5 +-
 .../apache/solr/packagemanager/PackageManager.java | 85 ++++++++++++++++------
 .../apache/solr/packagemanager/PackageUtils.java   |  7 ++
 .../solr/packagemanager/RepositoryManager.java     | 30 ++++----
 .../src/java/org/apache/solr/util/PackageTool.java | 18 +++--
 .../apache/solr/cloud/PackageManagerCLITest.java   |  9 +--
 6 files changed, 100 insertions(+), 54 deletions(-)

diff --git a/solr/core/src/java/org/apache/solr/packagemanager/DefaultPackageRepository.java b/solr/core/src/java/org/apache/solr/packagemanager/DefaultPackageRepository.java
index dee55a5..b55dc71 100644
--- a/solr/core/src/java/org/apache/solr/packagemanager/DefaultPackageRepository.java
+++ b/solr/core/src/java/org/apache/solr/packagemanager/DefaultPackageRepository.java
@@ -26,6 +26,7 @@ import java.util.HashMap;
 import java.util.Map;
 
 import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.FilenameUtils;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.apache.solr.common.SolrException;
@@ -82,8 +83,8 @@ public class DefaultPackageRepository extends PackageRepository {
   public Path download(String artifactName) throws SolrException, IOException {
     Path tmpDirectory = Files.createTempDirectory("solr-packages");
     tmpDirectory.toFile().deleteOnExit();
-    URL url = new URL(new URL(repositoryURL), artifactName);
-    String fileName = url.getPath().substring(url.getPath().lastIndexOf('/') + 1);
+    URL url = new URL(new URL(repositoryURL.endsWith("/")? repositoryURL: repositoryURL+"/"), artifactName);
+    String fileName = FilenameUtils.getName(url.getPath());
     Path destination = tmpDirectory.resolve(fileName);
 
     switch (url.getProtocol()) {
diff --git a/solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java b/solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java
index 31471e4..ab3b72a 100644
--- a/solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java
+++ b/solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java
@@ -26,15 +26,19 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Scanner;
+import java.util.Set;
+import java.util.stream.Collectors;
 
 import org.apache.solr.client.solrj.impl.HttpSolrClient;
 import org.apache.solr.common.NavigableObject;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.ZkStateReader;
 import org.apache.solr.common.util.Utils;
 import org.apache.solr.packagemanager.SolrPackage.Command;
 import org.apache.solr.packagemanager.SolrPackage.Manifest;
@@ -84,9 +88,9 @@ public class PackageManager implements Closeable {
     try {
       Map packagesZnodeMap = null;
 
-      if (zkClient.exists("/packages.json", true) == true) {
+      if (zkClient.exists(ZkStateReader.SOLR_PKGS_PATH, true) == true) {
         packagesZnodeMap = (Map)getMapper().readValue(
-            new String(zkClient.getData("/packages.json", null, null, true), "UTF-8"), Map.class).get("packages");
+            new String(zkClient.getData(ZkStateReader.SOLR_PKGS_PATH, null, null, true), "UTF-8"), Map.class).get("packages");
         for (Object packageName: packagesZnodeMap.keySet()) {
           List pkg = (List)packagesZnodeMap.get(packageName);
           for (Map pkgVersion: (List<Map>)pkg) {
@@ -112,34 +116,51 @@ public class PackageManager implements Closeable {
     Map<String, String> packages = null;
     try {
       NavigableObject result = (NavigableObject) Utils.executeGET(solrClient.getHttpClient(),
-          solrBaseUrl+"/api/collections/"+collection+"/config/params/PKG_VERSIONS?omitHeader=true&wt=javabin", Utils.JAVABINCONSUMER);
+          solrBaseUrl + PackageUtils.getCollectionParamsPath(collection) + "/PKG_VERSIONS?omitHeader=true&wt=javabin", Utils.JAVABINCONSUMER);
       packages = (Map<String, String>) result._get("/response/params/PKG_VERSIONS", Collections.emptyMap());
     } catch (PathNotFoundException ex) {
       // Don't worry if PKG_VERSION wasn't found. It just means this collection was never touched by the package manager.
     }
     if (packages == null) return Collections.emptyMap();
-    Map<String, SolrPackageInstance> ret = new HashMap<String, SolrPackageInstance>();
+    Map<String, SolrPackageInstance> ret = new HashMap<>();
     for (String packageName: packages.keySet()) {
-      if (Strings.isNullOrEmpty(packageName) == false) { // There can be an empty key, storing the version here
+      if (Strings.isNullOrEmpty(packageName) == false && // There can be an empty key, storing the version here
+          packages.get(packageName) != null) { // null means the package was undeployed from this package before
         ret.put(packageName, getPackageInstance(packageName, packages.get(packageName)));
       }
     }
     return ret;
   }
 
+  private void ensureCollectionsExist(List<String> collections) {
+    try {
+      List<String> existingCollections = zkClient.getChildren("/collections", null, true);
+      Set<String> nonExistent = new HashSet<>(collections);
+      nonExistent.removeAll(existingCollections);
+      if (nonExistent.isEmpty() == false) {
+          throw new SolrException(ErrorCode.BAD_REQUEST, "Collection(s) doesn't exist: " + nonExistent.toString());
+      }
+    } catch (KeeperException | InterruptedException e) {
+      throw new SolrException(ErrorCode.SERVER_ERROR, "Unable to fetch list of collections from ZK.");
+    }
+  }
+  
   private boolean deployPackage(SolrPackageInstance packageInstance, boolean pegToLatest, boolean isUpdate, boolean noprompt,
       List<String> collections, String overrides[]) {
-    for (String collection: collections) {
+    List<String> previouslyDeployed =  new ArrayList<>(); // collections where package is already deployed in
 
+    for (String collection: collections) {
       SolrPackageInstance deployedPackage = getPackagesDeployed(collection).get(packageInstance.name);
       if (packageInstance.equals(deployedPackage)) {
         if (!pegToLatest) {
           PackageUtils.printRed("Package " + packageInstance + " already deployed on "+collection);
+          previouslyDeployed.add(collection);
           continue;
         }
       } else {
         if (deployedPackage != null && !isUpdate) {
           PackageUtils.printRed("Package " + deployedPackage + " already deployed on "+collection+". To update to "+packageInstance+", pass --update parameter.");
+          previouslyDeployed.add(collection);
           continue;
         }
       }
@@ -148,9 +169,9 @@ public class PackageManager implements Closeable {
 
       // Get package params
       try {
-        boolean packageParamsExist = ((Map)PackageUtils.getJson(solrClient.getHttpClient(), solrBaseUrl + "/api/collections/abc/config/params/packages", Map.class)
+        boolean packageParamsExist = ((Map)PackageUtils.getJson(solrClient.getHttpClient(), solrBaseUrl + PackageUtils.getCollectionParamsPath(collection) + "/packages", Map.class)
             .getOrDefault("response", Collections.emptyMap())).containsKey("params");
-        SolrCLI.postJsonToSolr(solrClient, "/api/collections/" + collection + "/config/params",
+        SolrCLI.postJsonToSolr(solrClient, PackageUtils.getCollectionParamsPath(collection),
             getMapper().writeValueAsString(Collections.singletonMap(packageParamsExist? "update": "set",
                 Collections.singletonMap("packages", Collections.singletonMap(packageInstance.name, collectionParameterOverrides)))));
       } catch (Exception e) {
@@ -159,7 +180,7 @@ public class PackageManager implements Closeable {
 
       // Set the package version in the collection's parameters
       try {
-        SolrCLI.postJsonToSolr(solrClient, "/api/collections/" + collection + "/config/params",
+        SolrCLI.postJsonToSolr(solrClient, PackageUtils.getCollectionParamsPath(collection),
             "{set:{PKG_VERSIONS:{" + packageInstance.name+": '" + (pegToLatest? PackagePluginHolder.LATEST: packageInstance.version)+"'}}}");
       } catch (Exception ex) {
         throw new SolrException(ErrorCode.SERVER_ERROR, ex);
@@ -168,7 +189,7 @@ public class PackageManager implements Closeable {
       // If updating, refresh the package version for this to take effect
       if (isUpdate || pegToLatest) {
         try {
-          SolrCLI.postJsonToSolr(solrClient, "/api/cluster/package", "{\"refresh\": \"" + packageInstance.name + "\"}");
+          SolrCLI.postJsonToSolr(solrClient, PackageUtils.PACKAGE_PATH, "{\"refresh\": \"" + packageInstance.name + "\"}");
         } catch (Exception ex) {
           throw new SolrException(ErrorCode.SERVER_ERROR, ex);
         }
@@ -213,19 +234,27 @@ public class PackageManager implements Closeable {
 
       // Set the package version in the collection's parameters
       try {
-        SolrCLI.postJsonToSolr(solrClient, "/api/collections/" + collection + "/config/params",
+        SolrCLI.postJsonToSolr(solrClient, PackageUtils.getCollectionParamsPath(collection),
             "{update:{PKG_VERSIONS:{'" + packageInstance.name + "' : '" + (pegToLatest? PackagePluginHolder.LATEST: packageInstance.version) + "'}}}");
       } catch (Exception ex) {
         throw new SolrException(ErrorCode.SERVER_ERROR, ex);
       }
     }
 
-    // Verify that package was successfully deployed
-    boolean success = verify(packageInstance, collections);
-    if (success) {
-      PackageUtils.printGreen("Deployed and verified package: " + packageInstance.name + ", version: " + packageInstance.version);
+    List<String> deployedCollections = collections.stream().filter(c -> !previouslyDeployed.contains(c)).collect(Collectors.toList());
+
+    boolean success = true;
+    if (deployedCollections.isEmpty() == false) {
+      // Verify that package was successfully deployed
+      success = verify(packageInstance, deployedCollections);
+      if (success) {
+        PackageUtils.printGreen("Deployed on " + deployedCollections + " and verified package: " + packageInstance.name + ", version: " + packageInstance.version);
+      }
     }
-    return success;
+    if (previouslyDeployed.isEmpty() == false) {
+      PackageUtils.printRed("Already Deployed on " + previouslyDeployed + ", package: " + packageInstance.name + ", version: " + packageInstance.version);
+    }
+    return previouslyDeployed.isEmpty() && success;
   }
 
   private Map<String,String> getCollectionParameterOverrides(SolrPackageInstance packageInstance, boolean isUpdate,
@@ -243,7 +272,7 @@ public class PackageManager implements Closeable {
   Map<String, String> getPackageParams(String packageName, String collection) {
     try {
       return (Map<String, String>)((Map)((Map)((Map)
-          PackageUtils.getJson(solrClient.getHttpClient(), solrBaseUrl + "/api/collections/" + collection + "/config/params/packages", Map.class)
+          PackageUtils.getJson(solrClient.getHttpClient(), solrBaseUrl + PackageUtils.getCollectionParamsPath(collection) + "/packages", Map.class)
           .get("response"))
           .get("params"))
           .get("packages")).get(packageName);
@@ -260,7 +289,6 @@ public class PackageManager implements Closeable {
   public boolean verify(SolrPackageInstance pkg, List<String> collections) {
     boolean success = true;
     for (Plugin plugin: pkg.plugins) {
-      PackageUtils.printGreen(plugin.verifyCommand);
       for (String collection: collections) {
         Map<String, String> collectionParameterOverrides = getPackageParams(pkg.name, collection);
         Command cmd = plugin.verifyCommand;
@@ -322,6 +350,8 @@ public class PackageManager implements Closeable {
    */
   public void deploy(String packageName, String version, String[] collections, String[] parameters,
       boolean isUpdate, boolean noprompt) throws SolrException {
+    ensureCollectionsExist(Arrays.asList(collections));
+
     boolean pegToLatest = PackageUtils.LATEST.equals(version); // User wants to peg this package's version to the latest installed (for auto-update, i.e. no explicit deploy step)
     SolrPackageInstance packageInstance = getPackageInstance(packageName, version);
     if (packageInstance == null) {
@@ -342,11 +372,17 @@ public class PackageManager implements Closeable {
   }
 
   /**
-   * Undeploys a packge from given collections.
+   * Undeploys a package from given collections.
    */
   public void undeploy(String packageName, String[] collections) throws SolrException {
+    ensureCollectionsExist(Arrays.asList(collections));
+    
     for (String collection: collections) {
       SolrPackageInstance deployedPackage = getPackagesDeployed(collection).get(packageName);
+      if (deployedPackage == null) {
+        PackageUtils.printRed("Package "+packageName+" not deployed on collection "+collection);
+        continue;
+      }
       Map<String, String> collectionParameterOverrides = getPackageParams(packageName, collection);
 
       // Run the uninstall command for all plugins
@@ -374,13 +410,14 @@ public class PackageManager implements Closeable {
 
       // Set the package version in the collection's parameters
       try {
-        SolrCLI.postJsonToSolr(solrClient, "/api/collections/" + collection + "/config/params", "{set: {PKG_VERSIONS: {"+packageName+": null}}}");
-        SolrCLI.postJsonToSolr(solrClient, "/api/cluster/package", "{\"refresh\": \"" + packageName + "\"}");
+        SolrCLI.postJsonToSolr(solrClient, PackageUtils.getCollectionParamsPath(collection),
+            "{set: {PKG_VERSIONS: {"+packageName+": null}}}"); // Is it better to "unset"? If so, build support in params API for "unset"
+        SolrCLI.postJsonToSolr(solrClient, PackageUtils.PACKAGE_PATH, "{\"refresh\": \"" + packageName + "\"}");
       } catch (Exception ex) {
         throw new SolrException(ErrorCode.SERVER_ERROR, ex);
       }
 
-      // TODO: Also better to remove the package parameters
+      // TODO: Also better to remove the package parameters PKG_VERSION etc.
     }
   }
 
@@ -391,14 +428,14 @@ public class PackageManager implements Closeable {
   public Map<String, String> getDeployedCollections(String packageName) {
     List<String> allCollections;
     try {
-      allCollections = zkClient.getChildren("/collections", null, true);
+      allCollections = zkClient.getChildren(ZkStateReader.COLLECTIONS_ZKNODE, null, true);
     } catch (KeeperException | InterruptedException e) {
       throw new SolrException(ErrorCode.SERVICE_UNAVAILABLE, e);
     }
     Map<String, String> deployed = new HashMap<String, String>();
     for (String collection: allCollections) {
       // Check package version installed
-      String paramsJson = PackageUtils.getJsonStringFromUrl(solrClient.getHttpClient(), solrBaseUrl + "/api/collections/" + collection + "/config/params/PKG_VERSIONS?omitHeader=true");
+      String paramsJson = PackageUtils.getJsonStringFromUrl(solrClient.getHttpClient(), solrBaseUrl + PackageUtils.getCollectionParamsPath(collection) + "/PKG_VERSIONS?omitHeader=true");
       String version = null;
       try {
         version = JsonPath.parse(paramsJson, PackageUtils.jsonPathConfiguration())
diff --git a/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java b/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java
index 0336c47..01595a7 100644
--- a/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java
+++ b/solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java
@@ -59,6 +59,9 @@ public class PackageUtils {
    * Represents a version which denotes the latest version available at the moment.
    */
   public static String LATEST = "latest";
+  
+  public static String PACKAGE_PATH = "/api/cluster/package";
+  public static String REPOSITORIES_ZK_PATH = "/repositories.json";
  
   public static Configuration jsonPathConfiguration() {
     MappingProvider provider = new JacksonMappingProvider();
@@ -248,4 +251,8 @@ public class PackageUtils {
     }
     return ret;
   }
+
+  public static String getCollectionParamsPath(String collection) {
+    return "/api/collections/" + collection + "/config/params";
+  }
 }
diff --git a/solr/core/src/java/org/apache/solr/packagemanager/RepositoryManager.java b/solr/core/src/java/org/apache/solr/packagemanager/RepositoryManager.java
index 5dd503e..3d6075b 100644
--- a/solr/core/src/java/org/apache/solr/packagemanager/RepositoryManager.java
+++ b/solr/core/src/java/org/apache/solr/packagemanager/RepositoryManager.java
@@ -122,23 +122,23 @@ public class RepositoryManager {
 
     List<PackageRepository> repos = getMapper().readValue(existingRepositoriesJson, List.class);
     repos.add(new DefaultPackageRepository(name, uri));
-    if (packageManager.zkClient.exists("/repositories.json", true) == false) {
-      packageManager.zkClient.create("/repositories.json", getMapper().writeValueAsString(repos).getBytes("UTF-8"), CreateMode.PERSISTENT, true);
+    if (packageManager.zkClient.exists(PackageUtils.REPOSITORIES_ZK_PATH, true) == false) {
+      packageManager.zkClient.create(PackageUtils.REPOSITORIES_ZK_PATH, getMapper().writeValueAsString(repos).getBytes("UTF-8"), CreateMode.PERSISTENT, true);
     } else {
-      packageManager.zkClient.setData("/repositories.json", getMapper().writeValueAsString(repos).getBytes("UTF-8"), true);
+      packageManager.zkClient.setData(PackageUtils.REPOSITORIES_ZK_PATH, getMapper().writeValueAsString(repos).getBytes("UTF-8"), true);
     }
 
     if (packageManager.zkClient.exists("/keys", true)==false) packageManager.zkClient.create("/keys", new byte[0], CreateMode.PERSISTENT, true);
     if (packageManager.zkClient.exists("/keys/exe", true)==false) packageManager.zkClient.create("/keys/exe", new byte[0], CreateMode.PERSISTENT, true);
-    if (packageManager.zkClient.exists("/keys/exe/"+name+".der", true)==false) {
-      packageManager.zkClient.create("/keys/exe/"+name+".der", new byte[0], CreateMode.PERSISTENT, true);
+    if (packageManager.zkClient.exists("/keys/exe/" + name + ".der", true)==false) {
+      packageManager.zkClient.create("/keys/exe/" + name + ".der", new byte[0], CreateMode.PERSISTENT, true);
     }
-    packageManager.zkClient.setData("/keys/exe/"+name+".der", IOUtils.toByteArray(new URL(uri+"/publickey.der").openStream()), true);
+    packageManager.zkClient.setData("/keys/exe/" + name + ".der", IOUtils.toByteArray(new URL(uri + "/publickey.der").openStream()), true);
   }
 
   private String getRepositoriesJson(SolrZkClient zkClient) throws UnsupportedEncodingException, KeeperException, InterruptedException {
-    if (zkClient.exists("/repositories.json", true)) {
-      return new String(zkClient.getData("/repositories.json", null, null, true), "UTF-8");
+    if (zkClient.exists(PackageUtils.REPOSITORIES_ZK_PATH, true)) {
+      return new String(zkClient.getData(PackageUtils.REPOSITORIES_ZK_PATH, null, null, true), "UTF-8");
     }
     return "[]";
   }
@@ -195,7 +195,7 @@ public class RepositoryManager {
       add.manifest = "/package/" + packageName + "/" + version + "/manifest.json";
       add.manifestSHA512 = manifestSHA512;
 
-      V2Request req = new V2Request.Builder("/api/cluster/package")
+      V2Request req = new V2Request.Builder(PackageUtils.PACKAGE_PATH)
           .forceV2(true)
           .withMethod(SolrRequest.METHOD.POST)
           .withPayload(Collections.singletonMap("add", add))
@@ -308,10 +308,12 @@ public class RepositoryManager {
       installPackage(packageName, version);
     }
 
-    SolrPackageInstance updatedPackage = packageManager.getPackageInstance(packageName, PackageUtils.LATEST);
-    boolean res = packageManager.verify(updatedPackage, peggedToLatest);
-    PackageUtils.printGreen("Verifying version " + updatedPackage.version + 
-        " on " + peggedToLatest + ", result: " + res);
-    if (!res) throw new SolrException(ErrorCode.BAD_REQUEST, "Failed verification after deployment");
+    if (peggedToLatest.isEmpty() == false) {
+      SolrPackageInstance updatedPackage = packageManager.getPackageInstance(packageName, PackageUtils.LATEST);
+      boolean res = packageManager.verify(updatedPackage, peggedToLatest);
+      PackageUtils.printGreen("Verifying version " + updatedPackage.version + 
+          " on " + peggedToLatest + ", result: " + res);
+      if (!res) throw new SolrException(ErrorCode.BAD_REQUEST, "Failed verification after deployment");
+    }
   }
 }
diff --git a/solr/core/src/java/org/apache/solr/util/PackageTool.java b/solr/core/src/java/org/apache/solr/util/PackageTool.java
index 8464ec1..f7744c9 100644
--- a/solr/core/src/java/org/apache/solr/util/PackageTool.java
+++ b/solr/core/src/java/org/apache/solr/util/PackageTool.java
@@ -118,9 +118,13 @@ public class PackageTool extends SolrCLI.ToolBase {
                 } else {
                   String packageName = cli.getArgs()[1];
                   Map<String, String> deployedCollections = packageManager.getDeployedCollections(packageName);
-                  PackageUtils.printGreen("Collections on which package " + packageName + " was deployed:");
-                  for (String collection: deployedCollections.keySet()) {
-                    PackageUtils.printGreen("\t" + collection + "("+packageName+":"+deployedCollections.get(collection)+")");
+                  if (deployedCollections.isEmpty() == false) {
+                    PackageUtils.printGreen("Collections on which package " + packageName + " was deployed:");
+                    for (String collection: deployedCollections.keySet()) {
+                      PackageUtils.printGreen("\t" + collection + "("+packageName+":"+deployedCollections.get(collection)+")");
+                    }
+                  } else {
+                    PackageUtils.printGreen("Package "+packageName+" not deployed on any collection.");
                   }
                 }
                 break;
@@ -130,7 +134,7 @@ public class PackageTool extends SolrCLI.ToolBase {
                 String packageName = parsedVersion.first();
                 String version = parsedVersion.second();
                 repositoryManager.install(packageName, version);
-                PackageUtils.printGreen(repositoryManager.toString() + " installed.");
+                PackageUtils.printGreen(packageName + " installed.");
                 break;
               }
               case "deploy":
@@ -140,7 +144,7 @@ public class PackageTool extends SolrCLI.ToolBase {
                 String version = parsedVersion.second();
                 boolean noprompt = cli.hasOption('y');
                 boolean isUpdate = cli.hasOption("update") || cli.hasOption('u');
-                packageManager.deploy(packageName, version, PackageUtils.validateCollections(cli.getOptionValues("collections")), cli.getOptionValues("param"), isUpdate, noprompt);
+                packageManager.deploy(packageName, version, PackageUtils.validateCollections(cli.getOptionValue("collections").split(",")), cli.getOptionValues("param"), isUpdate, noprompt);
                 break;
               }
               case "undeploy":
@@ -150,7 +154,7 @@ public class PackageTool extends SolrCLI.ToolBase {
                   throw new SolrException(ErrorCode.BAD_REQUEST, "Only package name expected, without a version. Actual: " + cli.getArgList().get(1));
                 }
                 String packageName = parsedVersion.first();
-                packageManager.undeploy(packageName, cli.getOptionValues("collections"));
+                packageManager.undeploy(packageName, cli.getOptionValue("collections").split(","));
                 break;
               }
               case "help":
@@ -228,7 +232,7 @@ public class PackageTool extends SolrCLI.ToolBase {
 
         OptionBuilder
         .withArgName("COLLECTIONS")
-        .hasArgs()
+        .hasArg()
         .isRequired(false)
         .withDescription("List of collections. Run './solr package help' for more details.")
         .create("collections"),
diff --git a/solr/core/src/test/org/apache/solr/cloud/PackageManagerCLITest.java b/solr/core/src/test/org/apache/solr/cloud/PackageManagerCLITest.java
index 4c08aad..38b66b6 100644
--- a/solr/core/src/test/org/apache/solr/cloud/PackageManagerCLITest.java
+++ b/solr/core/src/test/org/apache/solr/cloud/PackageManagerCLITest.java
@@ -20,11 +20,9 @@ package org.apache.solr.cloud;
 import java.lang.invoke.MethodHandles;
 import java.util.Arrays;
 
-import org.apache.logging.log4j.Level;
-import org.apache.logging.log4j.core.config.Configurator;
-import org.apache.lucene.util.SuppressForbidden;
 import org.apache.solr.client.solrj.request.CollectionAdminRequest;
 import org.apache.solr.core.TestSolrConfigHandler;
+import org.apache.solr.util.LogLevel;
 import org.apache.solr.util.PackageTool;
 import org.apache.solr.util.SolrCLI;
 import org.eclipse.jetty.server.Handler;
@@ -39,6 +37,7 @@ import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+@LogLevel("org.apache=INFO")
 public class PackageManagerCLITest extends SolrCloudTestCase {
 
   // Note for those who want to modify the jar files used in the packages used in this test:
@@ -70,13 +69,9 @@ public class PackageManagerCLITest extends SolrCloudTestCase {
   }
 
   @Test
-  @SuppressForbidden(reason = "Need to turn off logging, and SLF4J doesn't seem to provide for a way.")
   public void testPackageManager() throws Exception {
     PackageTool tool = new PackageTool();
     
-    // Enable the logger for this test. Need to do this since the tool disables logger.
-    Configurator.setRootLevel(Level.INFO);
-
     String solrUrl = cluster.getJettySolrRunner(0).getBaseUrl().toString();
 
     run(tool, new String[] {"-solrUrl", solrUrl, "list-installed"});