nixos/google-compute-config: fix module #144761

picnoir · 2021-11-05T16:22:22Z

Motivation for this change

We were relying on some google-provided python script to perform various actions on the GCE guests. These scripts have been decommissioned in favor of a Golang daemon: https://github.com/GoogleCloudPlatform/guest-agent. The #131761 bump broke the google-compute-config NixOS module: the Python script are not longer present in the $out/bin directory (more info about that in the upstream commit), hence breaking the NixOS module that was relying on those scripts.

Migrating to the new Golang daemon to perform what the scripts used to do. See more details in the individual commit messages.

This module is tightly coupled to GCE, it's sadly hard to write any VM test for it and prevent future similar breakages :( . We tested this PR manually on a GCE test deployment containing some oslogin and metadata scripts.

 google-guest-agent: init at 20211019.00

The guest agent is depending in some bash scripts contained in the
google-compute-guest-configs packages to setup the virtio multiqueue
support at runtime. We inject those through a wrapper.

The oslogin implementation of the guest agent won't work on NixOS as
it currently is. On top of that, it's enabled/disabled status
exclusively depends on the remote GCE deployment metadata, we can't
disable that through the conf file. We add a patch allowing us to
disable this feature via the conf file.

 nixos/google-compute-config: use the gce guest-agent

The python scripts we're using have been deprecated in favor of the
guest agent, see more infos at:
GoogleCloudPlatform/compute-image-packages@276e520#diff-267a2788071ee63df1443363c2ab882e7d321adc77ee53462a920229a962eabbL40

The guest agent onboard all the old python scripts as part of two new
go binaries: google_guest_agent and google_metadata_script_runner.
Both are using the same /etc/default-instance_configs.cfg
configuration file.

The configuration file we embed in this module mimicks the setup we
had with the python scripts. We'll have two services:

- The metadata script service: in charge of running the
  google compute startup/showdown scripts.
- The guest agent: in charge of syncing the guest clock with the host
  and setup the network interfaces on boot.

Things done

nixos/modules/virtualisation/google-compute-config.nix

pkgs/tools/virtualization/google-compute-guest-agent/default.nix

pkgs/tools/virtualization/google-compute-guest-configs/default.nix

The guest agent is depending in some bash scripts contained in the google-compute-guest-configs packages to setup the virtio multiqueue support at runtime. We inject those through a wrapper. The oslogin implementation of the guest agent won't work on NixOS as it currently is. On top of that, it's enabled/disabled status exclusively depends on the remote GCE deployment metadata, we can't disable that through the conf file. We add a patch allowing us to disable this feature via the conf file. Co-authored-by: Mark Karpov <markkarpov92@gmail.com>

The python scripts we're using have been deprecated in favor of the guest agent, see more infos at: GoogleCloudPlatform/compute-image-packages@276e520#diff-267a2788071ee63df1443363c2ab882e7d321adc77ee53462a920229a962eabbL40 The guest agent onboard all the old python scripts as part of two new go binaries: google_guest_agent and google_metadata_script_runner. Both are using the same /etc/default-instance_configs.cfg configuration file. The configuration file we embed in this module mimicks the setup we had with the python scripts. We'll have two services: - The metadata script service: in charge of running the google compute startup/showdown scripts. - The guest agent: in charge of syncing the guest clock with the host and setup the network interfaces on boot. Co-authored-by: Mark Karpov <markkarpov92@gmail.com>

pkgs/tools/virtualization/google-compute-guest-agent/default.nix

pkgs/tools/virtualization/google-compute-guest-configs/default.nix

picnoir · 2021-11-05T21:34:53Z

. This is the default recommended style which should be used in most cases

Could you please point me to this Nixpkgs "recommended style" guidelines?

I don't see any reason to not use it in this package.

https://discourse.nixos.org/t/rec-env-allocation/15926 . Now you do.

Quoting literal paths does nothing, so it can also be removed

Meaning I'd have to edit that file again and rebase the PR. Meaning spend time on stuff you admit yourself would not lead to any qualitative improvement.

I don't understand what you're trying to do here by nitpicking details. This PR makes some technical changes to this module, we did not even discussed yet whether or not this is the way to go for the google-compute-config NixOS module.

What's the point of nitpicking style details when the core of the PR has not been discussed yet? What are you trying to achieve here?

I think reading https://sage.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ might help.

picnoir · 2021-11-05T21:37:39Z

Oh, btw, for the record: the quoting part you're not happy with is actually not coming from me, it's already in Nixpkgs, in one of the derivation packaging the now partly legacy google-compute-engine :

nixpkgs/pkgs/tools/virtualization/google-compute-engine/default.nix

Line 49 in 0a9e903

cp -r "src/etc/sysctl.d" $out

flokli

Nice work! Thanks for doing that digging!

Especially considering how hard this is to test automatically, and to avoid introducing regressions without noticing, we should try to send this upstream.

...virtualization/google-compute-guest-agent/0001-oslogin-add-disable-toggle-in-conf-file.patch

flokli · 2021-11-06T11:52:39Z

...virtualization/google-compute-guest-agent/0001-oslogin-add-disable-toggle-in-conf-file.patch

