cilium
collection: Add Pin/Unpin/Assign methods
Allow pinning entire object files at once instead of on a per-map or per-prog basis. Also make it work for bpf2go users.
See individual commits for motivation and details.
Seems like your code mostly deals with Collection, since you're fleshing out the API. Can you give some context why that is better than using CollectionSpec and fiddling with the specs?
Some issues I see with the Pin / Unpin changes:
* "Duplicates" MapOptions.PinPath for Maps. When is it appropriate to use one or the other?
According to the docs, PinPath will reuse the maps when possible. This is undesirable for cases where you want to re-construct an entire object for reconfiguration. Delta updates can cause the program to transition through invalid states. In my mind, Pin() does exactly what it says -- pin and nothing else.
* Encodes a non-standard behaviour for "special" maps like `.bss`
Yeah, true. libbpf-rs also has special handling. And from grepping, it looks like the rest of this codebase does too:
$ rg ".bss"
collection.go
738: // Skip .rodata, .data, .bss, etc.
elf_reader.go
96: case sec.Name == ".bss" || sec.Name == ".data" || strings.HasPrefix(sec.Name, ".rodata"):
1090: // NOBITS sections like .bss contain only zeroes, and since data sections
elf_reader_test.go
154: if key == ".bss" || key == ".data" || strings.HasPrefix(key, ".rodata") {
testdata/loader.c
112:static volatile unsigned int key1 = 0; // .bss
btf/btf.go
789:// used as a proxy for .bss, .data and .rodata map support, which generally
cmd/bpf2go/output.go
156: // Skip .rodata, .data, .bss, etc. sections
179: // Avoid emitting .rodata, .bss, etc. for now. We might want to
Not sure if there's a better way to handle it.
* Substituting unsupported characters creates a more complicated API, since at some point we might have to allow custom behaviour here. Better to return an error IMO.
Ack.
* Comes up with a scheme for Program pinning. Do we need a `ProgramOptions.PinPath` equivalent?
Hmm, good point. It'd be nice for symmetry, but I can't really think of any use cases. Maybe it's better to wait for a use case to add?
This is undesirable for cases where you want to re-construct an entire object for reconfiguration. Delta updates can cause the program to transition through invalid states.
Ah! So you want to use this to do "upgrades" of an entire collection? Can you describe the full flow you have in mind? I've been playing with ideas for this as well, so far I probably would've added a "map upgrade" callback to MapOptions.
And from grepping, it looks like the rest of this codebase does too:
I know, in the ELF reader it's impossible to avoid, the other places are kind of unfortunate. I've had vague ideas of hiding the special maps via instruction metadata or similar, but never got round to implementing it. If possible I'd like to avoid adding more special based on names, but it's not a show stopper. Worst case we can centralise the name checks in an uneported method on MapSpec.
It'd be nice for symmetry, but I can't really think of any use cases.
I see CollectionOptions as a declarative version of the imperative APIs. If there is no use case for the declarative version, why do you need the imperative one? Could you add PinMaps, UnpinMaps instead?
No worries. I still think it's useful but in the meantime I think I have a workaround. I've kinda paged out this out so I forget how I was planning on using this a bit. If you still think it's useful I can rebase and we can work on getting this in.
@danobi Would you be willing to remove Collection.Pin and .Unpin and only keep Assign()? I may have a use case in Cilium for it. It's also relatively straightforward and similar to existing APIs.
The pin/unpin stuff is what I would personally implement in my app instead of pushing it into the library. It ignores the pinning flag, pinning programs is pretty uncommon, and overall it feels like it implements a particular use case instead of providing a widely-useful abstraction.
I think Assign() is already in via https://github.com/cilium/ebpf/commit/5bb4c2f1ac09bae4db1a322f8e9a392c553ffdb1