Skip to content

Must-gather: add bg process for ceph cmds#1048

Merged
openshift-merge-robot merged 2 commits intored-hat-storage:masterfrom
crombus:mg_ceph_fast
Feb 16, 2021
Merged

Must-gather: add bg process for ceph cmds#1048
openshift-merge-robot merged 2 commits intored-hat-storage:masterfrom
crombus:mg_ceph_fast

Conversation

@crombus
Copy link
Contributor

@crombus crombus commented Feb 9, 2021

add bg process for ceph cmds. create
temp log file for ceph bg cmds and
cat them to gather-debug.log file.

Signed-off-by: crombus pkundra@redhat.com

Copy link
Member

@jarrpa jarrpa left a comment

Choose a reason for hiding this comment

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

In general this makes sense, but there's still a problem with the logging. By outputting the stdout of multiple background processes to the same log file there is no guarantee that the output lines will be coherent since multiple jobs could output lines between each other. You should output each command's stdout to separate files, then later aggregate them into the main debug log. If they're all in the same directory you don't need to worry about the exact file names, just loop over all files in the directory.

Copy link

@rajatcing rajatcing left a comment

Choose a reason for hiding this comment

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

You can remove the --no-header line since I already caught it in my PR #1044 and you have also missed a couple of processes to be moved in background.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 10, 2021

@crombus: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/red-hat-storage-ocs-ci-e2e-aws 4a777df link /test red-hat-storage-ocs-ci-e2e-aws

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@crombus crombus changed the title Must-gather: add bg process for ceph cmds [WIP]Must-gather: add bg process for ceph cmds Feb 10, 2021
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 10, 2021
@crombus crombus changed the title [WIP]Must-gather: add bg process for ceph cmds Must-gather: add bg process for ceph cmds Feb 11, 2021
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2021
@crombus crombus requested review from jarrpa and rajatcing February 11, 2021 15:06
@crombus
Copy link
Contributor Author

crombus commented Feb 11, 2021

@jarrpa I didn’t cat the output of logs in gather-debug.log. Reason they are still up and down it ls not in order. I suggest to have it in the log folder unless you have some other suggestions.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2021
Copy link
Member

@jarrpa jarrpa left a comment

Choose a reason for hiding this comment

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

I think it's totally okay if they're not in the same file. However, given that, it may just be easier to have both stdout and stderr output to the same file regardless. I can imagine someone looking through must-gather would find it convenient if they just had one file to determine the output (successful or failure) of a single command. Thoughts?

Also, merge the first and last commits.

ceph_collection(){
COMMAND_OUTPUT_DIR=${CEPH_COLLECTION_PATH}/must_gather_commands
COMMAND_JSON_OUTPUT_DIR=${CEPH_COLLECTION_PATH}/must_gather_commands_json_output
COMMAND_LOG_OUTPUT_DIR=${CEPH_COLLECTION_PATH}/logs
Copy link
Member

Choose a reason for hiding this comment

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

I think it might make sense to name this COMMAND_ERR_OUTPUT_DIR? The main purpose of this is to collect the output to fd 2, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. I'll update it

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 16, 2021
@crombus
Copy link
Contributor Author

crombus commented Feb 16, 2021

I think it's totally okay if they're not in the same file. However, given that, it may just be easier to have both stdout and stderr output to the same file regardless. I can imagine someone looking through must-gather would find it convenient if they just had one file to determine the output (successful or failure) of a single command. Thoughts?

As of now if the cmd has failed then the file will be empty and will be removed at end by MG.

For end user we are providing just the data which was collected. If at any point the data is not collected or cmd failed to find the reason we look in gather-debug.log.

I understand your point but in that case to list which cmds fail and which passed will be difficult as they will not be in one location. We have to check each file to find that it is failed or passed.

Also, merge the first and last commits.

add bg process for ceph cmds. create
temp log file for ceph bg cmds and
cat them to gather-debug.log file.
create err log file for each bg ceph
 cmd.

Signed-off-by: crombus <pkundra@redhat.com>
Signed-off-by: crombus <pkundra@redhat.com>
(cherry picked from commit 1820d61)
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 16, 2021
Copy link
Member

@jarrpa jarrpa 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-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jarrpa, rajatsing

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

The pull request process is described 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

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2021
@jarrpa
Copy link
Member

jarrpa commented Feb 16, 2021

For end user we are providing just the data which was collected. If at any point the data is not collected or cmd failed to find the reason we look in gather-debug.log.

I understand your point but in that case to list which cmds fail and which passed will be difficult as they will not be in one location. We have to check each file to find that it is failed or passed.

A solid argument, thanks!

@openshift-merge-robot openshift-merge-robot merged commit fa1659d into red-hat-storage:master Feb 16, 2021
@crombus
Copy link
Contributor Author

crombus commented Feb 17, 2021

/cherry-pick release-4.7

@openshift-cherrypick-robot

@crombus: new pull request created: #1082

Details

In response to this:

/cherry-pick release-4.7

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.

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

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

Development

Successfully merging this pull request may close these issues.

6 participants