Add node affinity and pod (anti)affinity to Subscription spec.config #250
Conversation
Signed-off-by: perdasilva <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: perdasilva The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
| // Describes node affinity scheduling rules for the pod. | ||
| // +optional | ||
| NodeAffinity *corev1.NodeAffinity `json:"nodeAffinity,omitempty" protobuf:"bytes,1,opt,name=nodeAffinity"` | ||
|
|
||
| // Describes pod affinity scheduling rules (e.g. co-locate this pod in the same node, zone, etc. as some other pod(s)). | ||
| // +optional | ||
| PodAffinity *corev1.PodAffinity `json:"podAffinity,omitempty" protobuf:"bytes,2,opt,name=podAffinity"` | ||
|
|
||
| // Describes pod anti-affinity scheduling rules (e.g. avoid putting this pod in the same node, zone, etc. as some other pod(s)). | ||
| // +optional | ||
| PodAntiAffinity *corev1.PodAntiAffinity `json:"podAntiAffinity,omitempty" protobuf:"bytes,3,opt,name=podAntiAffinity"` |
There was a problem hiding this comment.
Are there tradeoffs between explicitly including these three fields in the spec.config vs including the top level corev1.Affinity (which contains all of these)?
There was a problem hiding this comment.
it would give you more granular control without having to reproduce what's already defined in the deployment pod spec.
For instance, the stock deployment might define all three, but you might only want to change one. If we were to use just affinity, the UX becomes a bit wonky, imo. e.g.
affinity:
nodeAffinity: ...Does this mean, only override nodeAffinity and keep the others stock. Or, does it mean override the whole affinity stanza removing the others?
I thought by splitting them up it would be clearer what is actually being overridden and what is not. wdyt? @joelanford
There was a problem hiding this comment.
That's what I figured you'd say. My only counter argument is: if a new affinity sub-field is added in the future, we will have to add explicit support for it (i.e. more than just vendoring the new API version).
There was a problem hiding this comment.
That's true. But I'd still fall on the UX side rather than DX side. Let me check what the other configs do. If we can get away with saying is e.g. affinity.podAffinity = nil -> don't override affinity.podAffinity. But if they want to nil it out, it would need to be:
affiinity:
nodeAffinity: ..
podAffinity: {}(I'm assuming here the meaning of default podAffinity == nil - though if this changes, it could cause trouble)
There was a problem hiding this comment.
(I'm assuming here the meaning of default podAffinity == nil - though if this changes, it could cause trouble)
Hmmm...probably can't get away from this {} could be different than nil problem though.
There was a problem hiding this comment.
I don't have a strong opinion really. Just wanted to make sure it was considered.
There was a problem hiding this comment.
If you're good with it, then give us a lgtm =D
tbh it's probably not a huge risk either way, once OLM 1.x comes in to action this will go away...
There was a problem hiding this comment.
tbh it's probably not a huge risk either way, once OLM 1.x comes in to action this will go away...
I was still on the fence looking at these changes earlier, and whether this was the right UX going forward. But I'm also fine with this perspective, and we just need a short term bandaid while we continue to iterate on these new OLM 1.x components and behaviors.
/lgtm
/hold
It'll also allow admins to schedule the operator on a specific subset of the supported arches (e.g. if the cluster has s390x and x86 nodes, the operator supports both, but the admin only wants it scheduled on x86) |
Nice! I've updated the description =D (verbatim - I hope you don't mind) |
openshift-ci
bot
added
the
do-not-merge/hold
|
/lgtm ... for what it's worth :) |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
….config (operator-framework#250)" This reverts commit f8da725.
Signed-off-by: perdasilva [email protected]
Adds NodeAffinity, PodAffinity, and PodAntiAffinity to spec.config. This will allow admins to override affinity settings in multi-arch environments where the operator authors have not updated their deployments yet and It'll also allow admins to schedule the operator on a specific subset of the supported arches (e.g. if the cluster has s390x and x86 nodes, the operator supports both, but the admin only wants it scheduled on x86).
The original request was to add NodeAffinity. But I also decided to add Pod (Anti)Affinity settings for cases where Pod(Anti)Affinity is defined by the deployment and due to cluster topology still blocks the pod from being scheduled after the NodeAffinity has been overridden. I don't expect this will be used frequently, but it provides an escape hatch for the admins in for these unlikely edge cases.