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

fix: add node related commands to must-gather #142

Merged
merged 1 commit into from
Apr 7, 2022
Merged

fix: add node related commands to must-gather #142

merged 1 commit into from
Apr 7, 2022

Conversation

agarwal-mudit
Copy link
Contributor

Add node related commands to must-gather output

Signed-off-by: Mudit Agarwal [email protected]

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 4, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: agarwal-mudit

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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 4, 2022
@agarwal-mudit
Copy link
Contributor Author

Its a bit ugly but this is the best I can find as of now, attaching the output file as well.
node_info.txt

leelavg
leelavg reviewed Apr 4, 2022
View reviewed changes
Copy link
Contributor

@leelavg leelavg left a comment

Choose a reason for hiding this comment

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

Its a bit ugly but this is the best I can find as of now,

  • not sure what you mean by this, do we want to conform to any pattern as the collected info seems good enough to me.

@agarwal-mudit
Copy link
Contributor Author

Its a bit ugly but this is the best I can find as of now,

* not sure what you mean by this, do we want to conform to any pattern as the collected info seems good enough to me.

I meant that we have to use oc debug command and because of that I have to put everything in a file and then execute that file as a single command. Couldn't think of any other solution, I need to check with LVM developer also.

@agarwal-mudit agarwal-mudit requested a review from leelavg April 4, 2022 12:58
@leelavg
Copy link
Contributor

leelavg commented Apr 5, 2022

because of that I have to put everything in a file

  • The other way of doing this is a bit involved and so I believe this is good enough, it's like you need to run tail -f /dev/null and bg the process
  • Then you'll have a debug pod running and you could run commands from cli in this pod as any other pod however as part of cleanup, you need to capture the PID of bg process and send a kill signal, pros will be no need to create cmd file

I need to check with LVM developer also.

  • ack, IMHO we could just add -a to lvm lvs so that it'll show metadata & data vols for vols on thinpool (thin vols and snapshots)

we have to use oc debug command

  • one tip would be to use -q with oc debug so that all the debug info will be suppressed

@agarwal-mudit
Copy link
Contributor Author

@leelavg added -a and -q as per the last comment, PTAL

@leelavg
Copy link
Contributor

leelavg commented Apr 5, 2022

  • lgtm, if I approve this'll get merged, I'll await @nbalacha review as well.

nbalacha
nbalacha reviewed Apr 6, 2022
View reviewed changes
must-gather/collection-scripts/gather_node_info Outdated
commands_get=()

# lvm commands
commands_get+=("lvm lvs -a")
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to call lvm each time. For instance, just lvs, vgs etc will work. Why are we including lvm as well? lvm apparently opens the lvm shell.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets check with the LVM dev if this could have any unwanted effects. Otherwise I think this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check with him along with the other things, but as far as I have noticed running only "lvm" opens a lvm shell otherwise not. But yeah, all the commands work without prefixing lvm so I can remove that.

@agarwal-mudit
fix: add node related commands to must-gather
97b216b 
Add node related commands to must-gather output

Signed-off-by: Mudit Agarwal <[email protected]>
@agarwal-mudit agarwal-mudit requested a review from nbalacha April 6, 2022 14:12
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 7, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: agarwal-mudit, nbalacha

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 [agarwal-mudit,nbalacha]

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

@openshift-merge-robot openshift-merge-robot merged commit 6f0a61b into openshift:main Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nbalacha nbalacha nbalacha approved these changes

@leelavg leelavg Awaiting requested review from leelavg

Assignees

@nbalacha nbalacha

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.
4 participants
@agarwal-mudit @leelavg @nbalacha @openshift-merge-robot