ebpf: delete existing pinned map if incompatible with the spec

[jibi]

This PR updates the OpenOrCreate() method of the ebpf package to
gracefully handle the case of a pinned map that is incompatible with the
map's spec passed to the method.

Rather than just returning the error returned by NewMapWithOptions(), we
now log a warning and try to delete and recreate the pinned map.

This mimics the behaviour of the old OpenOrCreate() method of the bpf
package.

Fixes: #15629

[jibi]

test-me-please

[jibi]

retest-net-next

[aanm]

Is it safe to do this for all maps? I also assume this can't be done "atomically"?

[jibi]

Is it safe to do this for all maps?

That's what we are already doing with all the other maps that are opened using the bpf.OpenOrCreate() method:

cilium/pkg/bpf/map_linux.go

Lines 503 to 505 in 648f4f0

	// the Map. If the existing map's attributes such as map type, key/value size,
	// capacity, etc. do not match the Map's attributes, then the map will be
	// deleted and reopened without any attempt to retain its previous contents.

I'm not saying that this is necessarily the best approach, but since the aim of the ebpf package is to (at least initially) mimic the bpf package (to avoid introducing another dimension of complexity while we migrate to that) I think we should stick with this approach for now

edit: I just found that the bpf package has also an Open() method (in addition to the OpenOrCreate()) which will return an error in case the map's spec is incompatible with the existing map

I also assume this can't be done "atomically"?

That's my understanding, yes

[christarazi]

I haven't looked deeply at the code yet, but I glanced quickly and couldn't tell if the PR addresses this comment: #15629 (comment)

[jibi]

I haven't looked deeply at the code yet, but I glanced quickly and couldn't tell if the PR addresses this comment: #15629 (comment)

See my previous comment #15832 (comment)

In case the existing map is incompatible with the provided spec and we want to error on that, I believe we should use the bpf.Open() method (which has not been re implemented yet in the ebpf package as we haven't ported any map that requires that)

[aanm]

@joestringer will provide a look at this PR to understand the implications of it.

[jibi]

@joestringer will provide a look at this PR to understand the implications of it.

Thanks, I'll quickly recap all the context around this change as I fear it might be scattered around at this point:

• before the switch to cilium/ebpf, the cilium_metrics map was being opened/created with the bpf.OpenOrCreate() method
• according to the documentation, this method would delete and recreate the map if it was incompatible with the provided spec:

  cilium/pkg/bpf/map_linux.go

  Lines 503 to 505 in 648f4f0

  	// the Map. If the existing map's attributes such as map type, key/value size,
  	// capacity, etc. do not match the Map's attributes, then the map will be
  	// deleted and reopened without any attempt to retain its previous contents.

• when switching the cilium_metrics map to the cilium/ebpf library I rewrote this method under the ebpf package, missing the fact that the old bpf.OpenOrCreate() method would delete and recreate the map in case it was incompatible with the provided spec
• this led to the failure documented in Add more user-friendly log messages on map creation #15629
• this PR addresses this by updating ebpf.OpenOrCreate() to mimic the old bpf.OpenOrCreate() so I don't think I'm introducing anything new, but I'm rather just fixing the new method to be alligned with the behavior of old one
• (moreover, in case we need to actually handle the case of a mismatch between the map's spec and the pinned map, we should use the bpf.Open() method, which has not been ported yet to the ebpf package as we haven't ported yet any map that requires this)

[christarazi]

LGTM, one minor nit

[jibi]

test-me-please

[joestringer]

Thanks for the recap, that was useful framing to consider these changes.

I went through the call stack for both cases and noticed a couple of remaining minor differences which I've highlighted below.

[joestringer]

I think we're still missing one other difference between the implementations: pkg/bpf/bpf_linux.go:OpenOrCreateMap() will attempt to create the parent directory here before pinning; from a brief search through the ebpf library I didn't see the same logic.

Might not be too relevant for this PR depending on the order of map creation but we'll want to fix this up at some point before we fully switch libraries over.

[jibi]

Yep, I initially ignored it as cilium_metrics is not the first pinned map opened by the agent, but since we are on it I just added the logic to ebpf.OpenOrCreate()

[jibi]

test-me-please

[joestringer]

Thanks, just one remaining nit (with a few occurrences; we used to lint this out of the codebase but it seems we lost that linter somewhere along the way). Once that's addressed, LGTM.

[joestringer]

Usually we use .WithError() to include an individual error, so that if a log shows up multiple times with different parameters, it is possible to correlate the common pieces in structured logging systems (eg the message is the same across all, but the error itself may vary). This can also help with grepping for an error as we can always tell that the msg= part is static text:

Suggested change

	log.WithField("map", m.spec.Name).
	Warnf("Removing map to allow for property upgrade (expect map data loss): %v", errors.Unwrap(err))
	log.WithField("map", m.spec.Name).
	WithError(err).Warn("Removing map to allow for property upgrade (expect map data loss)")

ref. #14156

[jibi]

test-me-please