Remove duplicate placements for rook-ceph dameons due to specifying with all & specific key

[malayparida2000]

Rook creates the rook-ceph daemon's placement by merging the "all" and specific key placements. The OCS operator specifies the default OCS tolerations and node affinity in both the "all" key and the specific key leading to duplicated placements. To avoid this, skip specifying the default OCS tolerations and node affinity with the specific key.

[malayparida2000]

@travisn @parth-gr After looking at the possible sollutions & how it works I still think the fix has to come on rook side only. As ocs-operator supports specification of placement on the storagecluster CR at spec.placement. Even if we resolve the duplicate specification by ocs-operator. A customer can still end up specifiying same tolerations ones at the "all" key and again with a specific key like "mgr", And we will end up having duplicates. As rook is the last place which creates the pod spec that should be the ideal place to remove the duplicates if any.

[malayparida2000]

Test results-
osd

tolerations:
  - effect: NoSchedule
    key: node.ocs.openshift.io/storage
    operator: Equal
    value: "true"
  - effect: NoExecute
    key: node.kubernetes.io/unreachable
    operator: Exists
    tolerationSeconds: 5
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    operator: Exists
    tolerationSeconds: 300
  - effect: NoSchedule
    key: node.kubernetes.io/memory-pressure
    operator: Exists

    affinity:
    nodeAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
        nodeSelectorTerms:
        - matchExpressions:
          - key: cluster.ocs.openshift.io/openshift-storage
            operator: Exists
          - key: topology.kubernetes.io/zone
            operator: In
            values:
            - us-west-2b

osd-prepare

tolerations:
  - effect: NoSchedule
    key: node.ocs.openshift.io/storage
    operator: Equal
    value: "true"
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    operator: Exists
    tolerationSeconds: 300
  - effect: NoExecute
    key: node.kubernetes.io/unreachable
    operator: Exists
    tolerationSeconds: 300

affinity:
    nodeAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
        nodeSelectorTerms:
        - matchExpressions:
          - key: cluster.ocs.openshift.io/openshift-storage
            operator: Exists

[malayparida2000]

mgr

tolerations:
  - effect: NoSchedule
    key: node.ocs.openshift.io/storage
    operator: Equal
    value: "true"
  - effect: NoExecute
    key: node.kubernetes.io/unreachable
    operator: Exists
    tolerationSeconds: 5
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    operator: Exists
    tolerationSeconds: 300

affinity:
    nodeAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
        nodeSelectorTerms:
        - matchExpressions:
          - key: cluster.ocs.openshift.io/openshift-storage
            operator: Exists
    podAntiAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
      - labelSelector:
          matchLabels:
            app: rook-ceph-mgr
            rook_cluster: openshift-storage
        topologyKey: kubernetes.io/hostname

mon

tolerations:
  - effect: NoSchedule
    key: node.ocs.openshift.io/storage
    operator: Equal
    value: "true"
  - effect: NoExecute
    key: node.kubernetes.io/unreachable
    operator: Exists
    tolerationSeconds: 5
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    operator: Exists
    tolerationSeconds: 300

affinity:
    nodeAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
        nodeSelectorTerms:
        - matchExpressions:
          - key: cluster.ocs.openshift.io/openshift-storage
            operator: Exists
    podAntiAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
      - labelSelector:
          matchExpressions:
          - key: app
            operator: In
            values:
            - rook-ceph-mon
        topologyKey: topology.kubernetes.io/zone
      - labelSelector:
          matchLabels:
            app: rook-ceph-mon
        topologyKey: kubernetes.io/hostname

[malayparida2000]

mds

tolerations:
  - effect: NoSchedule
    key: node.ocs.openshift.io/storage
    operator: Equal
    value: "true"
  - effect: NoExecute
    key: node.kubernetes.io/unreachable
    operator: Exists
    tolerationSeconds: 5
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    operator: Exists
    tolerationSeconds: 300

affinity:
    nodeAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
        nodeSelectorTerms:
        - matchExpressions:
          - key: cluster.ocs.openshift.io/openshift-storage
            operator: Exists
    podAntiAffinity:
      preferredDuringSchedulingIgnoredDuringExecution:
      - podAffinityTerm:
          labelSelector:
            matchExpressions:
            - key: rook_file_system
              operator: In
              values:
              - test-storagecluster-cephfilesystem
          topologyKey: topology.kubernetes.io/zone
        weight: 100

