Allow explicit reboot on transactional minions#643
Allow explicit reboot on transactional minions#643m-czernek wants to merge 13 commits intoopenSUSE:openSUSE/release/3006.0from
Conversation
agraul
left a comment
agraul
left a comment
There was a problem hiding this comment.
It's good to have this as a first try. As you said, it already works. The risk with the current state is that it works too often, we should be more defensive 😄
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)
vzhestkov
left a comment
vzhestkov
left a comment
There was a problem hiding this comment.
I think it's better to adjust the code for _|- as a separator.
salt/modules/transactional_update.py
Outdated
|
|
||
|
|
||
| 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?
vzhestkov
force-pushed
the
openSUSE/release/3006.0
branch
from
0e28d38 to
df1cc89
Compare
meaksh
force-pushed
the
openSUSE/release/3006.0
branch
from
dc3027b to
5cfbb16
Compare
3 participants
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