Allow explicit reboot on transactional minions #643
Conversation
salt/modules/transactional_update.py
Outdated
| for _, value in local.items(): | ||
| if isinstance(value, dict) and "name" in value: |
There was a problem hiding this comment.
Does this match all kinds of state functions? If so, we risk rebooting when it wasn't actually requested. I think we should prefer false-negatives over false-positives. Rebooting when it's not asked for is disruptive while a missing reboot can be worked around.
There was a problem hiding this comment.
I've modified the actual matching below since this comment; so while we do iterate over all state functions, the function is matched with exact match only. Right now, we only match names that equal to reboot or system.reboot (from module.include). Is that sufficient to alleviate your concern, or do you think we should also match the key? (e.g. cmd or module in key)
Side note: the following sls seems not to start a transaction, so this change doesn't affect it.
always-passes-with-any-kwarg:
test.nop:
- name: reboot
The same applies to state.test - it also doesn't run in a transaction.
There was a problem hiding this comment.
Side note: the following sls seems not to start a transaction, so this change doesn't affect it.
Is that really the case? Without this patch, the condition to decide if a reboot should be called requires a pending transaction. With this patch, it's either pending transaction + activate_transaction=True or we find name: reboot. Am I missing something?
The same applies to state.test - it also doesn't run in a transaction.
Right, that's the default. We might want to still rule that out (as you've done anyway), the configuration what runs inside/outside a transaction can be configured.
There was a problem hiding this comment.
No indeed, you were right. When I was testing it, I think I mistakenly turned off the t-u executor. Upon re-checking my results, I realized I was wrong, and added a further check (and a test case)
salt/modules/transactional_update.py
Outdated
| return False | ||
|
|
||
| explicit_reboot_cmds = set(["reboot", "system.reboot"]) | ||
| explicit_reboot_modules = ["cmd_", "module_"] |
There was a problem hiding this comment.
Maybe it's a bit wrong to specify here _ as a part of the module name, actually _|- is used as a separator in the state apply results like you use in the tests:
"cmd_|-reboot_test_|-reboot_|-run": {
"name": "reboot",
"changes": {},
"result": True,
}
I don't know why but actually, cmd_|-reboot_test_|-reboot_|-run is a module name, id, name, fun separated with _|-, so maybe it's better to parse it using _|- instead of relying on |, but there is a mess that actually either the name or id can also contain _|- and it could add the mess, but even existing code in salt is producing wrong output in such cases.
There was a problem hiding this comment.
Makes sense,thank you for the suggestion! Implemented.
salt/modules/transactional_update.py
Outdated
| reboot() | ||
|
|
||
|
|
||
| def _user_specified_reboot(local, function): | ||
| if function != "state.highstate" and function != "state.sls": |
There was a problem hiding this comment.
Looks like we need to cover all of these functions:
salt/salt/executors/transactional_update.py
Lines 14 to 19 in 86d6235
Actually
state.highstate is not widely used in Uyuni/SUMA, but mostly state.apply with no mods specified (as state.apply with no mods is acting as state.highstate)
There was a problem hiding this comment.
Indeed, great catch - didn't want to maintain the list of functions in two places, so I pulled in the DELEGATION_MAP directly. WTYD?
What does this PR do?
In this PR, we parse the execution result on a transactional system. If user specified any of the provided
explicit_reboot_cmdscommands in a state file, we issue a reboot even whenactivate_transactionwas not explicitly passed to the module.Note that user still gets the result of
running in chroot, ignoring(but the system actually reboots).What issues does this PR fix or reference?
Part of https://github.com/SUSE/spacewalk/issues/23874
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes/No