feat!: Add new ListenerClass.pinnedNodePorts field

[sbernauer]

Description

Part of stackabletech/issues#770
Implementation is in stackabletech/listener-operator#340

Extend ListenerClass CRD with an extra field that allows configuring whether pinning should or should not be used for NodePorts, as this causes problems on (cloud) environments with rotating nodes.

Definition of Done Checklist

• Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
• Please make sure all these things are done and tick the boxes

Author

• Changes are OpenShift compatible
• CRD changes approved
• CRD documentation for all fields, following the style guide.
• Integration tests passed (for non trivial changes)
• Changes need to be "offline" compatible

Reviewer

• Code contains useful comments
• Code contains useful logging statements
• (Integration-)Test cases added
• Documentation added or updated. Follows the style guide.
• Changelog updated
• Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

• Feature Tracker has been updated
• Proper release label has been added

[NickLarsenNZ]

What (k8s resources changes) does it result in? Some changes to generated Services or just the generated Listener?

[sbernauer]

What (k8s resources changes) does it result in?

Actually the csi::v1::Volume, which in turn influences the created PVC (I guess). Feel free to have a look at rust/operator-binary/src/csi_server/controller.rs in stackabletech/listener-operator#340

[sbernauer]

Please vote on this comment

[NickLarsenNZ]

What (k8s resources changes) does it result in?

Actually the csi::v1::Volume, which in turn influences the created PVC (I guess). Feel free to have a look at rust/operator-binary/src/csi_server/controller.rs in stackabletech/listener-operator#340

I'm just now wondering if Stickyness is the correct naming.

I haven't had time to dig deep, and this is in voting now, so I feel a bit rushed.

Is this actual "stickyness" which is typically used to mean directing traffic for the same session to the same instance. This seems more like it's about "Permanency" "Node Pinning" or "Strict Node Affinity" (I see sticky has been used in the PR you linked, so this isn't a new thing, but I do want to discuss whether the naming is appropriate).

I feel like the description lacks context and instead links a bunch of things. Are you able to update the description with the missing context?

---

I left a related comment on the PR about the field name and avoiding "sticky"/"stickiness": https://github.com/stackabletech/listener-operator/pull/340/files#r2418860493

[sbernauer]

I'm just now wondering if Stickyness is the correct naming.

Good question, I'm no native speaker.
This only affect NodePorts!

The Pod "sticks" to one single node.
=> a.) stickyNodePorts

The Pod is "pinned" to a particular node.
=> b.) pinningNodePorts
=> c.) nodePinningNodePorts
=> d.) ???

Both work for me, I let the native speakers decide 😅 (a.) sounds a bit more natural to me)

[sbernauer]

I feel like the description lacks context and instead links a bunch of things. Are you able to update the description with the missing context?

The parent issue stackabletech/issues#770 should hopefully contain all the motivation and details on making the stickiness/pinning configurable. I would prefer to keep one well-maintained issue instead of cluttering things on issues, decisions and PRs. Pls feel free to ask if there are any missing things.

[NickLarsenNZ]

I feel like the description lacks context and instead links a bunch of things. Are you able to update the description with the missing context?

The parent issue stackabletech/issues#770 should hopefully contain all the motivation and details on making the stickiness/pinning configurable. I would prefer to keep one well-maintained issue instead of cluttering things on issues, decisions and PRs. Pls feel free to ask if there are any missing things.

My point is that the decision ticket could have an executive summary so we don't need to click a link straight away (links are handy for the deeper context).

[sbernauer]

Added a one-liner, is that sufficient? :)

[NickLarsenNZ]

I'm just now wondering if Stickyness is the correct naming.

Good question, I'm no native speaker. This only affect NodePorts!

The Pod "sticks" to one single node. => a.) stickyNodePorts

The Pod is "pinned" to a particular node. => b.) pinningNodePorts => c.) nodePinningNodePorts => d.) ???

Both work for me, I let the native speakers decide 😅 (a.) sounds a bit more natural to me)

My preferences (per comment) are nodePinning or strictNodeAffinity.

To support that, the top line of the CRD explains:

Whether a Pod exposed using a NodePort should be pinned to a specific Kubernetes node.

and the field is a boolean.

[sbernauer]

My preferences (per comment) are nodePinning or strictNodeAffinity.

Yes, but these names are for me missing the information that only NodePorts are affected by this property.
How would you call them? nodePortNodePinning or nodePortStrictNodeAffinity?

[NickLarsenNZ]

My preferences (per comment) are nodePinning or strictNodeAffinity.

Yes, but these names are for me missing the information that only NodePorts are affected by this property. How would you call them? nodePortNodePinning or nodePortStrictNodeAffinity?

If the pod isn't tied to the node (which I thought it was, until the PVC is deleted), then perhaps pinnedNodePorts?

[sbernauer]

Sure, pinnedNodePorts works for me

[NickLarsenNZ]

LGTM

[NickLarsenNZ]

LGTM, good catch

[nightkr]

This is a dangerous default. Having it be configurable is one thing (I'd probably still be against it, but my vague opinion doesn't really count for much anymore :p), but unpinned + NodePort should be a big warning sign that you're doing something Weird and are probably using the wrong tool (and about to walk into a rake).

[lfrancke]

I'm the person who looked at customer issues we had and said "let's change it". To me it sounds totally sane - otherwise I probably wouldn't have asked for this. Can you elaborate what I'm missing?

We've outlined the reasons a bit here: stackabletech/issues#770

And let me close by saying that your opinion does and will count going forward. Thanks for still looking into this. We do have jobs available if you'd like to be paid to work on this ;-)