Bridge refactor #874
Bridge refactor #874
Conversation
wkz
force-pushed
the
br-refactor
branch
2 times, most recently
from
a030ac3 to
9716d28
Compare
There was a problem hiding this comment.
Almost like the Texas Chain Saw Massacre ... impressive Christmas cleaning! 🤯
I only found the one thing, but since the tests pass maybe that commented line can just be dropped now? I say maybe because with this massive change it is now very hard to see how it all ties together. Maybe you can draw a picture or update the confd README with some help for us mere mortals?
Another thing, which is more of a feeling that I'm struggling to put into words is: the naming of functions in the new files vary and seem excessively long and filled with abbreviations at times. I think we need some guidelines here. For example, we could rename infix-if-bridge-port.c to infix-if-brport.c and then simply use brport_ as prefix for all public functions, same for infix-if-bridge-mcd.c -> infix-br-multicast.c (or mcast) then prefix public functions with brmcast_. All local/private functions do, in my view, not need any prefix. For example ixif_br_mcd_gen() -> brmcast_gen() and ixif_br_mcd_gen_br_vlan -> gen_br_vlan().
One `.tar.gz` extension ought to be enough for everyone.
🔥
|
@wkz might be a good idea to run the coverity job on this branch as well. I noticed it failed this passed weekend. Just go to https://github.com/kernelkit/infix/actions/workflows/coverity.yml and "Run workflow" on the right-hand side, then select your branch. |
TL;DR: > New code, all bugs removed ;) Incremental reconfiguration of bridge related settings had lots of ordering issues. This change tries to remedy that by: - Introducing the concept of "snippets": Instead of having to run C code in the correct order (which is doomed to fail since teardowns typically need to run in reverse), collect related settings in a snippet. Later on, we can then concatenate snippets into a larger file in the proper order. - Refactoring the bridge setup to take advantage of snippets. Now we can call out to functions that _both_ generate the VLAN config _and_ sets some bridge global options, for example. This gets us out of having to pass back settings to the main generation function. - Ensuring that deleted objects are always removed - in the proper order - in the exit action of the previous generation. This was previously not true for VLANs, fix #869. - Generating `mcd`:s config orthogonally from the bridge setup. Since we need to keep track of _all_ bridges, figure out when VLAN uppers exist etc., it becomes _very_ messy to generate the configuration from the diff of a single interface. There were lots of cases here where disabling the querier service on one bridge would disable the whole daemon, thereby disabling the service for other bridges that still had it enabled.
Now that we have a god object, we might as well use it to make our functions signatures a bit more readable.
Clean up bridge-port config generation to align with the bridge upper code. - Remove lots of unnecessary guards that we model guaratees are always met (existance of containers and leafs). This reduces indentation depth and increases readability, IMHO. - Split the generation into smaller pieces, each with a clear purpose. - Try to include all edge cases around the tricky subject of PVID migrations, with the actions running in either exit or init, as appropriate.
In ye olde confd, we basically had `50-init.ip` - and that was fine. Then came containers, VLANs, bridges, etc.; and with it: a whole forest of setup/teardown scripts - and it was not fine anymore. For example: if found an init script running at 61, you would have to grep the source tree and look at _all_ other call sites to get a sense of what runs before and/or after it. Therefore: create enums for the init/exit actions where the order is explicitly stated, and use those for all interface scripts.
Agreement: Avoid abbreviations in exported functions. Use short, prefix-less, names for local functions.
|
Wohoo, syslog support in podman and conmon! 🎉 |
@troglobit: For sure, that would be great. The problem with that seems to be that |
Instead of running initctl reload, which also prematurely enables services which are not supposed to be started until we have run through the netdag and evolved to the next generation. Handle the original use-case of disabling zeroconf is managed by asking finit to stop the service immediately in the exit action, and deferring the cleanup of the service until we call `initctl reload` after the init action of the new generation.
The only reliable way of getting logs from the container at all times is to let conmon be the logging agent. Unfortunately, the k8s-file format does not handle multiline output very well, which is something we often see on the stdout/stderr of containers. Therefore, add proper syslog support to conmon instead, and make sure that podman knows enough about it to pass the information along to conmon.
Now that conmon can do it, make use of it.
No worries mate, I'll pick up the battle axe next. It is time for you, our unsung hero, to rest a bit now, finally! |
Furl answers the question "is this URL serving the content I expect?" Sometimes, we might be asking a server that in the middle of being reconfigured, is crashing, is buggy, etc. - in which case it might RST the connection. In that case the answer to the question is "False" and not "BURN IT ALL DOWN!!"
Description
Checklist
Tick relevant boxes, this PR is-a or has-a: