Skip to content
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

Merged

Conversation

mauriciovasquezbernal
Copy link
Contributor

@mauriciovasquezbernal mauriciovasquezbernal commented Apr 27, 2022

    collection: Add MapReplacements to CollectionOptions
    
    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 https://github.com/cilium/ebpf/issues/517
    is fixed.

Fixes #517

Copy link
Collaborator

@lmb lmb left a 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 {
Copy link
Collaborator

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.

Copy link
Contributor Author

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().

Copy link
Collaborator

@ti-mo ti-mo Apr 29, 2022

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?

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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!

Copy link
Collaborator

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

Copy link
Contributor Author

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?

Copy link
Collaborator

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.

collection.go Outdated Show resolved Hide resolved
map.go Outdated Show resolved Hide resolved
collection.go Outdated Show resolved Hide resolved
collection.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ti-mo ti-mo left a 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.

@lmb
Copy link
Collaborator

lmb commented Apr 29, 2022

TBD, could happen in another PR: should we deprecate RewriteMaps in favour of this?

@mauriciovasquezbernal
Copy link
Contributor Author

TBD, could happen in another PR: should we deprecate RewriteMaps in favour of this?

I think yes. I'm also wondering if RewriteConstants should also follow this approach (to have a Constants field in CollectionOptions)?

@ti-mo
Copy link
Collaborator

ti-mo commented Apr 29, 2022

I'm also wondering if RewriteConstants should also follow this approach (to have a Constants field in CollectionOptions)?

Great idea! Then a CollectionSpec doesn't need to be copied anymore when inserting many instances of it.

@mauriciovasquezbernal
Copy link
Contributor Author

I updated the PR to call Clone() and extended the test cases to check user-provided maps are not closed.

@lmb
Copy link
Collaborator

lmb commented Apr 29, 2022

I'm also wondering if RewriteConstants should also follow this approach (to have a Constants field in CollectionOptions)?

Caveat: this needs some thinking to make it work nice with bpf2go. I'd like to generate a type constants struct and be able to pass that to NewCollection / LoadAndAssign somehow. For that to work CollectionOptions.Constants would have to be interface{}. If we want to replace RewriteConstants at the same time we'd need it to be map[string]interface{} however.

Copy link
Collaborator

@ti-mo ti-mo left a 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. 👌

collection.go Outdated Show resolved Hide resolved
collection_test.go Outdated Show resolved Hide resolved
collection_test.go Show resolved Hide resolved
collection_test.go Outdated Show resolved Hide resolved
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>
Copy link
Collaborator

@lmb lmb left a 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.

@ti-mo ti-mo merged commit 96aa1a7 into cilium:master May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LoadAndAssign fails after RewriteMaps
3 participants