+From f318f525149b06365e9f0c7f642f09f401c3e1c1 Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?F=C3=A9lix=20Baylac-Jacqu=C3=A9?= <felix@alternativebit.fr>
+Date: Fri, 5 Nov 2021 15:40:13 +0100
+Subject: [PATCH] oslogin: add disable toggle in conf file


Please reword this patch - we should send this upstream, so it should be reworded a bit (to explain this is about NixOS, which doesn't want to use the imperative install/uninstall bits) - and maybe add some more comments to their code, so it's clear why the config boolean is read.

In another context, upstream seems to have been open to accept patches, as long as it doesn't impair other users - so a config option disabling the "installation" phases in their daemon might be accepted.

@NinjaTrappeur are you fine with me trying to upstream this patch?

flokli · 2021-11-06T11:58:38Z

...virtualization/google-compute-guest-agent/0001-oslogin-add-disable-toggle-in-conf-file.patch

+@@ -76,7 +76,8 @@ func (o *osloginMgr) timeout() bool {
+ }
+
+ func (o *osloginMgr) disabled(os string) bool {


As explained above, needs to have a comment on why we disable this.

flokli · 2021-11-06T11:58:41Z

pkgs/tools/virtualization/google-compute-guest-agent/default.nix

+    sha256 = "sha256-DHpv6RIXV3Rn/8fuGUWS6nzsEqgyY5+zozsundX9ByM=";
+  };
+
+  patches = [ ./0001-oslogin-add-disable-toggle-in-conf-file.patch ];


should be sent upstream and fetchpatch'ed from there.

flokli · 2021-11-06T11:58:44Z

...virtualization/google-compute-guest-agent/0001-oslogin-add-disable-toggle-in-conf-file.patch

+@@ -76,7 +76,8 @@ func (o *osloginMgr) timeout() bool {
+ }
+
+ func (o *osloginMgr) disabled(os string) bool {


As explained above, needs to have a comment on why we disable this.

flokli · 2021-11-06T11:58:44Z

pkgs/tools/virtualization/google-compute-guest-agent/default.nix

+    sha256 = "sha256-DHpv6RIXV3Rn/8fuGUWS6nzsEqgyY5+zozsundX9ByM=";
+  };
+
+  patches = [ ./0001-oslogin-add-disable-toggle-in-conf-file.patch ];


should be sent upstream and fetchpatch'ed from there.

Artturin · 2021-12-01T15:16:40Z

should we downgrade the package for now?

PaulGrandperrin · 2021-12-01T20:02:11Z

I have a small question/suggestion related to this work:

On this line:

nixpkgs/nixos/modules/virtualisation/google-compute-image.nix

Line 10 in 69e4bae

, shouldn't it be google-compute-config.nix instead of google-compute-image.nix ?

I'm new to Nix, so maybe I don't understand something :)

PaulGrandperrin · 2021-12-01T20:05:14Z

Also, on GCE, I installed Nixos manually with a ZFS root FS. This prevents me from importing directly google-compute-config.nix as the root FS is hard-coded to be ext4.

Could there be a simple solution to this issue?

For now, I just copied the file in my /etc/nixos to adapt it (also to use systemd-boot with EFI).

picnoir · 2021-12-02T07:47:36Z

should we downgrade the package for now?

We have two options for a quick fix:

Downgrade google-compute-guest-configs. Pro: easy, Con: the downgrade could backfire and break some downstream deployments. Ping @cpcloud since you bumped that one.
Merge this PR as is. Pro: fixes the module, Con: procrastination being procrastination, the patch may end up never be sent upstream.

I'm unlikely to have the required bandwidth to properly push the patch upstream for the next 2 months.

flokli · 2021-12-02T12:57:46Z

Then let's downgrade. I don't want to discredit the work being done in this PR, but we shouldn't merge it in halfway-done - I'd then rather revert the bump to fix things (and backport it to 21.11, too).

cpcloud · 2021-12-02T13:04:58Z

Reverting is fine by me.

This reverts commit 1748291. This upgrade broke the google-compute-config module. See NixOS#144761 for more info.

This reverts commit 1748291. This upgrade broke the google-compute-config module. See #144761 for more info. (cherry picked from commit 5a4ea49)

abbradar · 2022-01-10T19:44:10Z

nixos/modules/virtualisation/google-compute-config.nix

+  environment.etc."sysctl.d/11-gce-network-security.conf".source = "${gce}/sysctl.d/11-gce-network-security.conf";
+
+  environment.etc."default/instance_configs.cfg".text = ''
+    [Accounts]


@NinjaTrappeur can you compare your configuration here to the one from my PR and tell which other options are a good idea to use? Or to paraphase: which parts of default configuration did you change?

picnoir · 2022-01-13T14:12:49Z

Closing in favor of #150837

picnoir requested a review from flokli November 5, 2021 16:22

github-actions bot added 6.topic: nixos 8.has: module (update) labels Nov 5, 2021

ofborg bot added the 8.has: package (new) label Nov 5, 2021

ofborg bot requested review from mrkkrp and kalbasit November 5, 2021 16:33

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Nov 5, 2021