Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add upgrade reliability params in storagecluster CR #2613

Merged
merged 3 commits into from
May 22, 2024
Merged

Add upgrade reliability params in storagecluster CR #2613

merged 3 commits into from
May 22, 2024

Conversation

Nikhil-Ladha
Copy link
Member

Add upgrade reliability parameters to storagecluster CR
and overriding the default values in cephCluster CR if the
values are defined in the storagecluster CR

JIRA: https://issues.redhat.com/browse/RHSTOR-5734

iamniting
iamniting approved these changes May 15, 2024
View reviewed changes
Copy link
Member

@iamniting iamniting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to make any checks in the ceph cluster status? Otherwise changes looks good to me.

/hold for Umanga reviews.

@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 May 15, 2024
@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 15, 2024
@Nikhil-Ladha
Copy link
Member Author

Don't we need to make any checks in the ceph cluster status? Otherwise changes looks good to me.

From what I understand, no we don't need to make any status changes as such. If at all needed, rook will be taking care of it. It reads the cephCluster CR and does other related operations based on that and will update the Status where and all needed.
@travisn please correct me if I am wrong.

travisn
travisn reviewed May 15, 2024
View reviewed changes
Copy link
Contributor

@travisn travisn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to make any checks in the ceph cluster status? Otherwise changes looks good to me.

From what I understand, no we don't need to make any status changes as such. If at all needed, rook will be taking care of it. It reads the cephCluster CR and does other related operations based on that and will update the Status where and all needed. @travisn please correct me if I am wrong.

Agreed, no status checks needed here, let's keep it simple and just pass through to Rook

controllers/storagecluster/cephcluster.go Outdated Show resolved Hide resolved
@Nikhil-Ladha Nikhil-Ladha force-pushed the add-upgrade-reliability-params branch from 15c58e8 to 898443a Compare May 17, 2024 06:39
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 17, 2024
@Nikhil-Ladha Nikhil-Ladha force-pushed the add-upgrade-reliability-params branch 2 times, most recently from d539948 to 2f66fe3 Compare May 17, 2024 06:57
@Nikhil-Ladha
Copy link
Member Author

/retest

@Nikhil-Ladha Nikhil-Ladha requested a review from travisn May 17, 2024 08:39
@Nikhil-Ladha
Copy link
Member Author

/retest

travisn
travisn reviewed May 17, 2024
View reviewed changes
Copy link
Contributor

@travisn travisn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a tiny suggestion!

controllers/storagecluster/cephcluster.go Outdated Show resolved Hide resolved
@Nikhil-Ladha Nikhil-Ladha force-pushed the add-upgrade-reliability-params branch 3 times, most recently from 6205319 to 8e4e8c4 Compare May 20, 2024 08:34
travisn
travisn reviewed May 20, 2024
View reviewed changes
Copy link
Contributor

@travisn travisn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 20, 2024
@Nikhil-Ladha
Update rook api and image version
fd723b3 
Update rook api and image to latest versions

Signed-off-by: Nikhil-Ladha <nikhilladha1999@gmail.com>
@Nikhil-Ladha Nikhil-Ladha force-pushed the add-upgrade-reliability-params branch from 8e4e8c4 to 2e292f1 Compare May 20, 2024 17:10
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 20, 2024
@Nikhil-Ladha
Add upgrade reliability parameters to storagecluster CR
b8355b3 
Add upgrade reliability parameters to storagecluster CR
and overriding the default values in cephCluster CR if the
values are defined in the storagecluster CR

Signed-off-by: Nikhil-Ladha <nikhilladha1999@gmail.com>
@Nikhil-Ladha
Add generated changes
e791497 
Add generated csv, crd changes

Signed-off-by: Nikhil-Ladha <nikhilladha1999@gmail.com>
@Nikhil-Ladha Nikhil-Ladha force-pushed the add-upgrade-reliability-params branch from 2e292f1 to e791497 Compare May 21, 2024 06:27
Nikhil-Ladha
Nikhil-Ladha commented May 21, 2024
View reviewed changes
controllers/defaults/defaults.go
Comment on lines -57 to -59
// DefaultWaitTimeoutForHealthyOSD is the default time for which the operator would wait before an OSD can be stopped
// for an upgrade or restart
DefaultWaitTimeoutForHealthyOSD = 10 * time.Minute
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had these in previous release but I guess that should be fine, it's not breaking anything.

@Nikhil-Ladha
Copy link
Member Author

/retest

travisn
travisn reviewed May 21, 2024
View reviewed changes
Copy link
Contributor

@travisn travisn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 21, 2024
@Nikhil-Ladha
Copy link
Member Author

@iamniting are we good to merge?
Or, still waiting for Umanga's review?

iamniting
iamniting approved these changes May 22, 2024
View reviewed changes
Copy link
Member

@iamniting iamniting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 22, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, Nikhil-Ladha, travisn

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 39c9b95 into red-hat-storage:main May 22, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@travisn travisn travisn left review comments

@iamniting iamniting iamniting approved these changes

@umangachapagain umangachapagain Awaiting requested review from umangachapagain

Assignees

@travisn travisn

@iamniting iamniting

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Nikhil-Ladha @travisn @iamniting