Don't leak sysctl.d drop-ins for internal networks - #1498
Conversation
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
This is mixing way to many things at once, this needs to be split in proper commits with proper reasons why they are needed. Staring with the bug at hand of the wrong internal condition.
I am also not convinced we need the "atomic" write behavior, our writes are small enough. And enabling forwarding on that one bridge should not be needed as we already enable the default forwarding.
We cannot skip writing the sysctl.d drop-ins if they exist already, anymore, because issue containers#1498 left stale ones with bad settings around. This patch ensures netavark recovers from this bogus state without rebooting the host. Since systemd-sysctl operates concurrently, we also have to assume that it is currently reading the file we're attempting to write. Thus, the write needs to be atomic. Signed-off-by: Hans-Christian Ebke <christian.ebke@rwth-aachen.de>
|
Fair point! I've split it up into multiple commits. I still think the atomic write is necessary (see my reasoning in the commit message). I removed emitting the forwarding = 1 setting for non-internal networks altogether, since it shouldn't be necessary as long as everything else operates as it should. (It would be kind of nice from a symmetry perspective, though, so maybe consider it. ;-)) Unit tests are green, Hoping the github tests will be too once they have finished running. 🤞 |
|
I did fat-finger the issue # in one commit message, though. :-( |
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
This needs a rebase, logic wise seem fine now for me
|
Rebased. Checks currently in progress. |
|
LGTM |
Fixes: containers#1497 Signed-off-by: Hans-Christian Ebke <christian.ebke@rwth-aachen.de>
We cannot skip writing the sysctl.d drop-ins if they exist already, anymore, because issue containers#1497 left stale ones with bad settings around. This patch ensures netavark recovers from this bogus state without rebooting the host. Since systemd-sysctl operates concurrently, we also have to assume that it is currently reading the file we're attempting to write. Thus, the write needs to be atomic. Signed-off-by: Hans-Christian Ebke <christian.ebke@rwth-aachen.de>
Relates to change from previous commit Signed-off-by: Hans-Christian Ebke <christian.ebke@rwth-aachen.de>
|
I rebased once more. If you don't have any more change requests, would you mind merging this? |
|
@Luap99 PTAL |
3 participants
Fixes: #1497
These changed were generated by Claude Opus and then reviewed, edited, and tested by me.