operator/dns: Add DNS pod placement API #870
Conversation
Miciah
force-pushed
the
NE-546-operator-slash-dns-add-DNS-pod-placement-API
branch
from
81b52c4 to
1c71dba
Compare
operator/v1/types_dns.go
Outdated
| type DNSNodePlacement struct { | ||
| // nodeSelector is the node selector applied to DNS pods. | ||
| // | ||
| // If unset, the default is the following: |
There was a problem hiding this comment.
indicate that this default may change over time.
There was a problem hiding this comment.
I don't foresee its changing, but I'll add a comment to leave us the option to do so.
| // +optional | ||
| Servers []Server `json:"servers,omitempty"` | ||
|
|
||
| // nodePlacement provides explicit control over the scheduling of DNS |
There was a problem hiding this comment.
can you provide examples of why this may be needed and the tradeoffs such a change will bring?
Off the top of my head, this has the potential to negatively impact availability by not having a local DNS authority on each node. I could see a reason for doing this as some kind of corporate standard that wishes to restrict where DNS pods may land in a datacenter.
There was a problem hiding this comment.
Yes, the top of your head is right. Not having a local pod has its drawbacks, but some users really don't want DNS on certain nodes, for security or compliance reasons. I'll add a comment to that effect.
operator/v1/types_dns.go
Outdated
| // If set, the specified selector is used and replaces the default. | ||
| // | ||
| // +optional | ||
| NodeSelector *metav1.LabelSelector `json:"nodeSelector,omitempty"` |
There was a problem hiding this comment.
pretty sure that pods are still restricted by NodeSelector map[string]string . I suggest having this match that schema even though it is an older style selector.
There was a problem hiding this comment.
We use metav1.LabelSelector for the IngressController API, so that puts us in a slightly icky place:
api/operator/v1/types_ingress.go
Lines 195 to 206 in 517b8a7
But I guess it's better to do the right thing here than to be consistent with a bad API, right?
There was a problem hiding this comment.
But I guess it's better to do the right thing here than to be consistent with a bad API, right?
I think so. I would like to avoid the ability to have incompatible data if possible. I should have caught it in that operator too.
Miciah
force-pushed
the
NE-546-operator-slash-dns-add-DNS-pod-placement-API
branch
from
1c71dba to
c69b45a
Compare
This commit implements NE-546. https://issues.redhat.com/browse/NE-546 * operator/v1/types_dns.go (DNSSpec): Add NodePlacement field with type DNSNodePlacement. (DNSNodePlacement): New type. Specify optional node selector and tolerations for DNS pods. * operator/v1/0000_70_dns-operator_00-custom-resource-definition.yaml: * operator/v1/zz_generated.deepcopy.go: * operator/v1/zz_generated.swagger_doc_generated.go: Regenerate.
Miciah
force-pushed
the
NE-546-operator-slash-dns-add-DNS-pod-placement-API
branch
from
c69b45a to
4a0c62d
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, Miciah 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-robot
added
the
approved
This PR implements NE-546.
operator/v1/types_dns.go(DNSSpec): AddNodePlacementfield with typeDNSNodePlacement.(
DNSNodePlacement): New type. Specify optional node selector and tolerations for DNS pods.operator/v1/0000_70_dns-operator_00-custom-resource-definition.yaml:operator/v1/zz_generated.deepcopy.go:operator/v1/zz_generated.swagger_doc_generated.go: Regenerate.