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

Jump to bottom

lxd: add criu to path and bash completions, lxc: fix bash completions #64532

Merged
merged 3 commits into from Jul 13, 2019
Merged

lxd: add criu to path and bash completions, lxc: fix bash completions #64532

merged 3 commits into from Jul 13, 2019

Conversation

megheaiulian
Copy link
Contributor

@megheaiulian megheaiulian commented Jul 9, 2019 •
edited

Motivation for this change

Adds criu to the path of lxd so live migrations can be performed.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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.

@worldofpeace
Copy link
Contributor

Hi @megheaiulian.

Can you add the commits from #64533 and #64534 into this PR?
It would be more appropriate to group the cleanup in this way, and to prevent conflicts.

@worldofpeace
Copy link
Contributor

Also make sure your commits messages meet the contribution guidelines.

For example, the commit in this PR should be
lxd: add criu to path.

This was referenced Jul 11, 2019
Closed
Closed
@megheaiulian megheaiulian changed the title [lxd] add criu to path lxd: add criu to path and bash completions, lxc: fix bash completions Jul 11, 2019
@megheaiulian
Copy link
Contributor Author

@worldofpeace Thanks for the feedback. Please have another look.

@worldofpeace
Copy link
Contributor

Huh, didn't notice that there was two lxd changes and one lxc change (so similar) 😄

Alas it's still fine, taking a look now.

worldofpeace
worldofpeace suggested changes Jul 12, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Looks good to so far, haven't tested the completions

pkgs/os-specific/linux/lxc/default.nix Show resolved Hide resolved
@worldofpeace
Copy link
Contributor

@megheaiulian Where in the source code does lxd need criu?

@megheaiulian
Copy link
Contributor Author

Have a look at https://github.com/lxc/lxd/blob/f802fbbf7442c539ac758870cc8b702dd51708c6/lxd/migrate_container.go#L43. Without it lxd can't do a live migration (move the current container's memory) to another server.
By the way those renames in lxc will make it load completions on demand instead of having to source the completions always. See the Where should I put it to be sure that interactive bash shells will find it and source it in https://github.com/scop/bash-completion/blob/master/README.md#faq.

@worldofpeace
Copy link
Contributor

Makes sense then @megheaiulian, thanks for explaining.

This should be good for merging.

@worldofpeace worldofpeace merged commit 9a84689 into NixOS:master Jul 13, 2019
@megheaiulian megheaiulian deleted the feature/lxd-criu branch July 13, 2019 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@worldofpeace worldofpeace worldofpeace approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@megheaiulian @worldofpeace