ceph: merge toleration for osd/prepareOSD pod if specified both places #8566

subhamkrai · 2021-08-19T11:24:59Z

earlier, ApplyToPodSpec() was only taking one toleration and ignoring
tolerations from placement.ALL().

Description of your changes:
this commit merge toleration for Mgr,Mon,Osd pod
For example, for osd it will merge
spec.placement.all and
storageDeviceClassSets.Placement(in case of pvc) or
spec.placement.osd(in case of non-pvc's).

Signed-off-by: subhamkrai srai@redhat.com

TODO
Test on PVC cluster

when toleration applied to placement.All()
when toleration applied to storageDeviceClassSets.All()

Test on non-PVC cluster

when toleration applied to placement.All()
when toleration applied to spec.placement.osd

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

subhamkrai · 2021-08-19T11:28:10Z

test result on cluster on pvc when toleration applied at spec.Placement.ALL()
cluster-on-pvc.yaml

spec:
  placement:
   all:
      nodeAffinity:
        requiredDuringSchedulingIgnoredDuringExecution:
          nodeSelectorTerms:
          - matchExpressions:
            - key: topology.kubernetes.io/zone
              operator: In
              values:
              - ap-southeast-1a
              - ap-southeast-1b
              - ap-southeast-1c
              - ap-southeast-1d
      tolerations:
      - key: "key1"
        operator: "Equal"
        value: "value1"
        effect: "NoSchedule"

get cephcluster -o yaml

 placement:
      all:
        nodeAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
            nodeSelectorTerms:
            - matchExpressions:
              - key: topology.kubernetes.io/zone
                operator: In
                values:
                - ap-southeast-1a
                - ap-southeast-1b
                - ap-southeast-1c
                - ap-southeast-1d
        tolerations:
        - effect: NoSchedule
          key: key1
          operator: Equal
          value: value1

get pods

rook-ceph-mgr-a-656447c65b-p6lbd                            1/1     Running     0          15m
rook-ceph-mon-a-669b744887-phqh6                            1/1     Running     0          19m
rook-ceph-mon-b-78f498cd68-mqq26                            1/1     Running     0          19m
rook-ceph-mon-c-6fdff8fd4f-9hhd9                            1/1     Running     0          17m
rook-ceph-operator-6768ff8467-7lbkd                         1/1     Running     0          29m
rook-ceph-osd-0-546ff4bb8b-4g9zp                            1/1     Running     0          14m
rook-ceph-osd-1-9c855657b-5st5z                             1/1     Running     0          14m
rook-ceph-osd-2-7874bdbd85-ldqqd                            1/1     Running     0          14m
rook-ceph-osd-prepare-set1-data-0cvh5z-z8mwn                0/1     Completed   0          15m
rook-ceph-osd-prepare-set1-data-1k765m-vmvd4                0/1     Completed   0          15m
rook-ceph-osd-prepare-set1-data-2gt4pb-9dbw7                0/1     Completed   0          15m

subhamkrai · 2021-08-19T11:49:23Z

test result on PVC cluster when tolerations applied at storageClassDeviceSets.placement
cluster-on-pvc.yaml

storage:
    storageClassDeviceSets:
    ---
        placement:
          nodeAffinity:
            requiredDuringSchedulingIgnoredDuringExecution:
            nodeSelectorTerms:
            - matchExpressions:
              - key: topology.kubernetes.io/zone
                operator: In
                values:
                - ap-southeast-1a
                - ap-southeast-1b
                - ap-southeast-1c
                - ap-southeast-1d
          tolerations:
          - key: "key1"
            operator: "Equal"
            value: "value1"
            effect: "NoSchedule"
          ---
        preparePlacement:
          nodeAffinity:
            requiredDuringSchedulingIgnoredDuringExecution:
            nodeSelectorTerms:
            - matchExpressions:
              - key: topology.kubernetes.io/zone
                operator: In
                values:
                - ap-southeast-1a
                - ap-southeast-1b
                - ap-southeast-1c
                - ap-southeast-1d
          tolerations:
          - key: "key1"
            operator: "Equal"
            value: "value1"
            effect: "NoSchedule"

storage:
      ---
        placement:
          nodeAffinity:
            requiredDuringSchedulingIgnoredDuringExecution:
              nodeSelectorTerms:
              - matchExpressions:
                - key: topology.kubernetes.io/zone
                  operator: In
                  values:
                  - ap-southeast-1a
                  - ap-southeast-1b
                  - ap-southeast-1c
                  - ap-southeast-1d
          tolerations:
          - effect: NoSchedule
            key: key1
            operator: Equal
            value: value1
          ---
        preparePlacement:
          nodeAffinity:
            requiredDuringSchedulingIgnoredDuringExecution:
              nodeSelectorTerms:
              - matchExpressions:
                - key: topology.kubernetes.io/zone
                  operator: In
                  values:
                  - ap-southeast-1a
                  - ap-southeast-1b
                  - ap-southeast-1c
                  - ap-southeast-1d
         ---
          tolerations:
          - effect: NoSchedule
            key: key1
            operator: Equal
            value: value1

sp98

nit

pkg/apis/ceph.rook.io/v1/placement.go

travisn

Can you also show the resulting OSD pod spec with a cluster with tolerations on both all and the storageClassDeviceSets.placement?

pkg/apis/ceph.rook.io/v1/placement.go

subhamkrai · 2021-08-19T13:52:00Z

Can you also show the resulting OSD pod spec with a cluster with tolerations on both all and the storageClassDeviceSets.placement?

different tolerations at all and storageClassDeviceSets.placement or same toleration at both places will work?

subhamkrai · 2021-08-19T14:33:09Z

Can you also show the resulting OSD pod spec with a cluster with tolerations on both all and the storageClassDeviceSets.placement?

different tolerations at all and storageClassDeviceSets.placement or same toleration at both places will work?

test result on PVC cluster with toleration at both all and storageDeviceClassSets.placement (applied same toleration at both places and 2 entries of toleration in osd-0 pod sec confirm it is taking toleration from both places)

yaml file

spec:
  placement:
    all:
      nodeAffinity:
        requiredDuringSchedulingIgnoredDuringExecution:
          nodeSelectorTerms:
          - matchExpressions:
            - key: topology.kubernetes.io/zone
              operator: In
              values:
              - ap-southeast-1a
              - ap-southeast-1b
              - ap-southeast-1c
              - ap-southeast-1d
      tolerations:
      - key: "key1"
        operator: "Equal"
        value: "value1"
        effect: "NoSchedule"
  storage:
    storageClassDeviceSets:
        placement:
          tolerations:
          - key: "key1"
            operator: "Equal"
            value: "value1"
            effect: "NoSchedule"

get pod rook-ceph-osd-0-84bf7697c8-c7526 -o yaml

tolerations:
  - effect: NoSchedule
    key: key1
    operator: Equal
    value: value1
  - effect: NoSchedule
    key: key1
    operator: Equal
    value: value1
  - effect: NoExecute
    key: node.kubernetes.io/unreachable
    operator: Exists
    tolerationSeconds: 5
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    operator: Exists
    tolerationSeconds: 300
  topologySpreadConstraints:
  - labelSelector:
      matchExpressions:
      - key: app
        operator: In
        values:
        - rook-ceph-osd

subhamkrai · 2021-08-19T16:06:23Z

I'll test for non PVC cluster tomorrow again with the latest minor changes

travisn · 2021-08-19T16:11:21Z

Can you also show the resulting OSD pod spec with a cluster with tolerations on both all and the storageClassDeviceSets.placement?

different tolerations at all and storageClassDeviceSets.placement or same toleration at both places will work?

It doesn't hurt to have the same toleration twice, but the scenario is that different tolerations would be specified at the two levels. If you test a different toleration it will also confirm that the same toleration isn't just being duplicated.

pkg/apis/ceph.rook.io/v1/placement.go

subhamkrai · 2021-08-19T16:34:32Z

Can you also show the resulting OSD pod spec with a cluster with tolerations on both all and the storageClassDeviceSets.placement?

different tolerations at all and storageClassDeviceSets.placement or same toleration at both places will work?

It doesn't hurt to have the same toleration twice, but the scenario is that different tolerations would be specified at the two levels. If you test a different toleration it will also confirm that the same toleration isn't just being duplicated.

spec:
  placement:
    all:
      nodeAffinity:
        requiredDuringSchedulingIgnoredDuringExecution:
          nodeSelectorTerms:
          - matchExpressions:
            - key: topology.kubernetes.io/zone
              operator: In
              values:
              - ap-southeast-1a
              - ap-southeast-1b
              - ap-southeast-1c
              - ap-southeast-1d
      tolerations:
      - key: "key1"
        operator: "Equal"
        value: "value1"
        effect: "NoSchedule"
  storage:
    storageClassDeviceSets:
        placement:
          tolerations:
          - key: "key2"
            operator: "Equal"
            value: "value1"
            effect: "NoSchedule"

get pod rook-ceph-osd-0-7785bb9478-jxsx7 -o yaml

tolerations:
  - effect: NoSchedule
    key: key2
    operator: Equal
    value: value1
  - effect: NoSchedule
    key: key1
    operator: Equal
    value: value1
  - effect: NoExecute
    key: node.kubernetes.io/unreachable
    operator: Exists
    tolerationSeconds: 5
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    operator: Exists
    tolerationSeconds: 300

this validate the changes, in all it has -key1 and in storageDeviceClassSets.placement key is -key2

pkg/apis/ceph.rook.io/v1/placement.go

subhamkrai · 2021-08-20T09:07:17Z

pkg/apis/ceph.rook.io/v1/placement.go

+	if with.NodeAffinity != nil {
+		ret.NodeAffinity = with.NodeAffinity


still, this two line of code is not perfect or have some issue. It is overriding node affinity when discovering nodes, but I don't think it will cause a problem for now

Why do we need a separate method for MergeOsd()? Seems like merging the tolerations should always be done in the Merge() method so it would apply to all daemons, then no need for this method.

agreed, merging toleration in merge() method will be better, and also it will be applied to other pods like OSD, MGR, MON

subhamkrai · 2021-08-20T09:09:35Z

test result on non-PVC cluster
cluster.yaml

spec:
  placement:
    all:
      tolerations:
      - key: "key1"
        operator: "Equal"
        value: "value1"
        effect: "NoSchedule"
    osd:
      tolerations:
      - key: "key2"
        operator: "Equal"
        value: "value1"
        effect: "NoSchedule"
    prepareosd:
      tolerations:
      - key: "key2"
        operator: "Equal"
        value: "value1"
        effect: "NoSchedule"

get pod rook-ceph-osd-0-7869bb94f4-vfkq9 -o yaml

tolerations:
  - effect: NoSchedule
    key: key2
    operator: Equal
    value: value1
  - effect: NoSchedule
    key: key1
    operator: Equal
    value: value1
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    operator: Exists
    tolerationSeconds: 300
  - effect: NoExecute
    key: node.kubernetes.io/unreachable
    operator: Exists
    tolerationSeconds: 300

travisn · 2021-08-20T15:23:30Z

pkg/apis/ceph.rook.io/v1/placement.go

+	if with.NodeAffinity != nil {
+		ret.NodeAffinity = with.NodeAffinity


Why do we need a separate method for MergeOsd()? Seems like merging the tolerations should always be done in the Merge() method so it would apply to all daemons, then no need for this method.

earlier, `ApplyToPodSpec()` was only taking one toleration and ignoring tolerations from `placement.ALL()`. this commit merge toleration for Mgr,Mon,Osd pod example, for osd it will merge spec.placement.all and storageDeviceClassSets.Placement(in case of pvc) or spec.placement.osd(in case of non-pvc's). Signed-off-by: subhamkrai <srai@redhat.com>

ceph: merge toleration for osd/prepareOSD pod if specified both places (backport #8566)

mergify bot added the ceph main ceph tag label Aug 19, 2021

subhamkrai requested review from travisn and sp98 August 19, 2021 11:28

sp98 requested changes Aug 19, 2021

View reviewed changes

pkg/apis/ceph.rook.io/v1/placement.go Outdated Show resolved Hide resolved

travisn requested changes Aug 19, 2021

View reviewed changes

pkg/apis/ceph.rook.io/v1/placement.go Outdated Show resolved Hide resolved

pkg/apis/ceph.rook.io/v1/placement.go Outdated Show resolved Hide resolved

subhamkrai force-pushed the merge-tolerations branch from 56c3a63 to 638937e Compare August 19, 2021 14:32

travisn reviewed Aug 19, 2021

View reviewed changes

pkg/apis/ceph.rook.io/v1/placement.go Outdated Show resolved Hide resolved

subhamkrai force-pushed the merge-tolerations branch 2 times, most recently from b42e697 to 5a2f4b7 Compare August 20, 2021 08:59

subhamkrai commented Aug 20, 2021

View reviewed changes

pkg/apis/ceph.rook.io/v1/placement.go Outdated Show resolved Hide resolved

subhamkrai commented Aug 20, 2021

View reviewed changes

subhamkrai requested review from sp98, travisn and BlaineEXE August 20, 2021 09:09

travisn requested changes Aug 20, 2021

View reviewed changes

subhamkrai force-pushed the merge-tolerations branch 2 times, most recently from f1909e6 to 61d2e7f Compare August 25, 2021 05:58

subhamkrai force-pushed the merge-tolerations branch from 61d2e7f to e1f232e Compare August 25, 2021 14:04

subhamkrai requested a review from travisn August 25, 2021 14:04

travisn approved these changes Aug 25, 2021

View reviewed changes

sp98 approved these changes Aug 26, 2021

View reviewed changes

travisn merged commit 42717d2 into rook:master Aug 26, 2021

travisn added backport-release-1.6 labels Aug 26, 2021

This was referenced Aug 26, 2021

ceph: merge toleration for osd/prepareOSD pod if specified both places (backport #8566) #8598

Closed

ceph: merge toleration for osd/prepareOSD pod if specified both places (backport #8566) #8599

Merged

subhamkrai deleted the merge-tolerations branch August 26, 2021 15:17

travisn added a commit that referenced this pull request Aug 26, 2021

Merge pull request #8599 from rook/mergify/bp/release-1.7/pr-8566

096eb81

ceph: merge toleration for osd/prepareOSD pod if specified both places (backport #8566)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ceph: merge toleration for osd/prepareOSD pod if specified both places #8566

ceph: merge toleration for osd/prepareOSD pod if specified both places #8566

subhamkrai commented Aug 19, 2021 •

edited

subhamkrai commented Aug 19, 2021 •

edited

subhamkrai commented Aug 19, 2021 •

edited

sp98 left a comment

travisn left a comment

subhamkrai commented Aug 19, 2021

subhamkrai commented Aug 19, 2021

subhamkrai commented Aug 19, 2021

travisn commented Aug 19, 2021

subhamkrai commented Aug 19, 2021

subhamkrai Aug 20, 2021 •

edited

travisn Aug 20, 2021

subhamkrai Aug 25, 2021

subhamkrai commented Aug 20, 2021

travisn Aug 20, 2021

		if with.NodeAffinity != nil {
		ret.NodeAffinity = with.NodeAffinity

ceph: merge toleration for osd/prepareOSD pod if specified both places #8566

ceph: merge toleration for osd/prepareOSD pod if specified both places #8566

Conversation

subhamkrai commented Aug 19, 2021 • edited

subhamkrai commented Aug 19, 2021 • edited

subhamkrai commented Aug 19, 2021 • edited

sp98 left a comment

Choose a reason for hiding this comment

travisn left a comment

Choose a reason for hiding this comment

subhamkrai commented Aug 19, 2021

subhamkrai commented Aug 19, 2021

subhamkrai commented Aug 19, 2021

travisn commented Aug 19, 2021

subhamkrai commented Aug 19, 2021

subhamkrai Aug 20, 2021 • edited

Choose a reason for hiding this comment

travisn Aug 20, 2021

Choose a reason for hiding this comment

subhamkrai Aug 25, 2021

Choose a reason for hiding this comment

subhamkrai commented Aug 20, 2021

travisn Aug 20, 2021

Choose a reason for hiding this comment

subhamkrai commented Aug 19, 2021 •

edited

subhamkrai commented Aug 19, 2021 •

edited

subhamkrai commented Aug 19, 2021 •

edited

subhamkrai Aug 20, 2021 •

edited