-
Notifications
You must be signed in to change notification settings - Fork 674
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
collection: Add ReplaceMaps to CollectionOptions #646
collection: Add ReplaceMaps to CollectionOptions #646
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine, wdyt @ti-mo?
collection.go
Outdated
var ok bool | ||
|
||
if m, ok = cl.opts.OverrideMaps[mapName]; ok { | ||
if err := mapSpec.checkCompatibility(m); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move this check into newCollectionLoader and then initialise cl.maps with opts.OverrideMaps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should happen to the maps in opts.OverrideMaps when a call to NewCollectionWithOptions()
/ LoadAndAssign()
fails? Should they be closed? I'm asking because if we initialize cl.maps
with opts.OverrideMaps
those will be closed in case of error in (cl *collectionLoader) cleanup()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should happen to the maps in
opts.OverrideMaps
when a call toNewCollectionWithOptions()
/LoadAndAssign()
fails? Should they be closed?
Probably not. We don't own those Maps, and it will cause bugs in user code (and cause data loss if not pinned).
those will be closed in case of error in
(cl *collectionLoader) cleanup()
.
Still the case in the current proposal, right? You're pulling a Map from OverrideMaps and putting it into cl.maps
below.
I agree with having the compatibility check here, but just make loadMap()
early-return the Map here if it exists in the overrides, without assigning it to cl.maps
. The overrides then effectively serve as a second Map cache.
That will also make the diff a bit shorter, since m, err, ok don't need to be explicitly declared, and the else
block will no longer be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch re: what happens on error. This leads to the follow up question: what should happen if I use NewCollectionWithOptions
directly (so not via LoadAndAssign
)? In that case coll.Maps
will contain references to Replacements
. Who owns those? Maybe we need to Clone()
the replacement and stick it in cl.maps after all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still the case in the current proposal, right? You're pulling a Map from OverrideMaps and putting it into cl.maps below.
Yes, almost the same. The small difference is that I'm not pulling all maps at once.
Who owns those?
It's a good question, not only in the error case but in general:
m := ebpf.NewMap(...)
col := ebpf.NewCollectionWithOptions(..., ebpf.CollectionOptions{MapReplacements: ... "mymap": m})
....
col.Close()
// should m be valid here?
Maybe we need to Clone() the replacement and stick it in cl.maps after all?
It seems it'd simplify things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need to
Clone()
the replacement and stick it in cl.maps after all?
Yep, sounds like the way to go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's OK to clone in loadMaps
, but the compat check should be in newLoader
, since loadMaps
isn't called for all maps when using LoadAndAssign. Right now the following doesn't return an error:
replacement := map[string]*Map{ "foo": ..., "bar": ... }
var objs struct {
Foo *Map `ebpf:"foo"`
}
x.LoadAndAssign(&objs, nil) // bar is never checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've trying to reproduce this problem but I can't.
since loadMaps isn't called for all maps when using LoadAndAssign.
If loadMaps is not called it means the map is not used, hence not replace and there's nothing to worry about its compatibility. What I am missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If loadMaps is not called it means the map is not used
Correct.
What I am missing?
Lorenz wants to make sure all given MapReplacements are always compatibility-checked, regardless of whether they are loaded or not. This avoids any surprises.
I'll pick this up in a follow-up together with the ReplaceMaps() deprecation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, good to go with a few changes. 👍 Let's use the term 'replacement' instead of 'override' to stick to the existing terminology.
TBD, could happen in another PR: should we deprecate |
56da005
to
7a726ee
Compare
I think yes. I'm also wondering if |
Great idea! Then a CollectionSpec doesn't need to be copied anymore when inserting many instances of it. |
7a726ee
to
2ce6cd0
Compare
I updated the PR to call Clone() and extended the test cases to check user-provided maps are not closed. |
Caveat: this needs some thinking to make it work nice with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good! Just a few nits left. 👌
Add a new way to replace references to specific maps when creating a new collection by introducing a MapReplacements field to the CollectionOptions struct. The goal of this functionality is the same provided by RewriteMaps(), but this new approach plays nicely with bpf2go as cilium#517 is fixed. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to make sure #646 (comment) isn't lost.
2ce6cd0
to
d5dcf5e
Compare
Fixes #517