Add privileged container and CI support for BTRFS system tests#1528
Add privileged container and CI support for BTRFS system tests#1528silverhadch wants to merge 5 commits intoshadow-maint:masterfrom
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
silverhadch
force-pushed
the
test-btrfs
branch
from
4f8c9c8 to
9d99982
Compare
silverhadch
force-pushed
the
test-btrfs
branch
from
9d99982 to
f4f29e5
Compare
silverhadch
force-pushed
the
test-btrfs
branch
5 times, most recently
from
82b0634 to
b2cdf7e
Compare
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
Thanks for putting this together. I'm really liking the direction of this proposal. I’ve been looking through it and wanted to touch on a few architectural points before we get into the the code review itself.
Regarding the environment, since we’re currently running with full privileges, I’m wondering if we could tighten the security surface by only granting the specific Linux capabilities (like CAP_SETUID or CAP_CHOWN) actually required for the test? It would be great to avoid full root access if we can help it.
I also think it would be worth decoupling the privileged and unprivileged container runs right from the start; by separating them, we could run them in parallel to keep the CI fast and prevent the unprivileged results from being overwritten by the second run.
Lastly, and most importantly, while the logic here is solid, it adds a fair amount of complexity to the test suite. I’m curious if you’ve identified other areas in the current shadow project that would actually require this kind of infrastructure? I fear this will increase the maintenance burden and having other test cases running in such an environment would justify for this complexity increase.
silverhadch
force-pushed
the
test-btrfs
branch
from
b2cdf7e to
d83f32c
Compare
|
@ikerexxe |
silverhadch
force-pushed
the
test-btrfs
branch
3 times, most recently
from
2fe5ea5 to
59d109b
Compare
silverhadch
force-pushed
the
test-btrfs
branch
2 times, most recently
from
493e3f8 to
e8d063d
Compare
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
With the current proposal, pytest.ini globally ignores tests/privileged/. This is safe, but it makes these tests "invisible" to developers running standard tests. They might not realize these tests exist or need to be run.
Rather than a global ignore in pytest.ini, have you considered using a pytest marker (e.g., @pytest.mark.privileged)?
This would allow developers to see that tests are being "skipped" rather than being totally hidden. It also makes the intent clearer:
- Normal run:
pytest -m "not privileged" - Privileged run:
pytest -m "privileged"
This keeps the "safety first" approach but makes the test suite's existence more obvious to new contributors.
silverhadch
force-pushed
the
test-btrfs
branch
4 times, most recently
from
ae2db65 to
8479d82
Compare
silverhadch
force-pushed
the
test-btrfs
branch
4 times, most recently
from
c5402af to
c6a6e15
Compare
silverhadch
force-pushed
the
test-btrfs
branch
from
c6a6e15 to
c77cb7d
Compare
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
I think this will take us a while, so please be patient until we reach a point where I am comfortable with your proposal
Introduce opt-in privileged container execution for CI and local runs. This enables filesystem-level tests (e.g. BTRFS, mounts) while keeping unprivileged execution as the default and safe path. Changes include: - Separate privileged and unprivileged builders - Conditional Ansible roles and inventories - Privileged test execution wiring - --privileged support in container-build.sh Signed-off-by: Hadi Chokr <[email protected]>
Document the distinction between unprivileged and privileged execution modes for containers and system tests. Add explicit warnings that privileged tests must never be run on the host system, as they may mount filesystems, create loop devices, or modify kernel-visible state. Provide clear guidance on when privileged tests are acceptable and how to execute them safely in disposable environments. Signed-off-by: Hadi Chokr <[email protected]>
silverhadch
force-pushed
the
test-btrfs
branch
3 times, most recently
from
4e13a1b to
d3f13b3
Compare
silverhadch
force-pushed
the
test-btrfs
branch
from
d3f13b3 to
41cb2cc
Compare
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
I have added a few more points for improvement
silverhadch
force-pushed
the
test-btrfs
branch
from
f71b2da to
e31f1e7
Compare
silverhadch
force-pushed
the
test-btrfs
branch
from
e31f1e7 to
1e33197
Compare
Extend the Python system test framework with a dedicated privileged topology and configuration, while keeping privileged tests excluded by default. And include a BTRFS Framework class for BTRFS Operations. This ensures: - Clear separation between safe and destructive tests - No accidental execution of privileged tests in normal CI or local runs - Explicit opt-in for privileged environments Signed-off-by: Hadi Chokr <[email protected]>
Add a privileged system test that verifies useradd can create home directories as BTRFS subvolumes. This test requires elevated privileges to use: The BTRFS filesystem Framework class to create a /home directory on BTRFS and check if the Users Home directory is a BTRFS Subvolume. The test is intentionally isolated and excluded from default test runs to prevent accidental execution on host systems. Signed-off-by: Hadi Chokr <[email protected]>
Signed-off-by: Hadi Chokr <[email protected]>
silverhadch
force-pushed
the
test-btrfs
branch
from
1e33197 to
3373b42
Compare
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
I will not insist with more changes to the btrfs.py, but taking a look at the automation I think it can be improved to reduce the codebase and help with future maintenance.
There's a code duplication of near-identical lines in the ansible roles, ansible playbooks and github action. Those files should be merged and I have left inline instructions to help you do so. I didn't test the changes myself, but they should work almost out of the box.
| containers.podman.podman_image: | ||
| name: '{{ image[distribution] }}' | ||
|
|
||
| - name: Create and start container |
There was a problem hiding this comment.
This can be "Create and start {{ 'privileged' if privileged | default(false) else 'unprivileged' }} container"
|
|
||
| - name: Create and start container | ||
| containers.podman.podman_container: | ||
| name: builder |
There was a problem hiding this comment.
And this can be "{{ container_name | default('builder') }}"
| name: builder | ||
| state: started | ||
| image: '{{ image[distribution] }}' | ||
| command: "sleep 1d" |
There was a problem hiding this comment.
Add an additional line after this one with privileged: "{{ privileged | default(false) }}"
|
|
||
| - name: Create repo | ||
| ansible.builtin.shell: | ||
| podman exec builder mkdir -p /usr/local/src |
There was a problem hiding this comment.
Substitute by podman exec {{ container_name | default('builder') }} mkdir -p /usr/local/src
|
|
||
| - name: Copy repo | ||
| ansible.builtin.shell: | ||
| podman cp ../../ builder:/usr/local/src/shadow |
There was a problem hiding this comment.
Substitute by podman cp ../../ {{ container_name | default('builder')}}:/usr/local/src/shadow
|
|
||
| # Run container builds requiring elevated privileges. | ||
| # Separated from unprivileged runs to make CI faster to debug and structurally clearer. | ||
| container-build-privileged: |
There was a problem hiding this comment.
container-build: should be enough here
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: [alpine, debian, fedora, opensuse] |
There was a problem hiding this comment.
Add another line after this one with privileged: [false, true]
| sudo apt-get -y install ansible | ||
|
|
||
| - name: Build container | ||
| - name: Build container (privileged) |
There was a problem hiding this comment.
Replace with "Build container (${{ matrix.privileged && 'privileged' || 'unprivileged'}})"
| sudo ansible-playbook playbook-privileged.yml \ | ||
| -i inventory.ini \ | ||
| -e "distribution=${{ matrix.os }}" |
There was a problem hiding this comment.
This should be:
${{ matrix.privileged && 'sudo' || '' }} ansible-playbook playbook.yml \
-i inventory.ini \
-e "distribution=${{ matrix.os }}" \
-e "privileged_mode=${{ matrix.privileged }}"
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: ${{ matrix.os }}-build | ||
| name: ${{ matrix.os }}-privileged-build |
There was a problem hiding this comment.
Replace with "${{ matrix.os }}-${{ matrix.privileged && 'privileged' || 'unprivileged'}}-build"
silverhadch
force-pushed
the
test-btrfs
branch
2 times, most recently
from
7b61595 to
3373b42
Compare
4 participants
This PR introduces opt-in privileged container support to enable filesystem-level
system tests (e.g. BTRFS subvolume homes) while keeping all existing CI and local
testing unprivileged by default.
Key changes:
Safety:
Privileged tests may mount filesystems, create loop devices, and modify kernel-visible
state. They must never be run on a host system. Execution is restricted to disposable
VMs or explicitly created privileged containers.
This keeps destructive tests clearly isolated while allowing coverage for features
that cannot be tested in unprivileged environments.
CC: @Conan-Kudo @ikerexxe
@ikerexxe wanted a Test Case for this Functionality, we would be relying on in Fedora and KDE Linux.
#1418 (review)