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.

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

Add test driver py copy from vm #74077

Merged
merged 2 commits into from Dec 1, 2019

Conversation

@7c6f434c
Copy link
Member

7c6f434c commented Nov 24, 2019

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

@7c6f434c 7c6f434c mentioned this pull request Nov 24, 2019
3 of 10 tasks complete
@7c6f434c 7c6f434c requested a review from flokli Nov 24, 2019
@ofborg ofborg bot added the 6.topic: nixos label Nov 24, 2019
"""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.
Comment on lines 531 to 533

This comment has been minimized.

Copy link
@flokli

flokli Nov 24, 2019

Contributor

What about

to a path relative to $out

via the shared_dir shared amongst all VMs.

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

This comment has been minimized.

Copy link
@flokli

flokli Nov 24, 2019

Contributor

Can you inline int_shared_dir into here?

This comment has been minimized.

Copy link
@flokli

flokli Nov 24, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@flokli

flokli Nov 24, 2019

Contributor

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.

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Nov 24, 2019

Was just wanting a method like this.

@7c6f434c 7c6f434c force-pushed the 7c6f434c:add-test-driver-py-copy-from-vm branch from 277eedf to 3e15a63 Nov 24, 2019
@7c6f434c

This comment has been minimized.

Copy link
Member Author

7c6f434c commented Nov 25, 2019

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 7c6f434c:add-test-driver-py-copy-from-vm branch from 87db829 to ad38a08 Nov 25, 2019
@7c6f434c

This comment has been minimized.

Copy link
Member Author

7c6f434c commented Dec 1, 2019

@flokli Any opinion on my updated version?

@flokli

This comment has been minimized.

Copy link
Contributor

flokli commented Dec 1, 2019

Sorry, forgot about that. LGTM.

@flokli flokli merged commit da3a320 into NixOS:master Dec 1, 2019
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@srhb

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.