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-init: 0.7.9 -> 20.2 (python3!) #95746

Merged
merged 5 commits into from Oct 15, 2020
Merged

cloud-init: 0.7.9 -> 20.2 (python3!) #95746

merged 5 commits into from Oct 15, 2020

Conversation

@Mic92
Copy link
Member

@Mic92 Mic92 commented Aug 18, 2020

Motivation for this change
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 nixpkgs-review --run "nixpkgs-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.
@Mic92 Mic92 requested a review from zimbatm Aug 18, 2020
@Mic92 Mic92 changed the title Cloud init cloud-init: 0.7.9 -> 20.2 (python3!) Aug 18, 2020

checkInputs = with pythonPackages; [ contextlib2 httpretty mock unittest2 ];
disabledTests = [
Copy link
Member Author

@Mic92 Mic92 Aug 18, 2020

Almost all tests are running!

};

patches = [ ./add-nixos-support.patch ];
patches = [ ./0001-add-nixos-support.patch ];
Copy link
Member Author

@Mic92 Mic92 Aug 18, 2020

I think about upstreaming this.

Copy link
Contributor

@flokli flokli Oct 14, 2020

I think we should - the patch is generic enough for it.

@Mic92 Mic92 requested a review from flokli Aug 18, 2020
Copy link
Member

@zimbatm zimbatm left a comment

nice! This should be included in 20.09 if possible.

@@ -72,7 +95,7 @@ diff -ruN cloud-init-0.7.6.orig/cloudinit/distros/nixos.py cloud-init-0.7.6/clou
+ if not conf:
+ conf = HostnameConf('')
+ conf.set_hostname(your_hostname)
+ util.write_file(out_fn, str(conf), 0644)
Copy link
Member

@zimbatm zimbatm Aug 18, 2020

is 0644 the default mode if left unspecified?

Copy link
Member Author

@Mic92 Mic92 Aug 18, 2020

It will use the permission of the old file, which is .r--r--r-- 14 root 1 Jan 1970  /nix/store/g1mkk8lygngbzvm632wbmnm8xxf9dymz-etc-hostname

Copy link
Member Author

@Mic92 Mic92 Aug 18, 2020

Same here. The actual line you are interested in is below.

@@ -52,6 +73,8 @@ diff -ruN cloud-init-0.7.6.orig/cloudinit/distros/nixos.py cloud-init-0.7.6/clou
+ # calls from repeatly happening (when they
+ # should only happen say once per instance...)
+ self._runner = helpers.Runners(paths)
+ self.usr_lib_exec = os.path.join(os.path.dirname(__file__),
+ "../../../../../libexec")
Copy link
Member

@zimbatm zimbatm Aug 18, 2020

where does that point to, the current-system libexec?

Copy link
Member Author

@Mic92 Mic92 Aug 18, 2020

/usr/lib if not overwritten. Now it points to the $out/libexec.

owner = "canonical";
repo = "cloud-init";
rev = version;
sha256 = "sha256-QeY/fdIIUSsp5oNxyRtZwpTB747Jf5KAJuYY9yiKUvc=";
Copy link
Member

@zimbatm zimbatm Aug 18, 2020

are SRI hashes allowed now?

Copy link
Member Author

@Mic92 Mic92 Aug 18, 2020

Yes. nix1 was removed a while ago. We have a few of them in nixpkgs already. @edolstra or somebody else gave green light in the past. I don't remember which thread though.

@Mic92
Copy link
Member Author

@Mic92 Mic92 commented Aug 18, 2020

@zimbatm could you give this a shot on ec2 or gcp. Or can you refer to someone else who can test it?

@zimbatm
Copy link
Member

@zimbatm zimbatm commented Aug 18, 2020

maybe @rbvermaa ?

@flokli
Copy link
Contributor

@flokli flokli commented Sep 27, 2020

@Mic92 I just stubled over some old work from me in January on this (which I never pushed, boo).

I just rebased and pushed it to https://github.com/flokli/nixpkgs/commits/cloud-init.

At least flokli@b19bcea seems to be a good addition for more test coverage, and maybe the hostnamectl patch as well, if applicable to the more recent 20.2 version (given NixOS now support setting transient ones).

@das-g das-g mentioned this pull request Sep 27, 2020
10 tasks
@Mic92
Copy link
Member Author

@Mic92 Mic92 commented Sep 28, 2020

@flokli feel free to push it on top. I won't get back to until 9th october.

@flokli
Copy link
Contributor

@flokli flokli commented Oct 11, 2020

I was also quite busy spending time away from computers 😆

Hope you're fine - let me know if you want to pick this up. I currently don't really have any strong ties with cloud-init, but felt like it'd be a waste to just ignore these commits.

flokli added 3 commits Oct 14, 2020
required rebasing the patch, disabling some tests.

I also changed the hash to be in conventional format - the

hash mismatch in fixed-output derivation '/nix/store/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx-source':
  wanted: sha256:yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
  got:    sha256:zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz

doesn't propose SRI syntax.
@flokli
Copy link
Contributor

@flokli flokli commented Oct 14, 2020

Pushed my commits on top.

I opted against changing the cloud-init patch to use hostnamectl, because:

  • This would require dropping https://github.com/NixOS/nixpkgs/blob/master/pkgs/os-specific/linux/systemd/default.nix#L45. Currently hostnamectl set-hostname… throws a NixOS-specific error message.
  • Even when I drop this patch, it only seemed to affect the kernel hostname retrieved via uname -n, not the output of the hostname command. I'm not sure if this is intended behaviour, but just changing the hostname like it was in the patch did work, so fine for me.

I also added a test checking the hostname change being successful, and bumped to 20.3 (which required some more tests to be disabled), PTAL.

Copy link
Member

@zimbatm zimbatm left a comment

LGTM

@flokli flokli merged commit 9d0d99f into NixOS:master Oct 15, 2020
18 of 19 checks passed
@Mic92 Mic92 deleted the cloud-init branch Oct 20, 2020
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

3 participants