[WIP] StaticHeader::setup Improve error reporting by GuillaumeGomez · Pull Request #1562 · stratis-storage/stratisd

GuillaumeGomez · 2019-06-17T15:19:27Z

Before finalizing this: do we want to provide two different kind of errors in here or consider the second case of error as non-fatal and providing it alongside the returned data (like I did currently)?

stratis-bot · 2019-06-17T15:20:04Z

Test with Jenkins?

stratis-bot · 2019-06-17T15:22:03Z

Test with Jenkins?

mulkieran · 2019-06-17T15:23:08Z

ok to test

mulkieran · 2019-06-17T15:33:50Z

Before finalizing this: do we want to provide two different kind of errors in here or consider the second case of error as non-fatal and providing it alongside the returned data (like I did currently)?

Yes, this is exactly right. We should consider the error on repair attempt as non-fatal, because if we tried to repair that means we think we have good data on hand.

GuillaumeGomez · 2019-06-17T15:45:55Z

@mulkieran suggested to use an enum instead and also to add #[must_use] usage. I'll check with those solutions.

GuillaumeGomez · 2019-06-18T12:39:42Z

Should I do something with the errors or...?

src/engine/strat_engine/backstore/metadata/bda.rs

GuillaumeGomez · 2019-06-19T12:18:08Z

Updated. Is there anything else to do in here?

src/engine/strat_engine/backstore/metadata/bda.rs

mulkieran · 2019-06-19T15:11:14Z

src/engine/strat_engine/backstore/metadata/bda.rs

-            Ok(Some(sh)) => Ok(Some((sh.pool_uuid, sh.dev_uuid))),
+            Ok(Some(SetupResult::Ok(sh))) => Ok(Some((sh.pool_uuid, sh.dev_uuid))),
+            Ok(Some(SetupResult::OkWithError(sh, err))) => {
+                warn!("StaticHeader::setup didn't completely succeed: {:?}", err);


The I/O error that you picked up doesn't really explain what happened. So, a clearer explanation would be something like:

warn!("Experienced an I/O error while attempting to repair an ill-formed, unreadable, or stale signature block: {:?}", err);

This has the flaw that it does not identify which signature block, on which device, etc., so it is more alarming than helpful in that simple state.

But you can improve it quite a bit, by including the good StaticHeader, the one that was succesfully read, in the error message (or possibly just selected identifying information from that StaticHeader). This would probably be worth a separate function, since you do it in two places.

I extended your text a bit. Tell me if it's what you had in mind otherwise I'll just update. :)

src/engine/strat_engine/backstore/metadata/bda.rs

mulkieran · 2019-06-25T13:34:47Z

@GuillaumeGomez Could you rebase this one? device_identifiers has moved and changed its structure, so there is a conflict. I believe it shouldn't be hard for you to adapt your changes to the new structure, and location.

GuillaumeGomez · 2019-06-25T13:48:16Z

Sure!

GuillaumeGomez · 2019-06-25T14:27:31Z

Rebased.

mulkieran · 2019-06-25T19:02:00Z

Makes sense to call this blocked b #1575.

mulkieran · 2019-07-02T13:17:09Z

unblocked.

GuillaumeGomez · 2019-07-02T13:17:57Z

I need to rebase once again. :)

mulkieran · 2019-07-02T13:30:35Z

yup. conflicts galore, unfortunately, but ultimately a better situation.

stratis-bot · 2019-07-02T19:32:08Z

Test with Jenkins?

src/engine/strat_engine/backstore/metadata/bda.rs

GuillaumeGomez · 2019-07-03T18:41:05Z

cargo fmt once again. T_T

mulkieran

I'm not done just yet, I think that if the extra curly brace is removed then it'll look like fewer changes, which will make it easier to review. So, please go ahead and do that and I'll take another look.

mulkieran · 2019-07-03T20:02:31Z

src/engine/strat_engine/backstore/metadata/bda.rs

    }
 }

