Add allocation_fallback_strategy option as fallback strategy for per-node strategy

[lhp-nemlig]

Description:

Adds allocation_fallback_strategy option as fallback strategy for per-node strategy, such that targets that are not related to a specific node can be assigned.

The fallback strategy is configurable via the config option allocation_fallback_strategy

Link to tracking Issue(s):

• Resolves: Add fallback for "per-node" allocation strategy #3477

Testing:

Fixed per-node tests to expect non-node target to be assigned according to fallback strategy

Documentation:

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: lhp-nemlig (ea6e956, 92d0069, e1207cd, e62f7d2, 7711a32, 381222c, 12e1282, a5b8dca, 6be618c, d6435e3, 31982b7, ee6e856, ef2d720, 7eabd10, 6f757c3, b7649be, 7d32f7e, bf5a367, 0254654, 0f878f0)

[swiatekm]

This is a breaking change, so we either have to mark it as such, or hide it behind a feature gate. I'd prefer the latter, but am willing to be convinced otherwise. We'll discuss it during today's SIG meeting, feel free to join if you'd like to contribute @lhp-nemlig.

On a more technical note, you'll need to sign the CLA and add a changelog entry before we can merge this PR.

[lhp-nemlig]

This is a breaking change, so we either have to mark it as such, or hide it behind a feature gate. I'd prefer the latter, but am willing to be convinced otherwise. We'll discuss it during today's SIG meeting, feel free to join if you'd like to contribute @lhp-nemlig.

On a more technical note, you'll need to sign the CLA and add a changelog entry before we can merge this PR.

@swiatekm I have changed the PR to add fallback strategy as a config option, making this a non-breaking change as the default is NO fallback.
That makes the changes a little more extensive of course, but I think I have implemented this in a mostly reasonable way :)

[swiatekm]

This is a breaking change, so we either have to mark it as such, or hide it behind a feature gate. I'd prefer the latter, but am willing to be convinced otherwise. We'll discuss it during today's SIG meeting, feel free to join if you'd like to contribute @lhp-nemlig.
On a more technical note, you'll need to sign the CLA and add a changelog entry before we can merge this PR.

@swiatekm I have changed the PR to add fallback strategy as a config option, making this a non-breaking change as the default is NO fallback. That makes the changes a little more extensive of course, but I think I have implemented this in a mostly reasonable way :)

I'm ok with this, but keep in mind that in order for this to actually be usable, you'll also need to add an attribute to the OpenTelemetryCollector CRD. That's not a problem, but since in that case you're changing the public API, it's going to invite more scrutiny during review. Just FYI.

[lhp-nemlig]

This is a breaking change, so we either have to mark it as such, or hide it behind a feature gate. I'd prefer the latter, but am willing to be convinced otherwise. We'll discuss it during today's SIG meeting, feel free to join if you'd like to contribute @lhp-nemlig.
On a more technical note, you'll need to sign the CLA and add a changelog entry before we can merge this PR.

@swiatekm I have changed the PR to add fallback strategy as a config option, making this a non-breaking change as the default is NO fallback. That makes the changes a little more extensive of course, but I think I have implemented this in a mostly reasonable way :)

I'm ok with this, but keep in mind that in order for this to actually be usable, you'll also need to add an attribute to the OpenTelemetryCollector CRD. That's not a problem, but since in that case you're changing the public API, it's going to invite more scrutiny during review. Just FYI.

Ok @swiatekm, I expect to join the SIG meeting and you can tell me what I need to do in order to complete this

[swiatekm]

@lhp-nemlig as per our discussion during the SIG meeting, the simplest way to proceed with this change without needing to add attributes to the CRDs, would be to:

1. Keep your current changes making it configurable on the target allocator.
2. Add a feature flag which, when enabled, would set the fallback to the consistent-hashing strategy.

You can add feature flags here and the target allocator ConfigMap is generated here. Feature flags are global, so you can just import the package, and add the config value for the fallback strategy if the flag is enabled. Does that make sense?

[lhp-nemlig]

@swiatekm Thanks for the clear instructions, I believe everything should be in order now.

[swiatekm]

Looks good to me overall, had some minor comments and nitpicks. I'm fine merging this PR as-is when they're addressed, but in an ideal world we'd also include an e2e test showing the fallback strategy assigning targets without Nodes when the flag is enabled. I'm ok with that happening in a follow-up though, as this is an off-by-default feature flag, and writing these kinds of tests in chainsaw isn't very straightforward.

[swiatekm]

LGTM, thank you for the contribution!

[jaronoff97]

Looks great! Thank you for your contribution 🙇