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

cloud-utils: fix dependency on gnused in $PATH #15737

Closed

Conversation

martijnvermaat
Copy link
Contributor

Motivation for this change

Fixes #15736

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

The cloud-utils package was extracted from inline use in
amazon-grow-partition where it used gnused in the initrd, probably
for legacy reasons. Now that it can be used separately, it should use
just sed which is provided by the stdenv.

Also, amazon-grow-partition can now do away with the gnused alias.

The cloud-utils package was extracted from inline use in
`amazon-grow-partition` where it used `gnused` in the initrd, probably
for legacy reasons. Now that it can be used separately, it should use
just `sed` which is provided by the stdenv.

Also, `amazon-grow-partition` can now do away with the `gnused` alias.
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @edolstra and @domenkozar to be potential reviewers

@martijnvermaat
Copy link
Contributor Author

martijnvermaat commented May 26, 2016

Note that this assumes nothing in amazon-grow-partition depends on gnused. I haven't been able to check or test this.

@martijnvermaat martijnvermaat changed the title cloud-utils: fix dependence on gnused in $PATH cloud-utils: fix dependency on gnused in $PATH May 26, 2016
@domenkozar
Copy link
Member

@martijnvermaat looks good. If you can test this still works deploying to EC2, I'm happy to merge.

@martijnvermaat
Copy link
Contributor Author

@domenkozar Thanks. I haven't used this on EC2 myself actually, but would be happy to explore that. Might be a while, but I'll ping you when I got the chance.

@domenkozar
Copy link
Member

@martijnvermaat great. I'd like to check we don't break EC2 as it's very important. Otherwise I will run my tests for OpenStack to be sure this works here.

@martijnvermaat
Copy link
Contributor Author

@domenkozar Could you give me a hint on how best to test this PR? Just took my first baby steps with NixOS/NixOps on EC2. If you want to do it faster, pleas go ahead, but I'm willing to learn.

I now know how to deploy from a custom nixpkgs tree to EC2 using NixOps, but of course amazon-grow-partition is in de AMI. I spent a little time trying to create my own AMI using create-amis.sh and this guide, but got stuck uploading the image (and I think that guide is a bit outdated)...

Perhaps another (easier) way would be to just start an EC2 instance and install NixOS in it manually with nixos-install using amazon-image.nix.

I feel like you had an easier way in mind I have yet to learn about.

@domenkozar
Copy link
Member

@martijnvermaat you can pass -I . flag to nixops to specify search path to point in current directory. If you put your nixpkgs fork with this branch, nixops will use it to deploy the VM to EC2.

@martijnvermaat
Copy link
Contributor Author

@domenkozar yes, but amazon-grow-partition is not deployed from nixpkgs, it is in the AMI initrd, which has already booted at that point, right?

@domenkozar
Copy link
Member

@martijnvermaat indeed it would be easier to first deploy a NixOS machine and then resize it. But correcting create-amis.sh instructions would also be very nice. cc @rbvermaa

@martijnvermaat
Copy link
Contributor Author

Steps I took to try and test the auto resizing codepath with a minimal test.nix and test-ec2.nix:

  1. nixops create -I nixpkgs=/path/to/nixpkgs-master ./test.nix ./test-ec2.nix -d test-ec2
  2. nixops deploy -d test-ec2
  3. nixops stop -d test-ec2
  4. In the AWS console, snapshot the volume, create a 30G volume from it, detach the old and attach the new.
  5. nixops start -d test-ec2
  6. Observe that the /dev/xvda is 30G but the root partition on it is still 20G.

Am I wrong expecting the partition to grow on boot?

When I deploy the same network with deployment.ec2.ebsInitialRootDiskSize = 30, the root partition is actually 30G when it comes up.

Unrelated note: nixops deploy consistently fails for me on first run and finishes successfully instantly on the second try: log output

@domenkozar

@domenkozar
Copy link
Member

I'm moving over the weekend so I'll take a look next week. Sorry :)

@martijnvermaat
Copy link
Contributor Author

No problem!

@ttuegel
Copy link
Member

ttuegel commented Jun 24, 2016

@domenkozar Did you have a chance to look at this PR?

@domenkozar
Copy link
Member

@ttuegel not really. I don't use EC2 so for me it's a bit of work also to get it all going. Anyone willing to help?

@jokogr
Copy link
Contributor

jokogr commented Jul 16, 2016

I have created another PR (#17015) to update cloud-utils to 0.29, where I remove the old patch and substitute the proper awk and sed paths to the growpart script.

I have verified the execution of growpart at ~okeanos, a cloud service for the Greek Research and Academic Community, by explicitly installing the cloud-utils package.

@joachifm
Copy link
Contributor

@jokogr your PR supplants this one?

@jokogr
Copy link
Contributor

jokogr commented Jul 19, 2016

@joachifm I think so, yes.

This PR removes the symbolic link to gnused. I am unsure if it can find a sed version when growpart is running, I haven't tested it.

My PR updates the package and requires the gawk and gnused packages explicitly.

@jokogr jokogr mentioned this pull request Jul 20, 2016
7 tasks
@Mic92 Mic92 closed this Oct 22, 2016
@domenkozar
Copy link
Member

@Mic92 why is this closed?

@Mic92
Copy link
Member

Mic92 commented Oct 22, 2016

@domenkozar because @jokogr said this pull request was replaced by #17015.

@jokogr
Copy link
Contributor

jokogr commented Oct 23, 2016

@domenkozar sorry for interfering, feel free to re-open it, but in any way, I am updating #17015 this week, so hopefully this PR won't be needed.

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

Successfully merging this pull request may close these issues.

7 participants