New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add missing affinity to ztunnel #50965
Conversation
Change-Id: Ie51df7029ce8798785f5e5d68fa31221de474abd
😊 Welcome @costinm! This is either your first contribution to the Istio istio repo, or it's been You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
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? |
Change-Id: If7b7232f11c833b69936dd81c015add621084d26
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:
***@***.***>
|
Change-Id: Ic74fe84f68ffa504006de7e05dc40427bc192fff
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 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. |
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:
***@***.***>
|
I don't love using affinity for node-critical singletons but yeah - to your point we already have them on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with caveats that having a template-able field != recommending usage of it :-)
/retest |
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.