nfs

tolerations:
  - effect: NoSchedule
    key: node.ocs.openshift.io/storage
    operator: Equal
    value: "true"
  - effect: NoExecute
    key: node.kubernetes.io/unreachable
    operator: Exists
    tolerationSeconds: 5
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    operator: Exists
    tolerationSeconds: 300

affinity:
    nodeAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
        nodeSelectorTerms:
        - matchExpressions:
          - key: cluster.ocs.openshift.io/openshift-storage
            operator: Exists
    podAntiAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
      - labelSelector:
          matchExpressions:
          - key: app
            operator: In
            values:
            - rook-ceph-nfs
        topologyKey: kubernetes.io/hostname

[malayparida2000]

exporter

tolerations:
  - effect: NoSchedule
    key: node.kubernetes.io/memory-pressure
    operator: Exists
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    operator: Exists
    tolerationSeconds: 300
  - effect: NoExecute
    key: node.kubernetes.io/unreachable
    operator: Exists
    tolerationSeconds: 5
  - effect: NoSchedule
    key: node.ocs.openshift.io/storage
    operator: Equal
    value: "true"

crashcollector

tolerations:
  - effect: NoSchedule
    key: node.kubernetes.io/memory-pressure
    operator: Exists
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    operator: Exists
    tolerationSeconds: 300
  - effect: NoExecute
    key: node.kubernetes.io/unreachable
    operator: Exists
    tolerationSeconds: 5
  - effect: NoSchedule
    key: node.ocs.openshift.io/storage
    operator: Equal
    value: "true"

[malayparida2000]

@travisn any idea why exporter & crashcollector do not have any node affinities on them? Inspite of node affinities being specified with the 'all' key.

[parth-gr]

@travisn @parth-gr After looking at the possible sollutions & how it works I still think the fix has to come on rook side only. As ocs-operator supports specification of placement on the storagecluster CR at spec.placement. Even if we resolve the duplicate specification by ocs-operator. A customer can still end up specifiying same tolerations ones at the "all" key and again with a specific key like "mgr", And we will end up having duplicates. As rook is the last place which creates the pod spec that should be the ideal place to remove the duplicates if any.

Yes even I saw that we have mechanism already to remove duplicate env variables https://github.com/rook/rook/blob/c33f6bda7bbe5af1ccb56d2573d86fba25244dde/pkg/operator/k8sutil/pod.go#L377

[travisn]

@travisn any idea why exporter & crashcollector do not have any node affinities on them? Inspite of node affinities being specified with the 'all' key.

@malayparida2000 The crash collector and exporter have node selectors to assign them to specific nodes, based on which nodes have other ceph daemons running. Rook basically calculates their node placement instead of relying on the node affinity settings. But those daemons should inherit the tolerations.

[travisn]

@travisn @parth-gr After looking at the possible sollutions & how it works I still think the fix has to come on rook side only. As ocs-operator supports specification of placement on the storagecluster CR at spec.placement. Even if we resolve the duplicate specification by ocs-operator. A customer can still end up specifiying same tolerations ones at the "all" key and again with a specific key like "mgr", And we will end up having duplicates. As rook is the last place which creates the pod spec that should be the ideal place to remove the duplicates if any.

If OCS operator owns fixing its own duplicates, then in case the customer adds the placement that is duplicate, if the alerts bother them, then they should know to remove the duplicates that they caused, right? In any case, everything still works, it's just a recommendation to avoid the duplication and very low impact. Merging complex types automatically could be prone to errors so we want to avoid that complexity.

[malayparida2000]

With this PR as you can see from the taste results I have pasted, there are no longer duplicates on daemons's placement due to ocs-operator.

[openshift-ci]

@parth-gr: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[parth-gr]

@travisn @malayparida2000
I tried appending different duplicate values to osd prepare pod and osd po placement and also duplicate values in all placement,

But the duplicate are only taken by osd prepare pod

PS: I got the reason, the osd and mons, etc are deployments so if we apply duplicate in deployment it will create a pod with only unique once, but the prepare-pod is a job which directly has pod spec, so that's why it still kept the duplicates

[iamniting]

looks good to me,
/hold for @travisn review

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, malayparida2000, parth-gr

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:

• OWNERS [iamniting]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment