Add missing affinity to ztunnel

[costinm]

Change-Id: Ie51df7029ce8798785f5e5d68fa31221de474abd

Please provide a description of this PR:

All other charts support affinity - this allows installing ztunnel on select nodes, CNI has a similar option.

I added 2 extra options - if controversial will remove, otherwise will add the same to cni chart.
This makes it easier to install on nodes with a specific label - I used 'ztunnel' as in the developer guide, but
probably should istio.io/mode or something similar. The end goal is to be able to designate a node as 'canary'
and have a new version of cni/ztunnel run only there. It doesn't fully work with helm - will need more work
to not re-create the service account and rbac.

[istio-policy-bot]

😊 Welcome @costinm! This is either your first contribution to the Istio istio repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

[bleggett]

Not against doing this as we need to support blue/green node pool upgrades, but wouldn't we want to do this with taints+tolerations, rather than affinity?

[costinm]

We should have affinity just for consistency. Taints are for a different thing AFAIK - we can use the nodeSelector plus affinity. Removed the custom affinity - added the extra nodeSelector ( also used in other charts, for consistency ).

…

On Thu, May 9, 2024 at 1:38 PM Ben Leggett ***@***.***> wrote: Not against doing this as we need to support blue/green node pool upgrades, but wouldn't we want to do this with taints, rather than affinity? — Reply to this email directly, view it on GitHub <#50965 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2VLCH3TF4MDAAIG6QLZBPNF5AVCNFSM6AAAAABHPKTLACVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBTGM4DIOJTGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[bleggett]

We should have affinity just for consistency. Taints are for a different thing AFAIK - we can use the nodeSelector plus affinity.

Consistency with what? Both ztunnel and istio-cni are node-criticial daemonsets with unique scheduling requirements versus the rest of Istio, as a matter of current state.

I worry that affinity is more of a hint and not really a good pick for node-critical daemonsets - ztunnel is node critical, so every node must have one ztunnel pod, and no more.

If you have node set A and node set B, and you install ztunnel daemonset A and B, you don't merely want to prefer that B goes with B and A goes with A, you want explicit mutual exclusion.

IIRC you can do that with "hard" vs "soft" affinity, but "soft affinity" is basically an invalid form of affinity/footgun for ztunnel, and "hard" affinity is just equivalent to taint/tolerate - so why not just use taint/tolerate to preclude people from using affinity strategies that don't match what ztunnel/cni require?

We don't want scheduling hints we want scheduling requirements, that can be guaranteed to be mutually exclusive.

[costinm]

On Thu, May 9, 2024, 14:24 Ben Leggett ***@***.***> wrote: We should have affinity just for consistency. Taints are for a different thing AFAIK - we can use the nodeSelector plus affinity. Consistency with what? Both ztunnel and istio-cni are node-criticial daemonsets with unique scheduling requirements versus the rest of Istio, as a matter of current state.

Consistency between Istio charts. Istio-cni has affinity settings, just like all others. No need to be inconsistent. Priority and deamon set are not incompatible with affinity. I worry that affinity is more of a hint and not really a good pick for

node-critical daemonsets - ztunnel is node critical, so every node *must* have one ztunnel pod, and no more.

Agree with first part - not completely with the second. We should be able to handle nodes without a ztunnel, will take a bit of work but when we add VMs and other environments we can't assume ztunnel is everywhere and same version. Pods that require ztunnel may need affinity or other scheduling options, but when we tackle live migrations without data loss - we will likely need this. Even 2 ztunnel pods ( and 2 CNIs !) are possible if we take care of coexisting. If you have node set A and node set B, and you install ztunnel daemonset A

and B, you don't merely want to *prefer* that B goes with B and A goes with A, you want explicit mutual exclusion. IIRC you *can* do that with "hard" vs "soft" affinity, but "soft affinity" is basically an invalid form of affinity for ztunnel, and "hard" affinity is just equivalent to taint/tolerate - so why not just use taint/tolerate to preclude people from using affinity strategies that don't match what ztunnel/cni require?

I think taint is a good strategy for some cases. Not sure it works for everyone - we want pods to still be assigned to the node ( without having to tolerate that taint). Maybe it is possible - but I don't think that's a reason to reject having an option to use affinity with ztunnel. —

…

Reply to this email directly, view it on GitHub <#50965 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2XJ2LKGUX6FV56MGLDZBPSQNAVCNFSM6AAAAABHPKTLACVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBTGQ2DCOBWHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[bleggett]

I think taint is a good strategy for some cases. Not sure it works for everyone - we want pods to still be assigned to the node ( without having to tolerate that taint). Maybe it is possible - but I don't think that's a reason to reject having an option to use affinity with ztunnel.

I don't love using affinity for node-critical singletons but yeah - to your point we already have them on istio-cni (right or wrong) and we can iterate.

[howardjohn]

LGTM with caveats that having a template-able field != recommending usage of it :-)

[howardjohn]

/retest