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

osd: Improve size scrub error handling and ignore system attrs in xattr checking #16407

Merged
merged 8 commits into from Aug 18, 2017

Conversation

Projects
None yet
2 participants
@dzafman
Copy link
Member

commented Jul 19, 2017

Scrub list-inconsistent-obj output improvements

  • Add primary info
  • Add new local object size inconsistent with local object info
  • Add output of ss_attr_missing and ss_attr_corrupt errors so not just in snapset results

Fixes: http://tracker.ceph.com/issues/20243
Fixes: http://tracker.ceph.com/issues/18836

@dzafman

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2017

5 fails: dzafman-2017-07-18_21:53:08-rados-wip-zafman-testing-distro-basic-smithi
1418366 Error ENOENT: osd.3 does not exist. create it before updating the crush map
1418387 Segmentation violation in cephx_verify_authorizer()
1418434 status 124: 'sudo adjust-ulimits ceph-coverage /home/ubuntu/cephtest/archive/coverage timeout 120 ceph --cluster ceph quorum_status'
1418448 Error compiling test crush map; probably need newer crushtool
1418521 src/osd/OSD.cc: 4701: FAILED assert(curmap) in handle_osd_ping()

@dzafman dzafman force-pushed the dzafman:wip-20243 branch from 894d91f to 8fb69d1 Jul 27, 2017

@dzafman dzafman added the needs-test label Jul 27, 2017

@dzafman

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2017

The osd-scrub-repair.sh test needs to be running in teuthology for this to actually be tested. It passed on my build machine before rebasing to current master.

@dzafman dzafman requested a review from tchaikov Jul 27, 2017

@tchaikov tchaikov self-assigned this Jul 31, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2017

sorry @dzafman , i missed this PR, will review it early tomorrow morning.

@dzafman dzafman removed the needs-test label Aug 1, 2017

@dzafman

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2017

Once #16709 merges this change will easily be tested with the following:

../qa/run-standalone.sh osd-scrub-repair.sh

}
if (first_bl.length() == 0) {
first_bl.append(bl);
} else if (!object_error.has_object_info_inconsistency() && !(bl == first_bl)) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Aug 1, 2017

Contributor

nit, could take this chance to use

!bl.content_equal(first_bl)

instead of

!(bl == first_bl)

for better performance. the later compares the bufferlist byte-wise.

This comment has been minimized.

Copy link
@tchaikov

tchaikov Aug 1, 2017

Contributor

and, i wonder if it's an optimal solution to usually use the first oi bufferlist as the auth copy? even if it's not as "good" as the other copies per the criteria of

oi.version > auth_version ||
(oi.version == auth_version && dcount(oi) > dcount(*auth_oi)))

This comment has been minimized.

Copy link
@dzafman

dzafman Aug 1, 2017

Author Member

@tchaikov This test is hopefully selecting the object info with the highest version and the most digests available. Amongst all the best object infos the first would be selected. The only improvement I can think of would be to favor the primary if all else is equal.

This comment has been minimized.

Copy link
@tchaikov

tchaikov Aug 1, 2017

Contributor

This test is hopefully selecting the object info with the highest version and the most digests available.

@dzafman i knew this. that's why i raised this concern. because the first one is not necessary the one with the highest version and the one with the most digests available.

This comment has been minimized.

Copy link
@dzafman

dzafman Aug 1, 2017

Author Member

@tchaikov This test gets the first one using auth_version == eversion_t() then compares the each subsequent version against the best version so far using the snippet you showed.

