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.

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

qa/standalone/scrub/osd-scrub-repair: no -y to diff #18079

Merged
merged 1 commit into from Oct 5, 2017

Conversation

liewegas
Copy link
Member

@liewegas liewegas commented Oct 2, 2017

With -y you can't see the entire line when it is long, which is
needed to identify the diff failure in
http://tracker.ceph.com/issues/21618

Signed-off-by: Sage Weil sage@redhat.com

@yuriw
Copy link
Contributor

yuriw commented Oct 2, 2017

@joscollin
Copy link
Member

joscollin commented Oct 3, 2017

The testing Label wip-jcollin-testing was added by the ptl-tool during its testing. Please ignore it. I will create some test PRs for testing ptl-tool. Thanks.

Copy link
Contributor

@dzafman dzafman left a comment

Choose a reason for hiding this comment

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

If you run the test in a wide terminal you can see more of the line. I suggest that we change the value of DIFFCOLOPTS to reflect how we want to diff to be generated. It is possible that under circumstances like a terminal width below a certain value we don't put the -y in. I'd prefer we don't remove ${DIFFCOLOPTS} as the way to change this.

@dzafman
Copy link
Contributor

dzafman commented Oct 3, 2017

@liewegas The value of DIFFCOLOPTS is set in ceph-helpers.sh. I started using -y because it was hard to determine where the differences were since the line numbers don't mean anything with the inline JSON. We could replace the -y with much more context (-C) or as I previously suggested suppress the -y with narrow terminal widths.

We could also allow an environment variable if set to replace the DIFFCOLOPTS.

@liewegas
Copy link
Member Author

liewegas commented Oct 3, 2017

In this case there's no terminal.. it was run via teuthology, and there wasn't enough info to tell what was different. How about leaving the env var in there, but don't define it in the file, so when running it interactively you can add -y (or whatever)?

@wjwithagen
Copy link
Contributor

@liewegas @dzafman
I created DIFFCOLOPTS to work around the fact that diff on FreeBSD does not have these.
Using the value from an ENV setting would make most use of the fact that it is only really usefull when run on a terminal. Could perhaps even test if we are running on a terminal in the script.

@dzafman
Copy link
Contributor

dzafman commented Oct 3, 2017

3 suggestions.

  1. Don't set $DIFFCOLOPTS in ceph-helpers.sh must be set in environment

DIFFCOLOPTS="-y -W 400" ../qa/run-standalone.sh osd-scrub-repair.sh

  1. ceph-helpers.sh doesn't set DIFFCOLOPTS if there isn't a terminal involved. We already make sure there is a non-zero terminal width before adding the -W option.

I assume the teuthology run had a diff -y without a -W showing that there was no terminal.

  1. Instead of calling diff directly create new function do_diff() that runs diff without $DIFFCOLOPTS and if there is a difference also outputs a diff with $DIFFCOLOPTS if it isn't empty.

I like this one since we can see the complete lines and when -y is available where those differences are.

@yuriw
Copy link
Contributor

yuriw commented Oct 3, 2017

@liewegas @liewegas this PR passed thru rados tests, ready to merge

With -y you can't see the entire line when it is long, which is
needed to identify the diff failure in
http://tracker.ceph.com/issues/21618

Instead, let the interactive user specify the option if they want it.

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
Copy link
Member Author

liewegas commented Oct 3, 2017

Opting for (1) for simplicity!

@dzafman dzafman self-assigned this Oct 3, 2017
@dzafman
Copy link
Contributor

dzafman commented Oct 3, 2017

@liewegas Maybe I'll make the changes later if osd-scrub-repair.sh and/or osd-scrub-snaps.sh tests are updated.

@liewegas liewegas merged commit f26c3c7 into ceph:master Oct 5, 2017
@liewegas liewegas deleted the wip-21618 branch October 5, 2017 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants