Stop upgrade process if invalid table locks are found.

[EdColeman]

Add pre-upgrade check that looks for tserver locks in ZooKeeper and checks that the locks are for the current version. Invalid tservers are terminated listed in the log and the upgrade process is terminated (termination can be over-ridden by property)

• If the lock data cannot be parsed, it is added to the bad server list. (data changed in 3.0)
• Make a thrift call to servers with locks that can be parsed, the thrift call will fail if incorrect version and it is added to the bad server list.

This addresses issue #1300 and when complete, can close https://github.com/apache/accumulo/issues/3098

[dlmarion]

Add pre-upgrade check that looks for tserver locks in ZooKeeper and checks that the locks for the current version. Invalid locks are removed from ZooKeeper.

• If the lock data cannot be parsed, it is removed. (data changed in 3.0)
• Make a thrift call to servers with locks that can be parsed, the thrift call will fail if incorrect version and it is removed.

For an upgrade wouldn't the typical process be?

1. Shutdown Accumulo
2. Install new version of Accumulo
3. Start Manager
4. Start remaining processes when upgrade is successful

I assume then that the goal of this is to stop any TabletServer processes that are running an older version and remove their locks? I'm confused by the description I guess - is the goal to remove invalid locks or is the goal to stop processes running older versions of software and you are doing it by checking the lock data?

[EdColeman]

I updated the initial comment. The goal is to terminate tservers that are running older versions - this is accomplished by removing their locks.

It is possible to have the tservers running before the upgrade process in the manager kicks off. If somehow, a tserver was started with an older version then, currently the way to tell that there are "old" versions running are log messages indicating that communications cannot be established. This attempts, that at least on the start of the upgrade that all of the tservers are running the target version.

[dlmarion]

Should it be limited to tservers? Why not check all locks from all processes?

[EdColeman]

Tservers were specifically called out in https://github.com/apache/accumulo/issues/1300 - it may be possible / desirable to add other services if they are identified.

[dlmarion]

I'd rather see this be a hard failure when there are old servers running. From a user perspective, I would rather know that I need to stop old servers or manually delete their ZK paths before proceeding with an upgrade. It's possible that I believe I stopped everything, but some stop commands in a distributed environment failed for some reason or another.

[EdColeman]

94b8f8b5d8 changes the logging to provide list of lock that were deleted as well as the locks that failed to be removed.

The timeout was increased to virtually block - and if it does timeout the check throws an exception to stop the upgrade.

[EdColeman]

@ivakegg - wonder if you'd like to chime in on this. Do you think this is a good check to perform? Also, is it better to have it fail hard, or should it allow the upgrade to continue?

[keith-turner]

It may be good to move the pre upgrade checks into the Upgrader because the current version of Accumulo may not know how to read the previous version of Accumulo's metadata in order to perform upgrade checks. Adding new methods make this step explicit when someone is thinking about writing a new Upgrader, it forces them to explicity consider if conditions are safe for an upgrade. What conditions are safe for an upgrade could be highly dependent on the two versions of Accumulo. If its an explicit step in the Upgrader, then it makes people consider upgrade safety in future versions of Accumulo. With all of this in mind I made the following attempt to update the Upgrader interface.

public interface Upgrader {

  /**
   * This method is called before {@link #upgradeZookeeper(ServerContext)} to ensure the system is
   * in a suitable state to upgrade. Examples of what this check could attempt to do are :
   *
   * <UL>
   * <LI>If root tablet metadata is not upgraded, ensure the root tablet is not currently assigned
   * to a live tserver.</LI>
   * <LI>Ensure there are no server processes running from a previous version of Accumulo</LI>
   * </UL>
   *
   * <P>
   * The implementation of this method is delegated to the Uprader because it may have to read data
   * from DFS and Zookeeper in formats used by previous versions of Accumulo. This code for reading
   * previous storage formats should always be delegated to the uprader.
   * </P>
   *
   * <p>
   * Any possible conditions in the distributed system that could cause the upgrade process to
   * corrupt metadata or data should be checked for here and fail if detected.
   * </p>
   *
   * @throws IllegalStateException if conditions are not suitable for upgrade. The exception message
   *         should clearly indicate specifics about what is blocking this upgrade step.
   */
  void preUpgradeZookeeperCheck(ServerContext context);

