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

Special scrub handling of hinfo_key errors #20947

Merged
merged 15 commits into from Apr 10, 2018
Merged

Conversation

dzafman
Copy link
Contributor

@dzafman dzafman commented Mar 16, 2018

No description provided.

bufferlist bl;
bufferlist::iterator bliter = k->second.begin();
decode(hi, bliter); // Can't be corrupted
f.dump_stream("hashinfo") << hi;
Copy link
Contributor Author

@dzafman dzafman Mar 17, 2018

Choose a reason for hiding this comment

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

Instead of using operator<< should we output JSON for the HashInfo?

Copy link
Member

Choose a reason for hiding this comment

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

sure

bufferlist bl;
bufferlist::iterator bliter = k->second.begin();
decode(ss, bliter); // Can't be corrupted
f.dump_stream("snapset") << ss;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update this too from operator<< to JSON for SnapSet?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, sounds like a good idea

@dzafman dzafman changed the title Special scrub handling of hinfo_key errors DNM: Special scrub handling of hinfo_key errors Mar 20, 2018
@dzafman
Copy link
Contributor Author

dzafman commented Mar 20, 2018

Make sure that non-authoritative corruption is also reflected in list-inconsistent-snapset.

@dzafman dzafman changed the title DNM: Special scrub handling of hinfo_key errors Special scrub handling of hinfo_key errors Mar 21, 2018
@dzafman
Copy link
Contributor Author

dzafman commented Mar 21, 2018

It would be a much more involved change to handle all shards when dealing with snapshot analysis. I filed tracker http://tracker.ceph.com/issues/23428 to at least indicate which authoritative shard was used to determine snapshot consistency.

@dzafman
Copy link
Contributor Author

dzafman commented Mar 21, 2018

Make sure that non-authoritative corruption is also reflected in list-inconsistent-snapset.

This is a lot of work which I don't think is a priority right now.

@jdurgin
Copy link
Member

jdurgin commented Mar 28, 2018

This looks good, I agree json for the snapset and hashinfo would be nice as well

@tchaikov
Copy link
Contributor

tchaikov commented Mar 29, 2018

@dzafman it's good to merge if we want to improve the json representation of snapset and hashinfo in another PR.

@dzafman dzafman added the DNM label Apr 4, 2018
@dzafman dzafman force-pushed the wip-23364 branch 2 times, most recently from d54e33a to 70e91b3 Compare April 6, 2018 04:28
@dzafman
Copy link
Contributor Author

dzafman commented Apr 6, 2018

@jdurgin @tchaikov I rebased this branch. Changed the system xattrs to output json format. Added a fix for tracker 23428.

Concerns for object_info/hashinfo/snapset:

  • It is easier to visually compare the differences in the single line format. Just use that format?
  • Keep the strings using old name (e.g. "object_info") for compatibility and add "*_json" for the json?
  • I noticed that the single line format doesn't show all fields
  • In the json output I'd like to show the flags as an array of strings instead of an integer

@dzafman dzafman removed the DNM label Apr 10, 2018
@dzafman
Copy link
Contributor Author

dzafman commented Apr 10, 2018

retest this please - run-tox-mgr-dashboard (Failed)

@jdurgin
Copy link
Member

jdurgin commented Apr 10, 2018

making the snapset/hash_info/object_info json means it's machine-readable, instead of having to parse it again. this is worth it imo, since the single-line format isn't legible to folks who don't know the code already.

Using flag names instead of an int is a good idea. I'm fine keeping the same field names, just add something to PendingReleaseNotes about it.

So basically, this looks good to me, it just needs a release note.

Signed-off-by: David Zafman <dzafman@redhat.com>
Signed-off-by: David Zafman <dzafman@redhat.com>
…t_missing/snapset_corrupted

Signed-off-by: David Zafman <dzafman@redhat.com>
oi_attr_missing -> info_missing
oi_attr_corrupted -> info_corrupted

Signed-off-by: David Zafman <dzafman@redhat.com>
data_digest_mismatch_oi -> data_digest_mismatch_info
omap_digest_mismatch_oi -> omap_digest_mismatch_info
size_mismatch_oi -> size_mismatch_info
obj_size_oi_mismatch -> obj_size_info_mismatch

Signed-off-by: David Zafman <dzafman@redhat.com>
System attributes shown as "object_info", "snapset" and "hashinfo"
Only output user attributes as "attrs"
	Drop leading undescore "_" for user attribute keys
Improve logic as to when to show user attributes or specific system attributes

Signed-off-by: David Zafman <dzafman@redhat.com>
Signed-off-by: David Zafman <dzafman@redhat.com>
Signed-off-by: David Zafman <dzafman@redhat.com>
Signed-off-by: David Zafman <dzafman@redhat.com>
Signed-off-by: David Zafman <dzafman@redhat.com>
Signed-off-by: David Zafman <dzafman@redhat.com>
@dzafman
Copy link
Contributor Author

dzafman commented Apr 10, 2018

@jdurgin I had to rebase because of conflicts with the PendingReleaseNotes

Add SnapSet bufferlist to inconsistent_snapset_t

Partial fix for http://tracker.ceph.com/issues/23428

Signed-off-by: David Zafman <dzafman@redhat.com>
Signed-off-by: David Zafman <dzafman@redhat.com>
Signed-off-by: David Zafman <dzafman@redhat.com>
@dzafman
Copy link
Contributor Author

dzafman commented Apr 10, 2018

retest this please

now snapset_missing). The error snapset_mismatch has been renamed to snapset_error
to better reflect what it means.

* The head snapset information is output in json format as "snapset." This means that
Copy link
Contributor

Choose a reason for hiding this comment

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

"snapset".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants