Removed tempfile dependency #1434

iulianbarbu · 2019-11-26T11:23:02Z

Signed-off-by: Iulian Barbu iul@amazon.com

Reason for This PR

Description of Changes

Some of Firecracker crates have dev-dependency on tempfile crate from crates.io. This crate has other dependencies on its own that are brought into Firecracker as well.

Removed tempfile crate dev-dependency and used instead vmm-sys-util tempfile module, which basically replaces tempfile crate with a simpler implementation for temporary files & temporary directories, without additional dependencies.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria. Where there are two options, keep one.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
Either this PR is linked to an issue, or, the reason for this PR is
clearly provided.
The description of changes is clear and encompassing.
Either no docs need to be updated as part of this PR, or, the required
doc changes are included in this PR. Docs in scope are all *.md files
located either in the repository root, or in the docs/ directory.
Either no code has been touched, or, code-level documentation for touched
code is included in this PR.
Either no API changes are included in this PR, or, the API changes are
reflected in firecracker/swagger.yaml.
Either the changes in this PR have no user impact, or, the changes in
this PR have user impact and have been added to the CHANGELOG.md file.
Either no new unsafe code has been added, or, the newly added unsafe
code is unavoidable and properly documented.

andreeaflorescu · 2019-11-26T11:27:25Z

devices/src/virtio/block.rs

+                        .read(true)
+                        .write(true)


Shouldn't we add open options based on is_disk_read_only flag?

Yes, I agree. I've updated, please take a look: devices/src/virtio/block.rs:716.

devices/src/virtio/block.rs

logger/src/lib.rs

devices/src/virtio/block.rs

jailer/src/cgroup.rs

devices/src/virtio/block.rs

serban300 · 2019-11-29T12:07:15Z

utils/src/tempfile.rs

