RFC: BPFNode for node configuration #38022
Conversation
commit d0c87d2 ("datapath/config: generate Go config scaffoldings for all bpf objects") generated a set of config structs used for runtime configuration of various BPF objects (lxc, host, etc.). While this is a great start for purely endpoint config, it's less clear how it helps us start migrating compile-time #defines out of node_config.h/cDefinesMap It would be nice to have one BPFNode config object similar to the auto-generated config objects like BPFLXC, BPFHost, etc. which contains the same kind of configuration previously contained in node_config.h. With that, we could incrementally migrate logic and config out of WriteNodeConfig()/cDefinesMap/node_config.h and into code that sets up this struct. That's what this commit seeks to do: 1. Introduce a macro called DECLARE_NODE_CONFIG() which works the same way as DECLARE_CONFIG, except that it adds a "kind:node" tag to the declared variable instead of "kind:object". 2. Generate BPFNode from all the "kind:node" variables and have dpgen embed BPFNode inside all object configs. 3. Embed BPFNode inside LocalNodeConfiguration and plumb it through everywhere we instantiate an object config. 4. Stub out populateBPFNodeConfig() which is where logic will go to set up the embedded BPFNode struct. Any node config would be declared inside bpf/config/global.h so that it's available across all objects. The idea is to slowly move logic out WriteNodeConfig() and into populateBPFNodeConfig() as config is moved from node_config.h into bpf/config/global.h. The embedded BPFNode struct makes this a fairly direct migration; instead of our logic adding properties to cDefinesMap it's setting properties in BPFNode. Signed-off-by: Jordan Rife <jrife@google.com>
IPV4_LOOPBACK was chosen here since it's pure config; it's the simplest possible example of a migration of configuration from node_config.h to BPFNode. Things get trickier for things like ENABLE_IPV4 or the collection of flags used to decide whether or not to define ENABLE_PER_PACKET_LB. Even so, this general pattern should still work as a mechanism to pass along config. Signed-off-by: Jordan Rife <jrife@google.com>
|
@jrife Thanks for the patch! I have some commits laying around that do pretty much this, but I wanted to get #37469 in first to cut down on the size of WriteNodeConfig before we start migrating things out. There's not much point in assigning bogus defaults to variables (like addresses) that don't affect control flow. ASSIGN_CONFIG should be used sparingly since it means the value can't be overridden from bpf tests. In my patches, I opted for NODE_CONFIG for brevity, copied the macro declaration instead of wrapping it and added a new config/node.h that wraps node_config.h. Also see dpgen's -embed parameter. BPFNode shouldn't be embedded in LocalNodeConfig -- it should be output from a method or free function taking a LNC instead. There should be a single way to output a BPFNode based on a LocalNodeConfig. We need to play around with a few options to see where it fits best in the pipeline. There's also the topic of documentation; this will need clear guidelines along with warning bells in the existing code paths to stop adding new #defines, along with a few examples. Another thing I wanted to add first is proper support in dpgen for struct and array variables to do away with the _1 _2 hacks for mac_t and v6_addr. This would be useful for the existing *Config structs already, there's a few cases there. Currently on PTO, feel free to hack on this further in the meantime. I'll revive my patch set when I'm back, not sure in which state I left it. Will sync up with you when I do. Thanks again! |
|
Hi @ti-mo , Thanks for taking a look. Hope you are enjoying your PTO.
Makes sense. I actually went back and forth a bit on using
Funnily enough, I had a
Thanks, I will check it out.
Yeah, looking at it again, the embedding is a bit unsightly. I'll play around with it and see if I can come up with something a bit cleaner. The main idea would be to have some function where the logic lives for translating an LNC + any other config to a Anyway, I'll take another look and see what I can come up with. Hopefully this isn't too redundant, considering you've already started working on something similar. I'm happy to discuss more to see where I can help out with clang-free. |
|
@jrife To get you involved, I'll tag you as a reviewer on my upcoming PR for the node config scaffolding, ETA next week. Once that lands, we can start porting over WriteNodeConfig, which will be a ton of work we'll definitely need some help with. There's some hive-related work to be done by @dylandreimerink there as well, since some defines have already moved to Simultaneously, I'll also start mapping out all the smaller tasks ahead so you can pull in chunks of work without us risking stepping on each others' toes. Does that sound okay? |
Sure, sounds good. Thanks for keeping me in the loop. |
|
Note to self: #38244 implements this. |
Revive my older PR, cilium#38022, since support for NODE_CONFIG was merged and move IPV4_LOOPBACK out of node_config.h. Change-Id: I076a80e3055b04c74781d8785964b29349b219fa Signed-off-by: Jordan Rife <jrife@google.com>
Revive my older PR, cilium#38022, since support for NODE_CONFIG was merged and move IPV4_LOOPBACK out of node_config.h. Signed-off-by: Jordan Rife <jrife@google.com>
Revive my older PR, cilium#38022, since support for NODE_CONFIG was merged and move IPV4_LOOPBACK out of node_config.h. Signed-off-by: Jordan Rife <jrife@google.com>
Revive my older PR, cilium#38022, since support for NODE_CONFIG was merged and move IPV4_LOOPBACK out of node_config.h. Signed-off-by: Jordan Rife <jrife@google.com>
Revive my older PR, cilium#38022, since support for NODE_CONFIG was merged and move IPV4_LOOPBACK out of node_config.h. Signed-off-by: Jordan Rife <jrife@google.com> Signed-off-by: Timo Beckers <timo@isovalent.com>
Revive my older PR, cilium#38022, since support for NODE_CONFIG was merged and move IPV4_LOOPBACK out of node_config.h. Signed-off-by: Jordan Rife <jrife@google.com> Signed-off-by: Timo Beckers <timo@isovalent.com>
Revive my older PR, cilium#38022, since support for NODE_CONFIG was merged and move IPV4_LOOPBACK out of node_config.h. Signed-off-by: Jordan Rife <jrife@google.com> Signed-off-by: Timo Beckers <timo@isovalent.com>
Revive my older PR, cilium#38022, since support for NODE_CONFIG was merged and move IPV4_LOOPBACK out of node_config.h. Signed-off-by: Jordan Rife <jrife@google.com> Signed-off-by: Timo Beckers <timo@isovalent.com>
Revive my older PR, #38022, since support for NODE_CONFIG was merged and move IPV4_LOOPBACK out of node_config.h. Signed-off-by: Jordan Rife <jrife@google.com> Signed-off-by: Timo Beckers <timo@isovalent.com>
In this week's community meeting I mentioned my interest in helping with the clang-free effort and asked where I could jump in. This is a milestone I'd really like to see the project reach. @dylandreimerink shared a bit about the current state and next steps, and @joestringer summarized some of the main talking points here. To get a better sense of the history and current direction, I've been looking at some of the old conversations like this one (#27320) which outlines some ideas for replacement of conditional compilation with the verifier's support for dead code elimination and some of the recent developments like the work done by @ti-mo to introduce dpgen+Go config structs.
As I thought about how I could help drive clang-free forward and how I might start migrating some of the compile-time config in node_config.h to something like
DECLARE_CONFIG(), it seemed there was a bit of a gap to bridge. While it seemsbpf/config/global.his where shared config is supposed to go by convention, in the endBPFLXC,BPFHost, etc. are just flat structs. It's a bit unclear how to migrate logic and config fromWriteNodeConfig(), which sets upcDefinesMapand writesnode_config.h, to this collection of object configs. Sure, you can put aDECLARE_CONFIGinsidebpf/config/global.hand it shows up in all the object config structs, but plumbing node config into each and every type is is a bit of a pain. Instead, it would be nice to just have one place where we configure aBPFNodeconfig struct that we plumb through everywhere. This is more-or-less in line with the current code structure.This is an RFC. I may be completely off-base on the intended use and direction of the config framework or missing a simpler way that's right in front of me, but wanted to share my observations and ideas to get some feedback. Here are the highlights of this PR:
bpf/config/global.hbut differentiated from object config usingDECLARE_NODE_CONFIG(). This adds a"kind:node"tag to the declared variable instead of"kind:object". All"kind:node"variables go intoBPFNode.BPFNodestruct.BPFNodeis also embedded inLocalNodeConfiguration, so we can plumb it through down to where the object config structs are instantiated (pkg/datapath/loader/loader.go).populateBPFNodeConfig()is whereBPFNodeis actually created and configured. The intent is to be able to slowly migrate logic out ofWriteNodeConfig()and intopopulateBPFNodeConfig()as config moves fromnode_config.hand intobpf/config/global.h.To illustrate the intended use, I (mostly arbitrarily) started by migrating
IPV4_LOOPBACK, since it's just pure config. Obviously things get trickier with flags likeENABLE_IPV4with the considerations around code and map elimination. This is something that would have to be addressed later on while migrating these flags toBPFNode.One possible concern here is that with this approach config is no longer conditionally defined. In the particular case of
ipv4_loopback, this will be loaded even ifENABLE_IPV4isn't set. I'm not sure how concerned everyone is with pruning unused variables, but if so this might be something that requires a bit of a rework.Thanks for any feedback!
-Jordan