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

collect rbd image and snap info per rados namespace #150

Merged
merged 1 commit into from
May 28, 2024
Merged

collect rbd image and snap info per rados namespace #150

merged 1 commit into from
May 28, 2024

Conversation

yati1998
Copy link
Member

This commit collects rbd image and snap info for each rados namespace

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 24, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@yati1998 yati1998 marked this pull request as ready for review April 25, 2024 05:36
Madhu-1
Madhu-1 suggested changes Apr 25, 2024
View reviewed changes
Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

  • Need to adjust omap code to collect omap in each radosnamespace for both pvc and snapshot
  • trash ls need to be collected for radosnamespace

Okay, we need to collect all the details we are collecting for each rbd pool. we need to like below

  • Move all the collection code to helper functions
  • Collect all details for the pool and also collect all details for each namespace inside the pool.

collection-scripts/gather_ceph_resources Show resolved Hide resolved
collection-scripts/gather_ceph_resources Show resolved Hide resolved
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 25, 2024

@Madhu-1: changing LGTM is restricted to collaborators

In response to this:

  • Need to adjust omap code to collect omap in each radosnamespace for both pvc and snapshot
  • trash ls need to be collected for radosnamespace

Okay, we need to collect all the details we are collecting for each rbd pool. we need to like below

  • Move all the collection code to helper functions
  • Collect all details for the pool and also collect all details for each namespace inside the pool.

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/test-infra repository.

jarrpa
jarrpa requested changes Apr 25, 2024
View reviewed changes
collection-scripts/gather_ceph_resources Outdated Show resolved Hide resolved
@jarrpa
Copy link
Member

jarrpa commented Apr 25, 2024 

* Move all the collection code to helper functions

This is not viable here as the namespaced commands need a flag that must be omitted from commands against the base pool. It'd be nice, but I don't think it's worth splitting it off into a helper function and then having to do conditionals based on inputs.

@yati1998 yati1998 requested review from jarrpa and Madhu-1 April 29, 2024 05:57
@yati1998
Copy link
Member Author

yati1998 commented May 6, 2024

@Madhu-1 @jarrpa please do have a look, if these details are all we need.

@yati1998 yati1998 requested review from black-dragon74 and removed request for Madhu-1 May 13, 2024 12:10
@yati1998
Copy link
Member Author

@jarrpa @Madhu-1 I have addressed the comments to add helper functions and some more of cleanup. Please do review the PR. Also I have tested the changes and could collect all the required resources both in case of radosnamespace as well as in it's absence

@yati1998 yati1998 requested a review from Madhu-1 May 23, 2024 07:07
@yati1998 yati1998 force-pushed the rados branch 2 times, most recently from e960174 to fa5f924 Compare May 28, 2024 06:17
Madhu-1
Madhu-1 reviewed May 28, 2024
View reviewed changes
Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

the suggested changes LGTM but as am not an expert in this repo not able to confirm its covering all the required items.

@yati1998
Copy link
Member Author

the suggested changes LGTM but as am not an expert in this repo not able to confirm its covering all the required items.

Thanks @Madhu-1 for your review, it collects all the info that is collected for other images. In case you guys find anything missing further while debugging , feel free to open a bug/ issue, will add it.

@yati1998
Copy link
Member Author

@black-dragon74 can you please review the PR

@yati1998 yati1998 force-pushed the rados branch 3 times, most recently from 0bcc5ba to 9f13695 Compare May 28, 2024 08:20
black-dragon74
black-dragon74 suggested changes May 28, 2024
View reviewed changes
Copy link
Collaborator

@black-dragon74 black-dragon74 left a comment

Choose a reason for hiding this comment

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

Please fix the indentation for collect_info function at line: 205

We can get rid of unnecessary braces surrounding commands at lines mentioned below as they serve no purpose.

  • 249
  • 275
  • 287
  • 322
  • 324
  • 334
  • 336

Regards

@black-dragon74
Copy link
Collaborator

Also, please mark the open conversations are resolved?

@yati1998
collect rbd image and snap info per rados namespace
55762c4 
collect rbd image and snap info as per
the rados namespace

Signed-off-by: yati1998 <ypadia@redhat.com>
jarrpa
jarrpa reviewed May 28, 2024
View reviewed changes
collection-scripts/gather_ceph_resources
@@ -14,6 +14,7 @@ ceph_resources+=(cephblockpools)
ceph_resources+=(cephfilesystems)
ceph_resources+=(cephclient)
ceph_resources+=(cephfilesystemsubvolumegroups)
ceph_resources+=(cephblockpoolradosnamespaces.ceph.rook.io)
Copy link
Member

Choose a reason for hiding this comment

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

Good to see this up here. Is there a reason why this has the complete CRD name .ceph.rook.io where the others don't? If not, chop it down.

@jarrpa
Copy link
Member

jarrpa commented May 28, 2024

Please fix the indentation for collect_info function at line: 205

The indentation looks fine to me... what are you seeing?

We can get rid of unnecessary braces surrounding commands at lines mentioned below as they serve no purpose.

* 249
* 275
* 287
* 322
* 324
* 334
* 336

This seems like a superfluous cleanup that doesn't need to be part of this PR. I'd say it's best to leave them as-is and remove them in a follow-up PR as desired.

@jarrpa
Copy link
Member

jarrpa commented May 28, 2024

/retest

@black-dragon74
Copy link
Collaborator

This seems like a superfluous cleanup that doesn't need to be part of this PR. I'd say it's best to leave them as-is and remove them in a follow-up PR as desired.

That is exactly what it is. Completely fine with taking it in a separate PR :)

Regards

black-dragon74
black-dragon74 approved these changes May 28, 2024
View reviewed changes
Copy link
Collaborator

@black-dragon74 black-dragon74 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 label May 28, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 28, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: black-dragon74, yati1998

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 [black-dragon74,yati1998]

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 96e89b3 into red-hat-storage:main May 28, 2024
5 checks passed
@yati1998
Copy link
Member Author

/cherry-pick release-4.15

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request May 29, 2024
Closed
@openshift-cherrypick-robot
Copy link

@yati1998: new pull request created: #168

In response to this:

/cherry-pick release-4.15

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.

@yati1998
Copy link
Member Author

/cherry-pick release-4.16

@openshift-cherrypick-robot
Copy link

@yati1998: new pull request created: #169

In response to this:

/cherry-pick release-4.16

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.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request May 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jarrpa jarrpa jarrpa requested changes

@Madhu-1 Madhu-1 Madhu-1 requested changes

@black-dragon74 black-dragon74 black-dragon74 approved these changes

Assignees

@jarrpa jarrpa

@black-dragon74 black-dragon74

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yati1998 @jarrpa @black-dragon74 @openshift-cherrypick-robot @Madhu-1