Mostly updating crates

[drahnr]

This includes using Pid ,Gid, 'Uid' wherever possible.

Next steps will include replacing error_chain with failure and revamping error handling in general.

[drahnr]

Nighlty rust version 1.30.0-nightly (67390c0c3 2018-08-14) builds just fine for me locally and I can not inspect the output of wercker since that pipeline is private.

[vishvananda]

fit looks like there is a conflict, perhaps you need to rebase?

The error message from the build pipeline is the following:

error[E0597]: borrowed value does not live long enough
   --> src/main.rs:390:32
    |
390 |     let _ = ::log::set_logger(&logger::SimpleLogger)
    |                                ^^^^^^^^^^^^^^^^^^^^ does not live long enough
391 |         .map(|()| log::set_max_level(level));
    |                                             - temporary value only lives until here
    |
    = note: borrowed value must be valid for the static lifetime...
    = note: consider using a `let` binding to increase its lifetime

I'll look at getting the pipelines opened up so you can view them.

[drahnr]

Should be fixed now.

Note that some error checks are more strict now and will cause an error rather than silently being ignored, and I am not sure if this is a good or a bad thing.

Generally, looking at the code it feels like one abstraction layer is missing, there are (for my taste) too many -1 process IDs passed around rather than using Some and hiding away magic numbers passed to POSIX/Linux API

[drahnr]

If there is anything that you think I should change, I'd be happy to receive some feedback :) + improve

[vishvananda]

In general this seems great. I agree that we could avoid using -1 for pid although it does simplify unpacking in some cases. I made a comment of one oddity inline. A few other points:

1. There are quite a few formatting changes. The formatting was all originally done via rust-fmt. Are these changes due to a newer version of rust-fmt? If not, I suggest running rust-fmt on the code to keep the formatting consistent.

2. For me to merge you'll have to sign and submit an OCA:

https://www.oracle.com/technetwork/community/oca-486395.html

Once you have sent it, please let me know so I can put it into the system on our end. It is a manual process at the moment. We are working on automating it.

3. There is some internal process I have to go through to update versions of rust dependencies, so it might take me a couple of weeks to merge (post oca signing).

Thanks for the submission!

[drahnr]

I'll run cargo fmt one more time, rebasing on master might have caused this.

OCA should be sent in by tonight, I 'll let you know once it's on its way.

[drahnr]

@vishvananda sent the OCA out

[drahnr]

I finally got around to complete this.

[vishvananda]

added a couple more comments inline. I'm still working on the internal process. Might take a week or two.

[vishvananda]

Ok i finally got through the internal process. I'm trying out this branch and it looks like there is a bug. Container is failing to start when using railcar as a docker backend. Debugging now. More info soon.

[vishvananda]

So the following patch makes everything work again:

diff --git a/src/mounts.rs b/src/mounts.rs
index daf274d..0c6fb4c 100644
--- a/src/mounts.rs
+++ b/src/mounts.rs
@@ -482,30 +482,32 @@ fn mask_path(path: &str) -> Result<()> {
         return Err(ErrorKind::InvalidSpec(msg).into());
     }

-    let _ = mount(
+    if let Err(e) = mount(
         Some("/dev/null"),
         path,
         None::<&str>,
         MsFlags::MS_BIND,
         None::<&str>,
-    ).map_err(|e| {
+    ) {
         match e {
             ::nix::Error::Sys(errno) => {
                 // ignore ENOENT and ENOTDIR: path to mask doesn't exist
                 if errno != Errno::ENOENT && errno != Errno::ENOTDIR {
-                    let _ = format!("could not mask {}", path);
-                    e
+                    let msg = format!("could not mask {}", path);
+                    Err(e).chain_err(|| msg)?;
                 } else {
                     debug!(
                         "ignoring mask of {} because it doesn't exist",
                         path
                     );
-                    e
                 }
             }
-            _ => e,
+            _ => {
+                let msg = format!("could not mask {}", path);
+                Err(e).chain_err(|| msg)?;
+            }
         }
-    })?;
+    }
     Ok(())
 }

@@ -529,6 +531,7 @@ fn readonly_path(path: &str) -> Result<()> {
                     Err(e).chain_err(|| msg)?;
                 }
                 debug!("ignoring remount of {} because it doesn't exist", path);
+                return Ok(());
             }
             _ => {
                 return Ok(());

I also included comments inline to show what was going wrong. In general was there a reason to switch to a match statement in these cases vs the ifs I was using previously? I think the match makes it more difficult to follow the logic. There may be a better way to organize the above as well, I just made it work without too much rewriting.

[drahnr]

I'll take care of it. Thanks for your review. Tbh since the testset passed I did not poke it much beyond that.

[vishvananda]

Note that my patch above also has that same unreachable condition in mask_path. Yes we need some functional tests to make sure it works. I was using the oci-runtime tests but I ran them manually and they weren't very comprehensive, so I ended up having to just run railcar as a backend for docker to pick up the complex cases. Maybe we can automate an integration test that does that.

[drahnr]

Do you have a collection of tests you do for complex cases that I could use to validate my changes before I push them, at least to reduce the ping pong a bit?

[vishvananda]

The best verification test is to add railcar as a backend for docker (explained in the readme) and just try to run hello world: docker run --runtime=rc -it --rm hello-world That should at least tell you if its working. Unfortunately, when that doesn't work there isn't a great path for debugging.

[vishvananda]

Looks like this builds ok. I will merge now. Then I'll try to figure out how to include some better merge testing.