@@ -274,7 +274,7 @@ string cleanbin(bufferlist &bl, bool &base64)
{
bufferlist::iterator it;
for (it = bl.begin(); it != bl.end(); ++it) {
if (iscntrl(*it))
if (iscntrl(*it) && !isspace(*it))

This comment has been minimized.

Copy link
@tchaikov

tchaikov Aug 1, 2017

Contributor

but a newline or a {vertical|horizontal} tab won't look right in cout, they will just mess up the output. and probably will also annoy the xml/json parser that the user will be using.

This comment has been minimized.

Copy link
@dzafman

dzafman Aug 1, 2017

Author Member

hmm...I'll probably pull this change. I did it because I was annoyed by the fact that I created a an attr with the data from /etc/fstab, but couldn't see the text because the newlines triggered a base64 encoding.

This comment has been minimized.

Copy link
@tchaikov

tchaikov Aug 1, 2017

Contributor

@dzafman i see. and i am re-reading how the cleanbin() is being used in cot. seems we are not using it to print out the json/xml elements. so i withdraw this comment. it should be fine. =)

This comment has been minimized.

Copy link
@dzafman

dzafman Aug 1, 2017

Author Member

No, you were right I use it in dump_shard() to determine whether to use Base64 encoding or not.

if (i->second.read_error || i->second.ec_hash_mismatch || i->second.ec_size_mismatch)
// Don't use this particular shard due to previous errors
// XXX: For now we can't pick one shard for repair and another's object info or snapset
if (skip)

This comment has been minimized.

Copy link
@tchaikov

tchaikov Aug 1, 2017

Contributor

is it equivalent with

if (shard_info.errors & ~shard_info.OBJECT_INFO_INCONSISTENCY)
  goto out;

?
if yes, can we use the above condition instead? because, i think it puts this criteria more explicitly.

This comment has been minimized.

Copy link
@dzafman

dzafman Aug 1, 2017

Author Member

Actually, I can just test shard_info.errors because OBJECT_INFO_INCONSISTENCY is an object_error not a shard error. So a shard with any errors can not be the auth copy.

@tchaikov tchaikov changed the title Wip 20243 osd: Improve size scrub error handling and ignore system attrs in xattr checking Aug 1, 2017

@tchaikov tchaikov removed their assignment Aug 1, 2017

@dzafman dzafman force-pushed the dzafman:wip-20243 branch from 4995789 to 73b0674 Aug 1, 2017

@dzafman

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2017

@tchaikov I made code review suggested changes.

dzafman added some commits Jun 29, 2017

osd: Fix test op error message
Signed-off-by: David Zafman <dzafman@redhat.com>
test: Add undocumented corrupt-size for testing
Signed-off-by: David Zafman <dzafman@redhat.com>
osd: Add whether shard is primary in list-inconsistent-obj
Add new field in the client interface
Update test case

Fixes: http://tracker.ceph.com/issues/18836

Signed-off-by: David Zafman <dzafman@redhat.com>
osd: Change a check to an assert() since it can't happen anymore
Signed-off-by: David Zafman <dzafman@redhat.com>
osd: Compare all object info even when can't consider for auth copy
Signed-off-by: David Zafman <dzafman@redhat.com>
osd, rados: Improve size scrub error handling
Fixes: http://tracker.ceph.com/issues/20243

Signed-off-by: David Zafman <dzafman@redhat.com>
osd, rados: Adding ss_attr_missing and ss_attr_corrupt errors to list…
…-inconsistent-obj

Signed-off-by: David Zafman <dzafman@redhat.com>
osd: In scrub's be_select_auth_object() detect multiple errors better
Signed-off-by: David Zafman <dzafman@redhat.com>

@dzafman dzafman force-pushed the dzafman:wip-20243 branch from 73b0674 to 75b4256 Aug 12, 2017

@dzafman

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2017

Passes osd-scrub-repair.sh:
../qa/run-standalone.sh osd-scrub-repair.sh

This passed a subset of rados suite (5 unrelated failures):
http://pulpito.ceph.com/dzafman-2017-08-11_20:13:59-rados-wip-20243-distro-basic-smithi

@@ -866,8 +866,6 @@ map<pg_shard_t, ScrubMap *>::const_iterator
auth = j;
*auth_oi = oi;
auth_version = oi.version;
auth_bl.clear();

This comment has been minimized.

Copy link
@tchaikov

tchaikov Aug 16, 2017

Contributor

@dzafman but David, the existing logic reset auth_bl with the current bl, as long as it's "better" by the criteria above. while this PR always stick with the first one. i know, it's more consistent this way, because instead of using the "best" bl so far which might change when we find a better one, we are always comparing the bl with the first non-empty OI attr bl. but again, the first non-empty is not necessarily the best OI attr bl.

This comment has been minimized.

Copy link
@dzafman

dzafman Aug 17, 2017

Author Member

@tchaikov The original auth_bl wasn't returned to the caller or used for anything other than determining whether to set "object_info_inconsistency" error. That is why I renamed it to first_bl and moved it out of the auth/auth_oi handling which are returned to the caller.

@dzafman dzafman merged commit 021177b into ceph:master Aug 18, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@dzafman dzafman deleted the dzafman:wip-20243 branch Aug 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.