ebpf icon indicating copy to clipboard operation
ebpf copied to clipboard

ErrMapIncompatible should contain additional information

Open ti-mo opened this issue 2 years ago • 5 comments

In https://github.com/cilium/cilium/pull/22693, Cilium will start relying exclusively on the library to tell it when a map is incompatible with an existing pinned map, but right now ErrMapIncompatible is a little vague. Callers need to open the old map explicitly, inspect its info, and work out the differences with the MapSpec.


Updated to echo https://github.com/cilium/ebpf/pull/1093#issuecomment-1640078398.

I think there are 2 issues with the existing code:

  • No clear communication (towards the user) of what 'expected' and 'got' means in (expected type %v, got %v). We should settle on language that clarifies which side is the existing Map and which is the incoming Spec.
  • Only one error is surfaced at a time, being the first-found one.

As a bonus, it would be nice if this would return a new ebpf.IncompatibleMapError{OldType, NewType ebpf.MapType, ...} that .Unwrap()s to ErrMapIncompatible so it can be used like:

var merr *ebpf.IncompatibleMapError
if errors.As(err, &merr) {
  if merr.OldType == ebpf.ArrayOfMaps && merr.NewType == ebpf.HashOfMaps {
    // Convert the old map from ArrayOfMaps to HashOfMaps here..
  }
}

This would be really useful to trigger up/downgrade logic.

Also, .Error() should only return a diff of the map's properties, not a dump of all old/new properties. That's just something I hacked up in Cilium to move ahead with the changes there.

ti-mo avatar May 05 '23 13:05 ti-mo

@ti-mo hi I am new to this org and want to contribute to this issue can you please guide me through it

swastik959 avatar May 15 '23 13:05 swastik959

@swastik959 There's a short document at https://github.com/cilium/ebpf/blob/master/CONTRIBUTING.md. Do you have any specific questions?

ti-mo avatar May 25 '23 09:05 ti-mo

Hello @ti-mo, I would like to work on this if no one is actively working on this.

sharathsivakumar avatar Jun 09 '23 17:06 sharathsivakumar

@sharathsivakumar Sure thing! Assigned the issue to you.

ti-mo avatar Jun 12 '23 12:06 ti-mo

Thanks!

sharathsivakumar avatar Jun 13 '23 15:06 sharathsivakumar

Closing this for now, since it's actually quite tricky to implement. This should happen if / when we use the info from the cilium side.

lmb avatar Mar 13 '24 11:03 lmb