  /**
   * Update entries in ZooKeeper - called before the root tablet is loaded.
   *
   * @param context the server context.
   */
  void upgradeZookeeper(ServerContext context);

  /**
   * This method is called before {@link #upgradeRoot(ServerContext)} to ensure the system is in a
   * suitabl state to upgrade. Examples of what this check could attempt to do are :
   *
   * <UL>
   * <LI>If metadata tablets metadata is not upgraded, ensure no metadata tablets are currently
   * assigned to a live tserver</LI>
   * </UL>
   *
   * @throws IllegalStateException if conditions are not suitable for upgrade. The exception message
   *         should clearly indicate specifics about what is blocking this upgrade step.
   */
  void preUpgradeRootCheck(ServerContext context);

  /**
   * Update the root tablet - called after the root tablet is loaded and before the metadata table
   * is loaded.
   *
   * @param context the server context.
   */
  void upgradeRoot(ServerContext context);

  /**
   * This method is called before {@link #upgradeMetadata(ServerContext)} to ensure the system is in
   * a suitable state to upgrade. Examples of what this check could attempt to do are :
   *
   * <UL>
   * <LI>If user tablet metadata is not upgraded, ensure no user tablets are currently assigned
   * to a live tserver</LI>
   * </UL>
   *
   * @throws IllegalStateException if conditions are not suitable for upgrade. The exception message
   *         should clearly indicate specifics about what is blocking this upgrade step.
   */
  void preUpgradeMetadataCheck(ServerContext context);

  /**
   * Update the metadata table - called after the metadata table is loaded and before loading user
   * tablets.
   *
   * @param context the server context.
   */
  void upgradeMetadata(ServerContext context);
}

Would the checks written so far in the PR fit into this model?

[keith-turner]

I'm confused by the description I guess - is the goal to remove invalid locks or is the goal to stop processes running older versions of software and you are doing it by checking the lock data?

I would say the overall goal should be to prevent the upgrade code from causing corruption of user metadata or data. Root and metadata tablets running on old server processes while the upgrade process runs could cause corruption of their metadata.

I'd rather see this be a hard failure when there are old servers running. From a user perspective, I would rather know that I need to stop old servers or manually delete their ZK paths before proceeding with an upgrade.

I agree with this, I think detecting old server processes and failing to upgrade is preferable to automatically killing them. What if someone accidentally runs the new upgrade code on the wrong cluster and they would not have wanted it to kill all the server processes?

[EdColeman]

With the current change - the check tries to contact the tservers and if any are found, then the list is printed at the end of the check, but the lock are not removed. The default behavior is then to exit the upgrade code and force the upgrade to halt. This can be controlled via a property if someone really wishes to carry on with tservers that may be incorrect.

[EdColeman]

The pre-upgrade stage was added to support code / checks that are likely independent of a specific version. Checking the ZooKeeper authorizations is one example. It is not version dependent, but continuing an upgrade with potentially invalid auths seemed like it should be avoided.

The ability to communicate with a tserver seems to also fit within this category. Before anything happens that is dependent on version, this check would establish a baseline of "at least I can talk to all of the registered tservers" before any version dependent code would have a chance to run.

[keith-turner]

The pre-upgrade stage was added to support code / checks that are likely independent of a specific version.

Yeah if the data is the same across the two versions then we do not need to push anything to the Upgrader. I was thinking we had changed how lock data is stored in ZK. That may be between 2.0 and 2.1 though, not for 2.1 to 3.0.

The ability to communicate with a tserver seems to also fit within this category. Before anything happens that is dependent on version, this check would establish a baseline of "at least I can talk to all of the registered tservers" before any version dependent code would have a chance to run.

We could also possibly add something to ServiceLockData that could be used to check if a lock on ZK is associated with something running an older code version. That could be done in addition to or instead of attempting to make a RPC.

[ctubbsii]

@keith-turner You mentioned in Slack that this might be useful to have for Upgrader9to10, which would upgrade to 2.1. It might be worth adding some upgrade checks to the 2.1 branch. I'm not sure how much of this PR could be backported, or if it would need a different, but similar, solution.