prog: avoid verifier loop of death

[ti-mo]

Supersedes #1679.

prog: factor retry decision into separate testable function

This commit special-cases LogDisabled into a clause without a retry loop since
it doesn't need a buffer allocated. It also simplifies the logic somewhat.

A follow-up commit will add another separate clause for a new LogSizeOverride
CollectionOption to factor out even more complexity related to buffer sizing.

The main purpose of this commit is to extract the decision-making logic around
retries. Instead of directly manipulating loop control flow, a function is
introduced that issues a retry verdict, and it can be tested indendently of
the program load action.

prog: give up when max verifier log size is reached

Previously, loading a program would loop forever if the log didn't fit
in the kernel's max log size.

prog: prevent prog_load loop of death

To limit the damage of any future regressions, limit the attempts we'll take
to load a program.

@lmb I eventually decided against deprecating LogSizeStart because it has utility beyond just break-glass cases, for example to reduce the amount of attempts the Cilium verifier tests take to obtain the full buffer, which is often quite large. We don't want to hardcode a buffer size there because the size varies significantly across programs and the environment is somewhat resource-constrained. It also ended up being more code; as of this proposal it's quite concise and testable, and I'm happy with it.

[brycekahle]

I tested and did not receive the same infinite loop we were seeing before.

[lmb]

I eventually decided against deprecating LogSizeStart because it has utility beyond just break-glass cases, for example to reduce the amount of attempts the Cilium verifier tests take to obtain the full buffer, which is often quite large.

That tells me that the current approach just doesn't work very well and we should try to do better. I have a firm belief that making log size at all configurable was a mistake from the get go.

So: how large is "often quite large"?

[lmb]

I find this whole factor thing not very intuitive. This is to avoid overflow?

[ti-mo]

Do you have an alternative suggestion? The intention is for this to keep working if/when maxVerifierLogSize gets bumped to MaxUint32. Currently, we can't rely on a u32 overflow happening, detecting it, and clamping the value to maxVerifierLogSize. Either way, that's more code and more obtuse than this solution. I'll go ahead with the current version for now.

[lmb]

This doesn't preserve the behaviour that we don't retry if LogSize >= LogTrueSize. I'm also not sure why this only happens on ENOSPC? LogTrueSize should always be populated?

[brycekahle]

LogTrueSize is not available on older kernels

[ti-mo]

If the kernel returns ENOSPC, LogSize is smaller than LogTrueSize by definition, and we just need to allocate whatever LogTrueSize specified and retry. I don't think what I wrote here will result in a retry if LogSize > LogTrueSize. What am I missing?

[brycekahle]

Where do we stand on this? Anything you need me to do? We'd like to update to get some other fixes/improvements, but not having this is a blocker.

[ti-mo]

Just got back from PTO, I'll try to get this in tomorrow.

[ti-mo]

That tells me that the current approach just doesn't work very well and we should try to do better. I have a firm belief that making log size at all configurable was a mistake from the get go.

I think at the time, you just picked whatever libbpf did. Over the years, this piece of code grew more and more complex. We added (and then removed) VerifierError.Truncated. Then LogTrueSize came and made things worse, although improving things in the long term. I remember a conversation years ago when I migrated the Cilium agent over to the lib, where we concluded to kick the can down the road and let the agent deal with automatic buffer resizing. Then years later you suddenly changed your mind and added resizing to the lib anyway.

Meanwhile, we only made half-hearted attempts at reining in the complexity, so we kept shipping regressions. I don't think it's helpful blaming this on a single decision made half a decade ago when the landscape was very different. :) In the current proposal, the starting size of the buffer is plumbed through in a way that makes it easy to remove if we really want to later on. If anything, blame it on the kernel API. 😉

So: how large is "often quite large"?

Large enough to matter. 😄 It doesn't really matter what the actual size is, this will vary per use case. The bigger the delta from minVerifierLogSize, the more retries have to be made. Think of the poor CPUs. 😢

Made a few small tweaks based on suggestions. Going for merge now, to at least stop the bleeding. We can make more changes later if you really feel strongly about it.