nat64: T6627: call check_kmod within standard config function #3927
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jestabro
requested review from
dmbaturin,
sarthurdev,
zdc,
sever-sever,
c-po and
fett0
|
👍 |
|
❌ |
sever-sever
approved these changes
Aug 2, 2024
dmbaturin
requested changes
Aug 2, 2024
src/conf_mode/nat64.py
Outdated
| if not nat64: | ||
| # no need to verify the CLI as nat64 is going to be deactivated | ||
| unload_kmod(['jool']) |
There was a problem hiding this comment.
I'm quite uneasy about system state changes in verify(). I'm inclined to think that verify() should never include any logic except config verification. Do you think it's feasible to move this to apply()?
There was a problem hiding this comment.
Agreed: though I consider the real problem with the logic of this script to be the need to modprobe in the verify() stage, no need to make it worse. Moved to apply().
Functions called from config scripts outside of the standard functions get_config/verify/generate/apply will not be called when run under configd. Move as appropriate for the general config script structure and the specific script requirements.
jestabro
force-pushed
the
nat64-check-kmod
branch
from
cb05004
to
aeb5197
Compare
dmbaturin
approved these changes
Aug 2, 2024
|
@Mergifyio backport circinus |
✅ Backports have been created
|
12 tasks
12 tasks
12 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change Summary
The config mode script nat64.py calls the modprobe utility
check_kmodoutside of the standard functions, leading to anOSErrorwhen run under configd. Current behavior of configd will re-run the script within the CLI context, leading to config and smoketest success; as that behavior will be changed in the future to accommodate all scripts running within the configd context (T6608), correct now.There are several other config scripts that use this pattern, which will be all changed in a separate PR. The nat64.py script deserved special attention as it provides an example of state configuration informing the syntax verification stage, which should be kept in mind as we move to distinguishing syntax verification from system state verification.
Types of changes
Related Task(s)
Related PR(s)
Component(s) name
Proposed changes
How to test
Confirmation provided in task. Success in smoketest and journalctl output, below.
Smoketest result
Checklist: