Adds "try" and "monitor-retry" options to consul lock command

[slackpad]

The "try" option should close #780 and the "monitor-retry" option should close #1162.

[slackpad]

Tweaked things in that last commit so it always waits for the timeout period trying to get the lock vs. trying once "up to" the timeout, which usually just falls through right away. This should better match what users expect.

[ryanuber]

Maybe at least move this out of the retry loop? Maybe we could use like an IsServerError(error) since I think we do this in a bunch of other places too IIRC.

[slackpad]

Haha I was paste-n good call.

[ryanuber]

Looks like you already addressed the minor comments. This LGTM otherwise 🚢

[issacg]

Question: Is it possible to use the new monitor-retry with a (slightly) larger timeout to facilitate restarting consul servers without damaging the quarum? I'm not sure if there wouldn't be unintended side-affects at first glance, but it seems that if you gave a large enough value to defaultMonitorRetry (say 60) one could use a configuration management tool to wrap something like service consul restart on all servers in each datacenter simultaneously without needing to time the rolling update. If it takes more than 60 seconds for the restart to complete and release the lock, then something is likely wrong with your setup anyway. It's a bit naive but it seems that the code should allow this, no?

Also, based on the naming of DefaultMonitorRetryTime it implies that this ought to be overridable, but based on the usage of defaultMonitorRetry and monitor-retry it seems to be intended to be hardcoded at 1s intervals. I'm just wondering what the folks who wrote the code intended.

Thanks!

[slackpad]

Hi @IsaacG we didn't design this feature with this use case in mind, but it seems like it could be possible to use it as part of a solution for this. One problem I can see though is that being able to take the lock doesn't necessarily mean that it's safe to take a server down, so doing this in a safe way would require more logic. For example, if one server out of three died during the upgrade and lost the lock, a second one would be able to get it and restart itself, putting the cluster into an outage condition. You'd still want to confirm things like last_log_index on the newly-added server before rolling another one - https://www.consul.io/docs/guides/servers.html.

For your second question, I named the DefaultMonitorRetryTime in case we decided to make it configurable later but opted for a 1 second time with just a configurable number of retries to keep the number of config knobs down. The time between retries isn't as important as the total time taken attempting retries so that seemed more important. I suppose if you had tons of nodes trying locks you might want to make that larger to avoid a thundering herd, but that's probably not a super common use case for locks.

[issacg]

@slackpad thanks for the comments!

In some very initial toying around, I already discovered that the 1 second DefaultMonitorRetryTime is a problem when trying to rolling upgrade, because consul lock died really fast. I don't have the logs in front of me ATM, but IIRC the problem was that consul client was unable to talk to the agent, and thus tried to immediately release the lock which, again, it couldn't do. I haven't had time to delve in to the source yet (out of round tuits for now 😞) but hypothesized that increasing the retry time might avoid this.

Also, as you pointed out, there are a lot of rough edges that I'm admittedly completely ignoring for now. I just wanted to start with something as a PoC.