Have strat engine verify existence of required binaries #974

mulkieran · 2018-06-01T21:47:46Z

Related: #530

This is adapted from work in #936. My principle concern in this is to avoid a panic while the engine is executing if there is a missing binary.

mulkieran · 2018-06-06T19:11:52Z

rebased.

Assume that all binaries will be required by stratisd at some time, and that it shouldn't run if they are not available. This seems better to me than having a panic if we try to use them and they are not needed. It looks like using lazy_static and using the techniques that were used for implementing get_dm are not really all that different. Hoist cmd.rs up a level, since it seems like the engine really has to interact with it. Signed-off-by: mulhern <[email protected]>

It's not really an engine error if the executable has gone missing. Signed-off-by: mulhern <[email protected]>

mulkieran · 2018-06-07T15:36:52Z

This behaves identically to the get_dm() approach. Using lazy_static is a bit less verbose, so it makes sense to continue to use it. Can we use lazy_static for get_dm() then? It turns out that we can not, because error::Error is not Send. I think that the failure crate solves this problem, although I am not sure.

tasleson · 2018-06-07T18:53:33Z

existance -> existence

tasleson · 2018-06-07T19:01:03Z

src/engine/strat_engine/cmd.rs

+/// Common function to call a command line utility, returning an Result with an error message which
+/// also includes stdout & stderr if it fails.
+fn execute_cmd(cmd: &mut Command, error_msg: &str) -> StratisResult<()> {
+    let result = cmd.output()?;


I think is would be better to remove the exists check from get_executable and place it in the error path for cmd.output() so that we can than either raise the missing file error or return the error returned from trying to execute the command.

If the executable has disappeared, the error that will be returned will be the one from get_executable which currently has the bad text "Unable to find absolute path for ", but should have the text "Executable located at path seem to have been removed since stratisd started up". If the executable is still there, then the command will actually be executed and if that fails an error will be returned. I think that's just what is wanted.

I'm going to go ahead and improve the text, but otherwise I think it's correct.

My point was that instead of having to stat the executable every time to see if it exists before it's called we could just assume that the executable is still there, which it likely is and only if we encounter an error trying to execute it, see if it had gone missing. Thus the error message would be more helpful. The end result should be the same, the difference is when the existence check is done, either every time an executable is called or only when we encounter an error.

This would require some significant alterations to existing code for very moderate gain. I don't think it's worth doing unless this PR is used as a test case for the failure crate.

The suggested change makes the code behave more like the usual exception handling model, which would be an improvement. However, I would still like to merge this as is, and then experiment with the failure crate which I hope will allow me to implement the suggestion in a better manner than could be managed at this time.

tasleson · 2018-06-07T19:07:17Z

src/engine/strat_engine/engine.rs

    pub fn initialize() -> StratisResult<StratEngine> {
        let dm = get_dm_init()?;
+        verify_binaries()?;


If for some reason a user has an existing stratis setup, they update their system and during the update they mess up the required binaries the stratisd daemon will exit with error. If we aren't running we cannot monitor the thin pool(s) for out of space and other errors. Having systemd unit file instructing us to restart us isn't going to help as we will just exit until systemd gives up. I'm wondering if not exiting here would be better? This is much better than my proposed PR where we just panic.

If they just moved the binaries to a place where we expect, stratisd will find them again and all will be well. I don't see why this should happen, but who knows?

If some of the binaries that we rely on are actually gone, then stratisd can not function properly, anymore than it could function properly if devicemapper context is missing. We might as well exit.

In a future, we could work to modify stratisd so that it runs in some kind of crippled mode. But that would be complex. Maybe the future that comes sooner is that all or most of these become available as libraries! That would be marvelous.

tasleson · 2018-06-07T19:09:19Z

src/engine/strat_engine/cmd.rs

+
+lazy_static! {
+    static ref BINARIES: HashMap<String, Option<PathBuf>> = [
+        (MKFS_XFS.to_string(), find_binary(MKFS_XFS)),


For required binaries that are only needed during test what are you suggesting we do?

Make a separate HashMap to store the names and absolute paths of these test-only binaries.

Don't bother to write a verify_binaries method. We have a deliberate policy of unwrapping all over in tests, that would be consistent with this policy.

Add a method get_test_executable which has fewer expects()s and more unwraps() than get_executable consistent w/ the way tests are run. Use get_test_executable() when getting executables in tests.

Also, as much of the code that I outlined above should be modified with the #cfg[test] annotation as can be managed. I believe that would be all the code.

tasleson

Overall this looks much better than my quick implementation in #936

mulkieran · 2018-06-08T15:05:34Z

Note: I looked at a tiny Rust library called quale, which claims it implements the functionality of the "which" command. That's a whole different way of finding an executable. Is it better than our way or hard-coding 4 directories, or wrong?

tasleson · 2018-06-08T15:09:01Z

Note: I looked at a tiny Rust library called quale, which claims it implements the functionality of the "which" command. That's a whole different way of finding an executable. Is it better than our way or hard-coding 4 directories, or wrong?

The which command searches the environment PATH which won't exist in our use case, so I don't think it's better in that it likely won't work, but I've not looked at the source code for quale.

mulkieran · 2018-06-08T15:35:22Z

src/engine/strat_engine/cmd.rs

+    if !executable.exists() {
+        return Err(StratisError::Error(format!(
+            "Executable previously located at \"{}\" seems to have been removed since stratisd was started",
+            executable.to_str().expect("All parts of path are constructed from stricly ASCII components")


Whoops, there is a typo.

Signed-off-by: mulhern <[email protected]>

mulkieran · 2018-06-11T13:29:50Z

My feeling about this is that we shouldn't have to be doing this in the first place. About 50% of the reason that Project Springfield exists is so that this kind of thing can go away. In the meantime, since we have to do it, we should focus more than anything on the quarantine aspect of software engineering, the part where we just make sure that the bad stuff is organized into one place, and not spread all over our application. I think that we're managing that pretty well. However, we shouldn't go about refining it to the nth degree. We should focus on the stuff that makes Stratis interesting and make that very good. I'm going to answer all questions with that in mind.

There is, however, another consideration. It is in the nature of the cmd module to have relatively few dependencies and a fairly simple structure. It is nicely isolated from the rest of stratisd and should remain so. That means that it is likely to be a good object for testing the failure crate. In theory, it should be possible to use the failure crate in this one module and have no difficulty in interacting with the rest of stratisd.

Assuming I give this a try, I will try to make use of principles I have been trying to come up with when working on error handling in the CLI.

One principle is that methods should only take the arguments they need for execution. That is, they should never require additional arguments that are only used for error reporting. If a method fails, then the context in which it failed should be enough to fill in any necessary contextual information.

Another principle is that string-munging should not be used to add additional information to an error. The only way to get that information back out of the error programmatically is via string parsing, which is grim and chronically broken.

I think that these two are pretty obvious.

There is a third bit, which is more tricky. In a previous life, I used to write code to generate error messages from the attributes of a Python exception. I've come to the conclusion that that just leads to really bad prose. Now, I generate the message at the site where the exception is raised. I may also pass a bunch of arguments to the exception, some of which may also appear in some form in the string exception. This may be duplication, but it is, I believe, worth it. I'm going to try to do that here.

It's the correct thing to do anyway. Previous versions of the compiler would have helpfully reported an error instead of crashing. Signed-off-by: mulhern <[email protected]>

mulkieran · 2018-06-11T20:11:38Z

In stand-up, the point was made that only the dm executables are available in initramfs. So, with these changes, stratisd would not run properly when that's all that's available.

How would one possibly manage this? Well, we have to assume that if the dm executables are the only ones available, then those are the only ones that will be used. Can we be certain that that is true? We really ought to find out.

But, in the meantime, it was suggested that it is reasonable to go ahead w/ this patch without addressing that difficult issue, which will probably force some changes in code that is quite unrelated to this code. I believe that we should do this.

mulkieran · 2018-06-11T20:50:48Z

I've put up the final version, so I'm closing this.

mulkieran added the in progress label Jun 1, 2018

mulkieran self-assigned this Jun 1, 2018

mulkieran force-pushed the master-binary branch 2 times, most recently from bca0a7d to 1e7b242 Compare June 4, 2018 14:49

mulkieran added review-prelim and removed in progress labels Jun 4, 2018

mulkieran mentioned this pull request Jun 4, 2018

Avoid unwrap() in get_dm() method #976

Merged

mulkieran force-pushed the master-binary branch from 1e7b242 to 8ea4f92 Compare June 6, 2018 19:11

mulkieran force-pushed the master-binary branch from 8ea4f92 to 92627aa Compare June 7, 2018 14:56

mulkieran added 2 commits June 7, 2018 11:22

Use StratisError instead of NotFound for missing executable

c4088d0

It's not really an engine error if the executable has gone missing. Signed-off-by: mulhern <[email protected]>

mulkieran force-pushed the master-binary branch from 92627aa to c4088d0 Compare June 7, 2018 15:25

tasleson reviewed Jun 7, 2018

View reviewed changes

tasleson suggested changes Jun 7, 2018

View reviewed changes

mulkieran changed the title ~~Have strat engine verify existance of required binaries~~ Have strat engine verify existence of required binaries Jun 8, 2018

mulkieran force-pushed the master-binary branch from 672d0cc to 9ff5577 Compare June 8, 2018 15:34

mulkieran commented Jun 8, 2018

View reviewed changes

Correct error message for executable gone missing

fb892f7

Signed-off-by: mulhern <[email protected]>

mulkieran force-pushed the master-binary branch from 9ff5577 to fb892f7 Compare June 8, 2018 15:35

Use suggested workaround to avoid compiler error

41d3c31

It's the correct thing to do anyway. Previous versions of the compiler would have helpfully reported an error instead of crashing. Signed-off-by: mulhern <[email protected]>

mulkieran mentioned this pull request Jun 11, 2018

Have strat engine verify existence of required binaries #986

Merged

mulkieran closed this Jun 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have strat engine verify existence of required binaries #974

Have strat engine verify existence of required binaries #974

mulkieran commented Jun 1, 2018 •

edited

Loading

mulkieran commented Jun 6, 2018

mulkieran commented Jun 7, 2018 •

edited

Loading

tasleson commented Jun 7, 2018

tasleson Jun 7, 2018

mulkieran Jun 8, 2018

tasleson Jun 8, 2018

mulkieran Jun 11, 2018

mulkieran Jun 11, 2018

tasleson Jun 7, 2018

mulkieran Jun 11, 2018

tasleson Jun 7, 2018

mulkieran Jun 11, 2018

mulkieran Jun 11, 2018 •

edited

Loading

tasleson left a comment

mulkieran commented Jun 8, 2018

tasleson commented Jun 8, 2018

mulkieran Jun 8, 2018

mulkieran commented Jun 11, 2018

mulkieran commented Jun 11, 2018

mulkieran commented Jun 11, 2018

Have strat engine verify existence of required binaries #974

Have strat engine verify existence of required binaries #974

Conversation

mulkieran commented Jun 1, 2018 • edited Loading

mulkieran commented Jun 6, 2018

mulkieran commented Jun 7, 2018 • edited Loading

tasleson commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mulkieran Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

tasleson left a comment

Choose a reason for hiding this comment

mulkieran commented Jun 8, 2018

tasleson commented Jun 8, 2018

Choose a reason for hiding this comment

mulkieran commented Jun 11, 2018

mulkieran commented Jun 11, 2018

mulkieran commented Jun 11, 2018

mulkieran commented Jun 1, 2018 •

edited

Loading

mulkieran commented Jun 7, 2018 •

edited

Loading

mulkieran Jun 11, 2018 •

edited

Loading