+fn setup_warn(header: &StaticHeader, err: StratisError) {


Please add some explanatory header comments.

I added a comment. Considering the function was just emitting a warning, I didn't think it was necessary to kinda repeat what it was doing but I guess a comment cannot hurt. Also, don't hesitate if the comment isn't well written.

src/engine/strat_engine/backstore/metadata/bda.rs

mulkieran · 2019-07-03T20:16:23Z

src/engine/strat_engine/backstore/metadata/bda.rs

+    warn!(
+        "Experienced an I/O error while attempting to repair an ill-formed, \
+         unreadable, or stale signature block: {:?}.\n\
+         Returning device \"{:?}\" from pool \"{:?}\".",


How about Read and returned static header {:?}, sh for the second sentence. We implemented Debug for StaticHeader so that is doable; we had to implement it explicitly since the Debug info for UUIDs is far too long.

I updated the text but not sure if it was how you had it in mind?

src/engine/strat_engine/backstore/metadata/bda.rs

GuillaumeGomez · 2019-07-04T20:26:36Z

Updated.

mulkieran · 2019-07-05T13:31:48Z

Travis failed due to infrastructure errors, not test failiures, I tried restarting it.

mulkieran

Thanks, I think we're getting close to finish. Please look into the further abstractions suggested...it doesn't make sense to waste the opportunity, once we've made it this far.

Right now this PR needs only 50 additional lines of code to introduce a significant improvement in behavior, because it improves the method structure at the same time; I hope we can do better still.

src/engine/strat_engine/backstore/metadata/bda.rs

mulkieran · 2019-07-05T13:20:47Z

src/engine/strat_engine/backstore/metadata/bda.rs

+fn setup_warn(header: &StaticHeader, err: StratisError) {
+    warn!(
+        "Experienced an I/O error while attempting to repair an ill-formed, \
+         unreadable, or stale signature block: {:?}.\n\


Please drop the newline. We should allow something else to do the formatting at some later, post-processing step.

mulkieran · 2019-07-05T13:41:57Z

src/engine/strat_engine/backstore/metadata/bda.rs

    where
        F: Read + Seek + SyncAll,
    {
+        fn bda_write_check<F>(


Please change name to just write_check. The use of bda is wrong, because some bad nomenclature crept in in a previous PR, and since the method is defined in setup, it doesn't really need a long identifying name anyway.

src/engine/strat_engine/backstore/metadata/bda.rs

GuillaumeGomez · 2019-07-08T13:04:05Z

Updated.

mulkieran · 2019-07-08T15:37:10Z

@GuillaumeGomez I've realized that there is yet another preliminary step that should happen before you try to move forward w/ this. I'ld already worked it out, but now I realize that it will conflict w/ this very badly. So, please put this on hold, and I'll assign to you this additional pre-project.

mulkieran · 2019-07-08T15:43:01Z

Blocked by #1579.

mulkieran · 2019-07-16T12:56:12Z

Blocked by #1586.

stratis-bot · 2020-06-23T14:13:31Z

Test with Jenkins?

stratis-bot · 2020-11-25T21:57:27Z

Can one of the admins verify this patch?

mulkieran assigned GuillaumeGomez Jun 17, 2019

GuillaumeGomez force-pushed the improve-error-handling branch from 3463876 to 8759a4e Compare June 18, 2019 12:36

mulkieran requested changes Jun 18, 2019

View reviewed changes

GuillaumeGomez force-pushed the improve-error-handling branch 2 times, most recently from 7307162 to d1ef8a3 Compare June 19, 2019 12:17

mulkieran requested changes Jun 19, 2019

View reviewed changes

GuillaumeGomez force-pushed the improve-error-handling branch 3 times, most recently from db984b7 to c39cb15 Compare June 24, 2019 15:47

bgurney-rh reviewed Jun 24, 2019

View reviewed changes

src/engine/strat_engine/backstore/metadata/bda.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the improve-error-handling branch from c39cb15 to 9daec43 Compare June 24, 2019 17:26

GuillaumeGomez force-pushed the improve-error-handling branch from 9daec43 to 68f6beb Compare June 25, 2019 14:27

mulkieran added the blocked label Jun 25, 2019

mulkieran removed the blocked label Jul 2, 2019

GuillaumeGomez force-pushed the improve-error-handling branch from 68f6beb to d63a18e Compare July 2, 2019 21:29

GuillaumeGomez force-pushed the improve-error-handling branch from d63a18e to 6457012 Compare July 2, 2019 21:41

bgurney-rh reviewed Jul 3, 2019

View reviewed changes

src/engine/strat_engine/backstore/metadata/bda.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the improve-error-handling branch 2 times, most recently from fe145a7 to 32d98e5 Compare July 3, 2019 18:40

mulkieran requested changes Jul 3, 2019

View reviewed changes

GuillaumeGomez force-pushed the improve-error-handling branch from 32d98e5 to b95c5c9 Compare July 4, 2019 20:23

mulkieran requested changes Jul 5, 2019

View reviewed changes

GuillaumeGomez force-pushed the improve-error-handling branch from b95c5c9 to 22f2281 Compare July 8, 2019 12:34

StaticHeader::setup Improve error reporting

8f09efb

GuillaumeGomez force-pushed the improve-error-handling branch from 22f2281 to 8f09efb Compare July 8, 2019 12:40

mulkieran added the blocked label Jul 8, 2019

mulkieran mentioned this pull request Jul 16, 2019

StaticHeader::setup presents numeorus opportunities for procedure extraction #1586

Closed

mulkieran marked this pull request as draft March 15, 2021 20:52

Conversation

GuillaumeGomez commented Jun 17, 2019

Uh oh!

stratis-bot commented Jun 17, 2019

Uh oh!

stratis-bot commented Jun 17, 2019

Uh oh!

mulkieran commented Jun 17, 2019

Uh oh!

mulkieran commented Jun 17, 2019

Uh oh!

GuillaumeGomez commented Jun 17, 2019

Uh oh!

GuillaumeGomez commented Jun 18, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuillaumeGomez commented Jun 19, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mulkieran commented Jun 25, 2019

Uh oh!

GuillaumeGomez commented Jun 25, 2019

Uh oh!

GuillaumeGomez commented Jun 25, 2019

Uh oh!

mulkieran commented Jun 25, 2019

Uh oh!

mulkieran commented Jul 2, 2019

Uh oh!

GuillaumeGomez commented Jul 2, 2019

Uh oh!

mulkieran commented Jul 2, 2019

Uh oh!

stratis-bot commented Jul 2, 2019

Uh oh!

Uh oh!

GuillaumeGomez commented Jul 3, 2019

Uh oh!

mulkieran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GuillaumeGomez commented Jul 4, 2019

Uh oh!

mulkieran commented Jul 5, 2019

Uh oh!

mulkieran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!