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.

Sign up for GitHub

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

Jump to bottom

Add test driver py copy from vm #74077

Merged
merged 2 commits into from
Dec 1, 2019
Merged

Add test driver py copy from vm #74077

merged 2 commits into from
Dec 1, 2019
+34 −1

Conversation

7c6f434c
Copy link
Member

Motivation for this change

Add functionality for convenient copying from VM to $out via the shared dir

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @flokli

Sorry, something went wrong.

@7c6f434c 7c6f434c mentioned this pull request Nov 24, 2019
Merged
10 tasks
@7c6f434c 7c6f434c requested a review from flokli November 24, 2019 20:25
@ofborg ofborg bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Nov 24, 2019
flokli
flokli reviewed Nov 24, 2019
View reviewed changes
nixos/lib/test-driver/test-driver.py Outdated
Comment on lines 531 to 533
"""Copy a file from the VM (specified by an in-VM source path) to $out
(with an optional subdirectory parameter). The file is copied via the
shared directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about

to a path relative to $out

via the shared_dir shared amongst all VMs.

nixos/lib/test-driver/test-driver.py Outdated
int_shared_dir = pathlib.Path("/tmp/xchg")
with tempfile.TemporaryDirectory(dir=self.shared_dir) as shared_td:
shared_temp = pathlib.Path(shared_td)
shared_temp_in = int_shared_dir / shared_temp.name
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you inline int_shared_dir into here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefix paths valid inside the VM context with vm_.

nixos/lib/test-driver/test-driver.py Outdated
shared_temp_in = int_shared_dir / shared_temp.name
src_in = shared_temp_in / src_orig.name
src_out = shared_temp / src_orig.name
tgt = out_dir / target_dir
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the absolute path of target_dir. I'd suggest to inline that or at least move if immediately before the actual copy action.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 24, 2019
@worldofpeace
Copy link
Contributor

✨ Was just wanting a method like this.

@7c6f434c 7c6f434c force-pushed the add-test-driver-py-copy-from-vm branch from 277eedf to 3e15a63 Compare November 24, 2019 22:58
@7c6f434c
Copy link
Member Author

Added make_command function to auto-quote a lot of arguments before passing to VM; not sure if looking for or implementing advanced type-based approach is worth it (with type Literal which is not quoted and the result of quoting being Literal)

@7c6f434c 7c6f434c force-pushed the add-test-driver-py-copy-from-vm branch from 87db829 to ad38a08 Compare November 25, 2019 12:33
@7c6f434c
Copy link
Member Author

7c6f434c commented Dec 1, 2019

@flokli Any opinion on my updated version?

@flokli
Copy link
Contributor

flokli commented Dec 1, 2019

Sorry, forgot about that. LGTM.

@flokli flokli merged commit da3a320 into NixOS:master Dec 1, 2019
@srhb
Copy link
Contributor

srhb commented Jan 7, 2020

This seems to have broken sharing files between VMs -- at least it looks that way in the ceph tests. Are we supposed to use a different directory now from within the VMs?

@7c6f434c
Copy link
Member Author

7c6f434c commented Jan 8, 2020

Hm, I think I saw it used as an external path to shared directory (which predictably did not work). So it should be split into two paths then?

@srhb
Copy link
Contributor

srhb commented Jan 8, 2020

I'm actually not sure. It feels like there should be one shared directory on the host that all VMs are able to access. I don't think we have to worry about compatibility, so whether we make this directory distinct from the one used by copy probably doesn't matter.

@7c6f434c
Copy link
Member Author

7c6f434c commented Jan 8, 2020

Well, when I was writing the copy code — I cannot exclude something has changed since then — I saw that /tmp/shared is not the (external) path used for the shared directory. I guess the problem is with using the variable with the external path as internal path?

@srhb
Copy link
Contributor

srhb commented Jan 8, 2020

Not sure I understood that, but, yes it's a problem if the variable now points to the vm-specific dirs on the host-side, whereas previously it was pointing into one directory on the host side for all VMs.

@7c6f434c
Copy link
Member Author

7c6f434c commented Jan 8, 2020

When I was changing it, the variable was pointing to a directory that did not exist during the test (host-side)

@flokli flokli mentioned this pull request Jan 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flokli flokli

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@7c6f434c @worldofpeace @flokli @srhb