+/// otherwise inside `/tmp` if non-Android system.
+pub fn tempfile() -> Result<tempfile::TempFile> {
+    let mut in_tmp_dir = temp_dir();
+    in_tmp_dir.push("");


why is this needed ?

I'll leave a comment with the reason above the line in the code.

The reason is that the call for temp_dir is returning a PathBuf with the content /tmp. When creating a TempFile::new(prefix), the prefix needs to be an explicit path/prefix where the temporary file name (a random string of 5 chars) will be placed/appended to.

If we would have just the path /tmp, when we will try to create a temporary file inside the /tmp directory, that will not happen because, the actual call to TempFile::new("/tmp") will create a file which will have a name similiar to this /tmpXVFGB, which is incorrect.

The call to in_tmp_dir.push("") will append a trailing /, which will result in /tmp/ and which will make successful the call to TempFile::new(in_tmp_dir).

serban300 · 2019-11-29T12:08:52Z

utils/src/tempfile.rs

+/// * `path` - A path to a specific directory (e.g "/tmp/x" or "/tmp/x/").
+pub fn tempfile_in(path: &Path) -> Result<tempfile::TempFile> {
+    let mut path_buf = path.canonicalize().unwrap();
+    path_buf.push("");


why is this needed ?

Same as above.

serban300 · 2019-11-29T12:09:34Z

utils/src/tempfile.rs

+    use crate::tempfile::tempfile;
+
+    #[test]
+    fn test_tempfile_new() {


the method name seems outdated.

serban300 · 2019-11-29T12:12:25Z

utils/src/tempfile.rs

+/// Wrapper over vmm_sys_util::tempfile::TempFile::new().
+/// This creates a temporary file inside $TMPDIR if set,
+/// otherwise inside `/tmp` if non-Android system.
+pub fn tempfile() -> Result<tempfile::TempFile> {


I'm not sure if this should be called simply tempfile() or if something like create_temfile() would be better.

I'd rather keep it simple. I admit that I am biased because of the API offered by tempfile crate from crates.io, which exposes the method with the same name for creating an "anonymous" temporary file.

The method is documented and commented, so its usage should be pretty clear, while its name, although does not explicitly suggest creation of a tempfile, it surely implies it, because you can see it as a short form of get_tempfile().

andreeaflorescu · 2019-11-29T11:59:23Z

utils/src/tempdir.rs

+
+/// Wrapper over vmm_sys_util::tempdir::TempDir::new().
+/// This creates a temporary directory inside $TMPDIR if set,
+/// otherwise inside `/tmp` if non-Android system.


What's with the non-Android comment?

I'll remove the note, because it is useless in our case. In Android systems the "/tmp" directory is "/data/local/tmp".

andreeaflorescu · 2019-11-29T12:52:21Z

utils/src/tempfile.rs

+/// Wrapper over vmm_sys_util::tempfile::TempFile::new().
+/// This creates a temporary file inside $TMPDIR if set,
+/// otherwise inside `/tmp` if non-Android system.
+pub fn tempfile() -> Result<tempfile::TempFile> {


I was picturing this more like a new Tempfile structure that extends the existing Tempfile from vmm-sys-util. I also thing that the goal should be to merge this changes in vmm-sys-util. To unblock ourselves it's okay to have these changes in Firecracker.

I quickly typed something that works as an example (it can be made nicer):

pub struct TempFile(tempfile::TempFile); impl TempFile { fn new() -> Result<Self> { let mut in_tmp_dir = temp_dir(); in_tmp_dir.push(""); let temp_file = tempfile::TempFile::new(in_tmp_dir)?; Ok(TempFile(temp_file)) } fn new_with_path(path: &Path) -> Result<Self> { let mut in_tmp_dir = temp_dir(); let temp_file = tempfile::TempFile::new(in_tmp_dir)?; Ok(TempFile(temp_file)) } fn path_ref(&self) -> &Path { self.0.as_path() } }

The reason for which I prefer a structure is because I don't particularly like orphan functions, but this is subjective. Let me know what you think.

Update: you don't need path_ref as you can implement deref for TempFile.

impl Deref for TempFile { type Target = tempfile::TempFile; fn deref(&self) -> &Self::Target { &self.0 } }

Sure, this would work better, since it resembles the struct from vmm-sys-utils. I'll make it so.

acatangiu · 2019-12-02T13:09:12Z

src/jailer/Cargo.toml


 [dev-dependencies]
-tempfile = ">=3.0.2"
+utils = { path = "../utils" }


not required, already present in dependencies

acatangiu · 2019-12-02T13:14:16Z

src/logger/Cargo.toml


 [dev-dependencies]
-tempfile = ">=3.0.2"
+utils = { path = "../utils" }


same here, not required.

acatangiu · 2019-12-02T13:15:23Z

src/logger/src/lib.rs

-    use self::tempfile::NamedTempFile;
-    use super::*;
-    use log::MetadataBuilder;
+    extern crate utils;


Not required, already declared outside of tests.

acatangiu · 2019-12-02T13:19:47Z

src/memory_model/Cargo.toml


 [dev-dependencies]
-tempfile = ">=3.0.2"
+utils = { path = "../utils" }


not required

acatangiu · 2019-12-02T13:29:27Z

src/utils/src/tempdir.rs

+    pub fn new() -> Result<Self> {
+        let mut in_tmp_dir = temp_dir();
+        // This `push` adds a trailing slash ("/tmp" -> "/tmp/").
+        // This is safe for paths with already trailing slash.
+        in_tmp_dir.push("");
+        let temp_dir = tempdir::TempDir::new(in_tmp_dir)?;
+        Ok(TempDir(temp_dir))
+    }


I would've liked to have this directly in vmm-sys-util and not wrap it ourselved in Firecracker. I say you open a PR there as well and not let this dangling.

I agree. We should expose this functionality from vmm-sys-util. I will open a PR there soon.

acatangiu · 2019-12-02T13:30:09Z

src/vmm/Cargo.toml


 [dev-dependencies]
-tempfile = ">=3.0.2"
+utils = { path = "../utils" }


Suggested change

utils = { path = "../utils" }

acatangiu · 2019-12-02T13:34:13Z

src/vmm/src/lib.rs

-    extern crate tempfile;
-
-    use super::*;
+    extern crate utils;


Suggested change

extern crate utils;

And while you're at it please also remove:

extern crate logger; extern crate utils;

in src/vmm/src/signal_handler.rs.

acatangiu · 2019-12-02T13:35:05Z

src/vmm/src/vmm_config/drive.rs

 #[cfg(test)]
 mod tests {
-    extern crate tempfile;
+    extern crate utils;


Suggested change

extern crate utils;

acatangiu · 2019-12-02T13:35:27Z

src/vmm/src/vmm_config/logger.rs

 mod tests {
-    extern crate tempfile;
-    use self::tempfile::NamedTempFile;
+    extern crate utils;


Suggested change

extern crate utils;

iulianbarbu · 2019-12-03T09:27:54Z

I've included the received feedback. Please, take a look at 2399233.

dianpopa · 2019-12-05T12:22:46Z

Will continue work here only after rust-vmm/vmm-sys-util#54 has been merged.

dianpopa · 2019-12-10T13:24:26Z

src/devices/Cargo.toml

 rate_limiter = { path = "../rate_limiter" }
 virtio_gen = { path = "../virtio_gen" }

 [dev-dependencies]


Your commit message contains a typo but I d go further and I d say Replace tempfile external dependency and in the description I would say that we are now using vmm-sys-util's tempfile implementation. Remove tempfile... suggests that we are no longer using temporary files in Firecracker at all which is inaccurate.

Remove tempfile ... is inaccurate, I agree, but Remove tempfile dependency it has something to do with the dependency, not with the tempfile. I think the commit needs a proper description, to log basically what the change has achieved. I'll add a description.

dianpopa · 2019-12-10T13:24:39Z

src/devices/src/virtio/block.rs


+    use libc;
+
+    use self::utils::tempfile::TempFile;


You do not need self:: here.

True that. Removed self::. Take a look: src/devices/src/virtio/block.rs.

dianpopa · 2019-12-10T13:26:22Z

src/firecracker/Cargo.toml


-[dev-dependencies]
-tempfile = ">=3.0.2"
+[dev-dependencies]


There is a no newline at end of file warning. However, why not removing the dev-dependencies section for good?

dianpopa · 2019-12-10T13:27:54Z

src/jailer/src/cgroup.rs

    use std::io::Write;
    use std::path::PathBuf;

+    use self::utils::tempdir::TempDir;


No need for self::

dianpopa · 2019-12-10T13:29:52Z

src/memory_model/src/mmap.rs

    use std::mem;
    use std::path::Path;

+    use self::utils::tempfile::TempFile;


no need for self::

I couldn't run the tests if I wouldn't use self::. The reason is that extern crate utils appears only inside themmap::tests module, and is not defined outside, in other file from memory_model crate, to be imported when we use super::*. Per my understanding, in this situation, utils crate is visible only within mod tests. To be used even by the module itself, the use has to be referenced with self::.

dianpopa · 2019-12-10T14:44:18Z

src/logger/src/lib.rs

                &app_info,
-                Box::new(log_file_temp),
-                Box::new(metrics_file_temp),
+                Box::new(


By looking at the TempFile implementation, calling the new() method will create a Path and File object for you.
However, calling OpenOptions::new will create a new File at the path previosuly returned by TempFile. In order to avoid recreating a new File, we need a way to make use of the File inside the TempFile object. For this logger specific usecase, one way would be to implement the relevant File traits on TempFile (I.e Write, Read etc).
I already opened a PR in vmm-sys-util that implements those traits since I think this is a miss of the initial implementation. So, I suggest waiting for ti to be merged.
After that, your code will just look like this:

ssert!(l .init( &app_info, Box::new( log_file_temp ), Box::new( metrics_file_temp ), ) .is_ok());

Actually, because it might spread to another week, what do you think if we merge this like it is, if we get the approves? We can update with your suggestion in a subsequent PR. Thinking about it, this PR should've not been blocked at all. The updates could've been added in subsequent PRs (TempFile API changes, signal API refactoring). Please let me know what you think. I just want to not get blocked here everytime we find things that miss or need changes in rust-vmm/vmm-sys-util, because I found another thing that needs a change on rust-vmm/vmm-sys-util, and I've managed to work around it, although I would prefer pushing changes to rust-vmm/vmm-sys-util.

dianpopa · 2019-12-10T15:07:58Z

src/devices/src/virtio/block.rs

            DummyBlock {
-                block: Block::new(f, is_disk_read_only, epoll_config, Some(rate_limiter)).unwrap(),
+                block: Block::new(
+                    OpenOptions::new()


Why not tmp_f.as_file().try_clone().unwrap() instead of OpenOptions::new()..?

Yes. This option will do, but we have to modify the permissions on the cloned file. Basically, we have to give write permission or not, depending on the is_disk_read_only variable. Take look: src/devices/src/virtio/block.rs.

dianpopa · 2019-12-10T15:10:17Z

src/memory_model/src/mmap.rs

            .is_ok());

-        let mut f = tempfile().unwrap();
+        let temp_f = TempFile::new().unwrap();


When TempFile will implement Read, you can do:

let mut temp_f = ... `.read_to_memory(1, &mut temp_f, mem::size_of::<u32>())`

Removed `tempfile` crate dependency, from crates.io, which was heavily using other dependencies which Firecracker does not use. Used instead `rust-vmm/vmm-sys-util` tempfile implementation, which provides support from temporary file and directory creation. Fixes firecracker-microvm#1125 Signed-off-by: Iulian Barbu <iul@amazon.com>

Changes look good now

acatangiu · 2019-12-11T16:07:52Z

@iulianbarbu if we merge it like this, please add an issue describing the leftover improvements so we can track them.

iulianbarbu · 2019-12-11T17:39:49Z

@iulianbarbu if we merge it like this, please add an issue describing the leftover improvements so we can track them.

I've added the following issue: #1466.

iulianbarbu force-pushed the remove_tempfile_dependency branch from 7f77d6d to 79cde8f Compare November 26, 2019 11:23

andreeaflorescu reviewed Nov 26, 2019

View reviewed changes

iulianbarbu force-pushed the remove_tempfile_dependency branch from 79cde8f to 145621d Compare November 26, 2019 12:42

lauralt reviewed Nov 26, 2019

View reviewed changes

devices/src/virtio/block.rs Show resolved Hide resolved

logger/src/lib.rs Show resolved Hide resolved

iulianbarbu force-pushed the remove_tempfile_dependency branch from 145621d to f1754fb Compare November 26, 2019 15:22

lauralt reviewed Nov 26, 2019

View reviewed changes

devices/src/virtio/block.rs Outdated Show resolved Hide resolved

jailer/src/cgroup.rs Outdated Show resolved Hide resolved

iulianbarbu force-pushed the remove_tempfile_dependency branch 4 times, most recently from 712a370 to fac9248 Compare November 28, 2019 09:31

dianpopa assigned iulianbarbu Nov 28, 2019

serban300 reviewed Nov 28, 2019

View reviewed changes

devices/src/virtio/block.rs Outdated Show resolved Hide resolved

iulianbarbu force-pushed the remove_tempfile_dependency branch 6 times, most recently from ed4ee2c to d7d365f Compare November 29, 2019 11:46

serban300 reviewed Nov 29, 2019

View reviewed changes

andreeaflorescu reviewed Nov 29, 2019

View reviewed changes

iulianbarbu force-pushed the remove_tempfile_dependency branch 3 times, most recently from 94c3466 to f68b1ad Compare November 29, 2019 14:19

acatangiu reviewed Dec 2, 2019

View reviewed changes

dianpopa added the NextStep: assignee's feedback label Dec 2, 2019

iulianbarbu force-pushed the remove_tempfile_dependency branch 2 times, most recently from 7f593e3 to 2399233 Compare December 2, 2019 15:45

serban300 previously approved these changes Dec 3, 2019

View reviewed changes

dianpopa added Status: Blocked Indicates that an issue or pull request cannot currently be worked on and removed Status: Author labels Dec 5, 2019

iulianbarbu dismissed serban300’s stale review via bb427fd December 9, 2019 18:55

iulianbarbu force-pushed the remove_tempfile_dependency branch 5 times, most recently from b1cee13 to 1279129 Compare December 10, 2019 06:47

serban300 previously approved these changes Dec 10, 2019

View reviewed changes

iulianbarbu dismissed serban300’s stale review via 116e01e December 10, 2019 14:10

iulianbarbu force-pushed the remove_tempfile_dependency branch 2 times, most recently from 116e01e to aa9dd8f Compare December 10, 2019 14:29

dianpopa previously requested changes Dec 10, 2019

View reviewed changes

iulianbarbu force-pushed the remove_tempfile_dependency branch 2 times, most recently from a48e5de to 10ded83 Compare December 10, 2019 20:00

iulianbarbu added Status: Awaiting review Indicates that a pull request is ready to be reviewed and removed Status: Blocked Indicates that an issue or pull request cannot currently be worked on labels Dec 10, 2019

iulianbarbu force-pushed the remove_tempfile_dependency branch from 10ded83 to c61203f Compare December 10, 2019 23:56

iulianbarbu force-pushed the remove_tempfile_dependency branch from c61203f to 3cc80f3 Compare December 10, 2019 23:58

dianpopa approved these changes Dec 11, 2019

View reviewed changes

Merge branch 'master' into remove_tempfile_dependency

48197fd

serban300 approved these changes Dec 11, 2019

View reviewed changes

iulianbarbu merged commit f244f51 into firecracker-microvm:master Dec 11, 2019

iulianbarbu mentioned this pull request Dec 11, 2019

Use public SIGRTMIN() and SIGRTMAX() from rust-vmm/vmm-sys-util #1467

Closed

dianpopa mentioned this pull request Jan 13, 2020

Remove rand dependency #884

Closed

iulianbarbu deleted the remove_tempfile_dependency branch February 18, 2020 14:21

Removed tempfile dependency #1434

Removed tempfile dependency #1434

Uh oh!

Conversation

iulianbarbu commented Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

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!

Uh oh!

Uh oh!

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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iulianbarbu Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iulianbarbu commented Dec 3, 2019

Uh oh!

dianpopa commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

iulianbarbu commented Nov 26, 2019 •

edited

Loading

iulianbarbu Dec 2, 2019 •

edited

Loading

dianpopa commented Dec 5, 2019 •

edited

Loading

iulianbarbu Dec 10, 2019 •

edited

Loading

iulianbarbu Dec 10, 2019 •

edited

Loading