@Clement_Goubert is this still needed?

Not strictly, but it would be nice to have for peace of mind. This may be a task that @Blake can work on in the coming quarter as prep for the next switchover.

@Blake could you move this on the board if you plan to do it this quarter?

The way I'm considering going about this would be to create a switchover lock or flag file, and a cookbook allowlist. All cookbooks would need to check if a switchover lock exists, then, if it does, the cookbook would check the allowlist for its own name. If the name isn't in the allowlist, execution should cease. I'm going to see if I can find someone on Infrastructure Foundations to have a chat with about this approach.

Change #1239368 had a related patch set uploaded (by Blake; author: Blake):

[operations/software/spicerack@master] spicerack: Add a mechanism for a global Spicerack lock.

https://gerrit.wikimedia.org/r/1239368

@elukey and @Volans, do you happen to have thoughts about the best way to go about checking for a global lock and associated allowlist? My intuition would be that this might be something to be included in CookbookRunnerBase, but I'm not sure.

@Blake I think we should do that check inside the logic that fires up the cookbook, namely inside CookbookItem.run() [1].
Depending on what the API in locking will be there are different options of course.

One way to go could be to do it automatically when we try to get the automatic cookbook lock in [2], namely if the lock instance is for COOKBOOKS_PREFIX when trying to acquire a lock check first the global lock for the provided lock_key splitted on the : (the cookbook FQDN). In that way it will be transparent on the cookbooks side. The only drawback of that is that the lock is acquired after the cookbook parse_args() and get_runner() are run (so also the init of the cookbook runner has run). But per API contract only RO operation can be done in that part of the code.

Another way to go if we want to be even more defensive and not allow to run any code from the cookbook when a global lock is in place is to put it at the start of run() [1], before even parsing the arguments. In this case the API in locking will need something to be called to check if there is a global lock and if it allows this cookbook to be run. The drawback of this approach is that the client (but there is only one) is responsible to know that it has to check for a global lock and how.

[1] https://gerrit.wikimedia.org/r/plugins/gitiles/operations/software/spicerack/+/refs/heads/master/spicerack/_menu.py#192
[2] https://gerrit.wikimedia.org/r/plugins/gitiles/operations/software/spicerack/+/refs/heads/master/spicerack/_menu.py#220

I think I'd be inclined to prefer the more-defensive option (maybe @Clement_Goubert has a preference here?).

Just to make sure I understand - does this mean that we'd add a function in locking.py (something like check_global_lock()), which will then check if the global lock exists, and if it does, will check whether the cookbook that executed the function is on the allowlist? The client in this case is _menu.py?

I'd rather we be more defensive than not, especially if there is no strong enforcement of RO-only operations in get_runner().

In T330997#11635578, @Blake wrote:

I think I'd be inclined to prefer the more-defensive option (maybe @Clement_Goubert has a preference here?).

Just to make sure I understand - does this mean that we'd add a function in locking.py (something like check_global_lock()), which will then check if the global lock exists, and if it does, will check whether the cookbook that executed the function is on the allowlist? The client in this case is _menu.py?

More or less yes, you would not have a way to get the "caller" as the cookbook would not be calling it. I think it would be much easier to have a function that accepts the FQDN of the cookbook as input and returns none if it's all good or raises either a LockError or a new dedicated GlobalLockError exception with a message that a global lock created on $DATE with TTL N doesn't allow to run this cookbook, to retry later (and include any user-provided message when the lock was created if there is any).

As for the naming that's the really hard part :-P Either what you proposed like check_global_lock(name: str) -> None or maybe a more verbose one like ensure_not_globally_locked? No strong opinion

In T330997#11639650, @Volans wrote:

In T330997#11635578, @Blake wrote:

I think I'd be inclined to prefer the more-defensive option (maybe @Clement_Goubert has a preference here?).

Just to make sure I understand - does this mean that we'd add a function in locking.py (something like check_global_lock()), which will then check if the global lock exists, and if it does, will check whether the cookbook that executed the function is on the allowlist? The client in this case is _menu.py?

More or less yes, you would not have a way to get the "caller" as the cookbook would not be calling it. I think it would be much easier to have a function that accepts the FQDN of the cookbook as input and returns none if it's all good or raises either a LockError or a new dedicated GlobalLockError exception with a message that a global lock created on $DATE with TTL N doesn't allow to run this cookbook, to retry later (and include any user-provided message when the lock was created if there is any).

As for the naming that's the really hard part :-P Either what you proposed like check_global_lock(name: str) -> None or maybe a more verbose one like ensure_not_globally_locked? No strong opinion

Not a fan of negative names, I think check_global_lock is descriptive enough. That would result in something like this in run()?

try:
  check_global_lock(cookbook_fqdn)
except LockError:
  #log and exit

In T330997#11640417, @Clement_Goubert wrote:

Not a fan of negative names, I think check_global_lock is descriptive enough. That would result in something like this in run()?

try:
  check_global_lock(cookbook_fqdn)
except LockError:
  #log and exit

yes, logging and instead of exit just returning one of the reserved retcodes (either the generic EXCEPTION_RETCODE or a new dedicated one if we feel like having one for just this.

@Blake did you use that in recent switchover? We didn't account for capacity in Q4 so shall we park this until Q1/next switchover?

@MLechvien-WMF This was not completed in time for the switchover. I'm in the middle of a significant rework after the last round of comments in https://gerrit.wikimedia.org/r/c/operations/software/spicerack/+/1239368. If I pivot back to working on this, I can likely have it out for review in a week or so.

From what I've seen of the switchover process, it seems well-communicated enough that this is more of a fallback safeguard, so I'm not sure that it's urgent to prioritize. On the other hand, I have context for it right now. If there aren't more urgent things for me to be doing, I don't mind continuing work here.

Moving this to the backlog for now.

Given this has been moved to the backlog I'll leave here a comment for our future selves: it could be useful to also adjust the proposed feature to support a slightly different use case, that of a maintenance on a cumin host.
The operator should be able to set a lock that prevent any cookbook from running on the given cumin host but allows them to run on the other cumin host(s) (and saying so in the lock message for the user).