Open
Conversation
jfroche
force-pushed
the
system-manager-base
branch
from
4d4ff65 to
293c344
Compare
This comment has been minimized.
This comment has been minimized.
jfroche
force-pushed
the
system-manager-base
branch
3 times, most recently
from
e9e7d85 to
ee5fe34
Compare
picnoir
force-pushed
the
system-manager-base
branch
from
6e9dd1f to
ee5fe34
Compare
samrose
force-pushed
the
system-manager-base
branch
from
fa57ba5 to
9fb8bdf
Compare
* Create default system manager configuration * chores: add nix run .#check-system-module to github actions workflows * feat: replace Docker-based system-manager tests with container test framework Switch from building Docker images and running pytest+testinfra externally to using system-manager's built-in makeContainerTest API backed by systemd-nspawn. The test is now a Nix check derivation that runs inside the build sandbox. It requires auto-allocating UIDs in the ephemeral Nix installation, which is now enabled by default in the GitHub Action. Rebased from #2010 Co-authored-by: Yvan Sraka <yvan@sraka.xyz>
system-manager's userborn service rewrites /etc/passwd entries: - root shell: /bin/bash -> /run/system-manager/sw/bin/bash - nobody shell: /usr/sbin/nologin -> /run/system-manager/sw/bin/nologin
The check should be part of the regular nix-build workflow
Enabling the nginx service in the system configuration was a good start, but it had implications for the test suite verifying that the AMI was correctly configured. We change the configuration to set up a basic ssh config file that matches the expected configuration for the AMI, and update the tests to verify that the file is created with the correct content and permissions.
samrose
force-pushed
the
system-manager-base
branch
from
f6e2bb4 to
3788804
Compare
PostgreSQL Extension Dependency Analysis: PR #2063
SummaryNo extensions had dependencies with MAJOR version updates. Full Analysis ResultsPostgreSQL 15 Extension DependenciesPostgreSQL 17 Extension DependenciesOrioleDB 17 Extension Dependencies |
PostgreSQL Package Dependency Analysis: PR #2063
SummaryNo packages had MAJOR version updates. Full Analysis ResultsPostgreSQL 15 Dependency ChangesExtracting PostgreSQL 15 dependencies...
Runtime Closure Size
Raw Dependency ClosurePostgreSQL 17 Dependency ChangesExtracting PostgreSQL 17 dependencies...
Runtime Closure Size
Raw Dependency Closure |
Collaborator
|
Need to re-run supadev testing, and then we're ready to review |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces system-manager as a declarative system configuration layer for non-NixOS Linux builds, and replaces the existing Docker/testinfra approach with system-manager’s Nix-native container test check to validate activation inside a systemd-nspawn container.
Changes:
- Add a flake
systemConfigsoutput plus a genesis module and Linux-onlysystem-managerpackage export. - Add a Linux-only
check-system-managerNix check usingsystem-manager.lib.containerTest.makeContainerTest. - Wire system-manager deployment into the Ansible AMI playbook, update docs/navigation, and enable auto UID allocation in the CI Nix installer action.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
nix/systemModules/tests/default.nix |
Adds a Linux-only check-system-manager container test check. |
nix/systemModules/default.nix |
Introduces the system module registry and a genesis placeholder module. |
nix/systemConfigs.nix |
Defines per-architecture system-manager system configs exported as flake outputs. |
nix/packages/default.nix |
Re-exports system-manager as a Linux-only flake package. |
nix/mkdocs.yml |
Adds system-manager docs page to mkdocs nav. |
nix/docs/system-manager.md |
Documents system-manager integration, activation, and testing. |
nix/docs/nix-directory-structure.md |
Documents new Nix directory entries for system-manager modules/configs. |
nix/docs/README.md |
Links to the new system-manager documentation. |
flake.nix |
Adds the system-manager flake input and imports system-manager outputs. |
flake.lock |
Locks the new system-manager input and its transitive dependencies. |
audit-specs/baselines/ami-build/user.yml |
Updates expected shells for root and nobody to system-manager paths. |
ansible/vars.yml |
Updates postgres_release version strings (currently with suffixes). |
ansible/tasks/setup-system-manager.yml |
Adds tasks to install and activate system-manager config from the remote flake. |
ansible/tasks/setup-nix.yml |
Adds a Nix installation task file (currently not wired into a playbook). |
ansible/playbook.yml |
Adds a playbook step to deploy system-manager. |
.github/actions/nix-install-ephemeral/action.yml |
Enables auto-allocate-uids/cgroups experimental features for CI runners. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -10,9 +10,9 @@ postgres_major: | |||
|
|
|||
| # Full version strings for each major version | |||
| postgres_release: | |||
There was a problem hiding this comment.
postgres_release values now include suffixes (e.g. -sysmg-2 and -orioledb-sysmg-2). This will fail the merge requirements check in .github/workflows/ci.yml which rejects any suffix after the semver (and any text after -orioledb). Please remove these suffixes or move this metadata to a different variable that CI does not validate.
Suggested change
| postgres_release: | |
| postgres_release: | |
| postgresorioledb-17: "17.6.0.053-orioledb" | |
| postgres17: "17.6.1.096" | |
| postgres15: "15.14.1.096" | |
| postgres_release_full: |
| ignore_errors: true | ||
|
|
||
| - name: Install nix | ||
| ansible.builtin.shell: curl --proto '=https' --tlsv1.2 -sSf -L https://artifacts.nixos.org/experimental-installer | sh -s -- install --no-confirm --extra-conf 'substituters = https://cache.nixos.org https://nix-postgres-artifacts.s3.amazonaws.com' --extra-conf 'trusted-public-keys = nix-postgres-artifacts:dGZlQOvKcNEjvT7QEAJbcV6b6uk7VF/hWMjhYleiaLI=% cache.nixos.org-1:6NCHdD59X431o0gWypbMrAURkbJ16ZPMQFGspcDShjY=' |
There was a problem hiding this comment.
The trusted-public-keys value contains an extra % character (...YleiaLI=% ...), which does not match the key used elsewhere in the repo and will likely break Nix’s signature verification / nix.conf parsing. Please remove the % so the key remains exactly ...YleiaLI=.
Suggested change
| ansible.builtin.shell: curl --proto '=https' --tlsv1.2 -sSf -L https://artifacts.nixos.org/experimental-installer | sh -s -- install --no-confirm --extra-conf 'substituters = https://cache.nixos.org https://nix-postgres-artifacts.s3.amazonaws.com' --extra-conf 'trusted-public-keys = nix-postgres-artifacts:dGZlQOvKcNEjvT7QEAJbcV6b6uk7VF/hWMjhYleiaLI=% cache.nixos.org-1:6NCHdD59X431o0gWypbMrAURkbJ16ZPMQFGspcDShjY=' | |
| ansible.builtin.shell: curl --proto '=https' --tlsv1.2 -sSf -L https://artifacts.nixos.org/experimental-installer | sh -s -- install --no-confirm --extra-conf 'substituters = https://cache.nixos.org https://nix-postgres-artifacts.s3.amazonaws.com' --extra-conf 'trusted-public-keys = nix-postgres-artifacts:dGZlQOvKcNEjvT7QEAJbcV6b6uk7VF/hWMjhYleiaLI= cache.nixos.org-1:6NCHdD59X431o0gWypbMrAURkbJ16ZPMQFGspcDShjY=' |
Comment on lines
+2
to
+11
| - name: Check if nix is installed | ||
| ansible.builtin.command: which nix | ||
| register: nix_installed | ||
| failed_when: nix_installed.rc != 0 | ||
| ignore_errors: true | ||
|
|
||
| - name: Install nix | ||
| ansible.builtin.shell: curl --proto '=https' --tlsv1.2 -sSf -L https://artifacts.nixos.org/experimental-installer | sh -s -- install --no-confirm --extra-conf 'substituters = https://cache.nixos.org https://nix-postgres-artifacts.s3.amazonaws.com' --extra-conf 'trusted-public-keys = nix-postgres-artifacts:dGZlQOvKcNEjvT7QEAJbcV6b6uk7VF/hWMjhYleiaLI=% cache.nixos.org-1:6NCHdD59X431o0gWypbMrAURkbJ16ZPMQFGspcDShjY=' | ||
| when: nix_installed.rc != 0 | ||
| become: true |
There was a problem hiding this comment.
This new setup-nix.yml task file is not referenced by any playbook/import in the repo (no import_tasks: tasks/setup-nix.yml), so it will never run and may drift out of sync. Either wire it into the appropriate playbook/stage or remove it to avoid dead code.
Suggested change
| - name: Check if nix is installed | |
| ansible.builtin.command: which nix | |
| register: nix_installed | |
| failed_when: nix_installed.rc != 0 | |
| ignore_errors: true | |
| - name: Install nix | |
| ansible.builtin.shell: curl --proto '=https' --tlsv1.2 -sSf -L https://artifacts.nixos.org/experimental-installer | sh -s -- install --no-confirm --extra-conf 'substituters = https://cache.nixos.org https://nix-postgres-artifacts.s3.amazonaws.com' --extra-conf 'trusted-public-keys = nix-postgres-artifacts:dGZlQOvKcNEjvT7QEAJbcV6b6uk7VF/hWMjhYleiaLI=% cache.nixos.org-1:6NCHdD59X431o0gWypbMrAURkbJ16ZPMQFGspcDShjY=' | |
| when: nix_installed.rc != 0 | |
| become: true | |
| # This file previously contained Nix setup tasks but was never referenced | |
| # by any playbook/import, so the tasks were dead code and could drift out | |
| # of sync. It has been intentionally left empty to avoid unused tasks. |
Comment on lines
+12
to
+13
| system-manager register --store-path "$STORE_PATH" --sudo | ||
| system-manager activate --store-path "$STORE_PATH" --sudo |
There was a problem hiding this comment.
These tasks run with become: true (root) but pass --sudo to system-manager. Per system-manager docs, --sudo causes it to invoke the sudo binary and is unnecessary (and can fail if sudo is unavailable) when already running as root. Please drop --sudo and rely on Ansible privilege escalation.
Suggested change
| system-manager register --store-path "$STORE_PATH" --sudo | |
| system-manager activate --store-path "$STORE_PATH" --sudo | |
| system-manager register --store-path "$STORE_PATH" | |
| system-manager activate --store-path "$STORE_PATH" |
| - name: Install system-manager from binary cache | ||
| ansible.builtin.shell: | | ||
| . /nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh | ||
| nix profile add --accept-flake-config "github:supabase/postgres/{{ git_commit_sha }}#system-manager" |
There was a problem hiding this comment.
Elsewhere in the Ansible codebase we use nix profile install ... rather than nix profile add .... While add works on newer Nix, install is the more widely supported alias and keeps this consistent with the rest of the repo’s tasks.
Suggested change
| nix profile add --accept-flake-config "github:supabase/postgres/{{ git_commit_sha }}#system-manager" | |
| nix profile install --accept-flake-config "github:supabase/postgres/{{ git_commit_sha }}#system-manager" |
Comment on lines
+28
to
+31
| assert machine.file("/etc/system-manager-genesis").exists, "/etc/system-manager-genesis should exist" | ||
| assert machine.file("/etc/system-manager-genesis").mode == 0o644, "/etc/system-manager-genesis should have mode 0644" | ||
| assert machine.file("/etc/system-manager-genesis").user == "root", "/etc/system-manager-genesis should be owned by root" | ||
| assert machine.file("/etc/system-manager-genesis").group == "root", "/etc/system-manager-genesis should be owned by root" |
There was a problem hiding this comment.
machine.file(...) is not part of the NixOS-style Python test driver API used by makeContainerTest, so these assertions are likely to fail at runtime. Please switch to supported primitives like machine.wait_for_file(...) and machine.succeed("stat ...") (and parse/compare mode/owner/group) for the genesis file checks.
Suggested change
| assert machine.file("/etc/system-manager-genesis").exists, "/etc/system-manager-genesis should exist" | |
| assert machine.file("/etc/system-manager-genesis").mode == 0o644, "/etc/system-manager-genesis should have mode 0644" | |
| assert machine.file("/etc/system-manager-genesis").user == "root", "/etc/system-manager-genesis should be owned by root" | |
| assert machine.file("/etc/system-manager-genesis").group == "root", "/etc/system-manager-genesis should be owned by root" | |
| machine.wait_for_file("/etc/system-manager-genesis") | |
| mode, user, group = machine.succeed("stat -c '%a %U %G' /etc/system-manager-genesis").strip().split() | |
| assert mode == "644", "/etc/system-manager-genesis should have mode 0644" | |
| assert user == "root", "/etc/system-manager-genesis should be owned by root" | |
| assert group == "root", "/etc/system-manager-genesis should be owned by root" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Create default system manager configuration
chores: add nix run .#check-system-module to github actions workflows
feat: replace Docker-based system-manager tests with container test framework
Switch from building Docker images and running pytest+testinfra externally to using system-manager's built-in makeContainerTest API backed by systemd-nspawn. The test is now a Nix check derivation that runs inside the build sandbox.
It requires auto-allocating UIDs in the ephemeral Nix installation, which is now enabled by default in the GitHub Action.
Splited from #1882
Rebased from #2010
Testing: The test uses system-manager's makeContainerTest API which spins up a lightweight systemd-nspawn container (not a full VM). Inside the container it waits for the system to reach multi-user
target, then activates the system-manager configuration and waits for system-manager.target to come up. Once active, it verifies that the /etc/system-manager-genesis file was created by
checking that it exists, has mode 0644, and is owned by root:root. This proves the full system-manager pipeline works end to end, from Nix module definition through to file deployment inside a
running system.