Skip to content

Fix pod toleration issues#3672

Open
aruniiird wants to merge 1 commit intored-hat-storage:mainfrom
aruniiird:fix-missing-tolerations-dfbugs-5253
Open

Fix pod toleration issues#3672
aruniiird wants to merge 1 commit intored-hat-storage:mainfrom
aruniiird:fix-missing-tolerations-dfbugs-5253

Conversation

@aruniiird
Copy link
Contributor

@aruniiird aruniiird commented Jan 27, 2026

Problem:
The function 'mergePlacements()' used to replace the current/default placement with the user provided placement spec. This replaced any default needed placement->tolerations which were added by the operator by default.

Solution:
We need to merge the new/custom placements additively. This way, any required placement values, provided by the operator, won't be replaced and any new/additional values, provided by the user, will be appended to the existing ones.

For more details: https://issues.redhat.com/browse/DFBUGS-5253

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 27, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aruniiird
Once this PR has been reviewed and has the lgtm label, please assign travisn for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

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

@aruniiird
Copy link
Contributor Author

@malayparida2000 ,
While re-checking whether the change fixes all the placement+toleration related issue, noticed that CephNonResilientPools OSDs bypass the common getPlacemen() function and we manually construct the placement configurations. This means, the fix here won't be applied to the CephNonResilientPools.

Code referred: https://github.com/red-hat-storage/ocs-operator/blob/main/controllers/storagecluster/cephcluster.go#L945-L976
Made by the PR: #1782
Bug / Issue: https://issues.redhat.com/browse/DFBUGS-5253

Can you please check? Once confirmed, I can add / extend the changes to the above component as well? If current code is required and intentional we can ignore this comment.
Thanks

@malayparida2000
Copy link
Contributor

/hold

the All key should only be utilised & always has been utlised by rook only. We do not apply the tolerations or affinity under all to other components

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 27, 2026
Problem:
The function 'mergePlacements()' used to replace the current/default
placement with the user provided placement spec. This replaced any
default needed placement->tolerations which were added by the
operator by default.

Solution:
We need to merge the new/custom placements additively.
This way, any required placement values, provided by the
operator, won't be replaced and any new/additional values,
provided by the user, will be appended to the existing ones.

Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>
@aruniiird aruniiird force-pushed the fix-missing-tolerations-dfbugs-5253 branch from b9f8afe to faf6192 Compare February 5, 2026 09:38
@aruniiird
Copy link
Contributor Author

aruniiird commented Feb 5, 2026

@malayparida2000 , yes I agree to your above point, but we still have a bug.

When a custom placement is added (to the StorageCluster spec), our current behavior is to replace the existing (default + required + operator provided placement tolerations) with the new-custom-provided one.
The correct behavior should be to merge additively (the fix provided here), without replacing the existing.
PS: this is the same pattern which rook-operator also follows.

I've removed the previous code to add 'all' placement to all the components.
If user want to put additional placement props, then they have to specify it under StorageCluster -> Spec -> Placement map with the specific component name (like rbd-mirror , metrics-exporter , api-server etc) separately.

Please take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants