tests: properly build snapd snap #13517

valentindavid · 2024-01-24T15:22:43Z

Now we build also the test version of snapd snap in snap-builds workflow job. We copy this into the spread tests. And we use that snap, which we only instrument instead of copying the snapd deb build.

If the snap is not available, then we build it in spread. On CI, this happens on arm since the workflow does not build it. It will also happen when triggering test manually.

codecov-commenter · 2024-01-29T15:08:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.83%. Comparing base (ab996a0) to head (534fff4).
Report is 215 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13517      +/-   ##
==========================================
- Coverage   78.86%   78.83%   -0.03%     
==========================================
  Files        1043     1046       +3     
  Lines      134595   136020    +1425     
==========================================
+ Hits       106144   107237    +1093     
- Misses      21837    22083     +246     
- Partials     6614     6700      +86

Flag	Coverage Δ
unittests	`78.83% <100.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sergiocazzolato · 2024-06-03T12:58:43Z

.github/workflows/test.yaml

@@ -796,7 +833,7 @@ jobs:
        key: "${{ github.job }}-results-${{ github.run_id }}-${{ matrix.group }}-${{ github.run_attempt }}"

  spread-nested:
-    needs: [unit-tests]
+    needs: [unit-tests, snap-builds]


this job is changing in #14032, we should skip any change in spread-nested

sergiocazzolato

Hi, thanks for this change, some comments inline

sergiocazzolato · 2024-06-03T13:56:25Z

tests/lib/nested.sh

-        # sign the snapd snap with fakestore if requested
-        if [ "$NESTED_SIGN_SNAPS_FAKESTORE" = "true" ]; then
-            "$TESTSTOOLS"/store-state make-snap-installable --noack "$NESTED_FAKESTORE_BLOB_DIR" "$(nested_get_extra_snaps_path)/$output_name" "$snap_id"
+            for f in "${NESTED_ASSETS_DIR}"/snapd_*.snap; do


how many snapd are we going to have? 2?, perhaps we could do:
`snap_path="$(ls ${NESTED_ASSETS_DIR}"/snapd_*.snap | head -n1)"
if [ -z "$snap_path" ]; then
...

Only 1. There is a break.

Loops are what is recommended by shellcheck on posix scripts. But given that prepare.sh uses bash (it is in the shebang), maybe we should just enable extglob. And stop doing any of those weird hacks.

sergiocazzolato · 2024-06-03T14:04:10Z

tests/lib/prepare.sh

+build_snapd_snap() {
+    local TARGET
+    local snapd_snap_cache
+    TARGET="${1}"


TARGET=$1 works as well

I'd keep the quotes just in case, in general almost everything should be quoted unless there's clear reason to not do it eg. when one wants splitting to take place.

sergiocazzolato · 2024-06-03T14:10:58Z

tests/lib/prepare.sh

+    local snapd_snap_cache
+    TARGET="${1}"
+
+    snapd_snap_cache="${NESTED_WORK_DIR:-/tmp/work-dir}/snapd_snap"


I would prefer to avoid using NESTED_WORK_DIR here, perhaps we could accept a parameter with the work dir

TARGET=$1
WORK_DIR=$2
snapd_snap_cache="${WORK_DIR:-/tmp/work-dir}/snapd_snap"

Why accepting it as parameter? We need it to be the same across the tests. Otherwise there will be multiple cache.

sergiocazzolato · 2024-06-03T14:12:42Z

tests/lib/prepare.sh

+    snapd_snap_cache="${NESTED_WORK_DIR:-/tmp/work-dir}/snapd_snap"
+    [ -d "${snapd_snap_cache}" ] || mkdir -p "${snapd_snap_cache}"
+    for snap in "${snapd_snap_cache}"/snapd_*.snap; do
+        if ! [ -f "${snap}" ]; then


is it possible that $snap is not a file?

Yes. If if the glob does not expand.

sergiocazzolato · 2024-06-03T14:13:26Z

tests/lib/prepare.sh

-    # data/preseed.json is not included in the deb, use the latest
-    # version from source tree to replace the one in the re-packed snapd snap.
-    cp "$PROJECT_PATH/data/preseed.json" "$UNPACK_DIR"/usr/lib/snapd
+    snapd_snap_cache="${NESTED_WORK_DIR:-/tmp/work-dir}/snapd_snap_with_tweaks"


same about the WORK_DIR

sergiocazzolato · 2024-06-03T14:13:43Z

tests/lib/prepare.sh

+    snapd_snap_cache="${NESTED_WORK_DIR:-/tmp/work-dir}/snapd_snap_with_tweaks"
+    [ -d "${snapd_snap_cache}" ] || mkdir -p "${snapd_snap_cache}"
+    for snap in "${snapd_snap_cache}"/snapd_*.snap; do
+        if [ -f "${snap}" ]; then


is it possible that $snap is not a file?

Yes. Like the other place, if the glob does not expand, because there is no matching file.

sergiocazzolato · 2024-06-03T14:39:26Z

tests/lib/prepare.sh

-    # file in place. Its called usr.lib.snapd.snap-confine on 14.04 but
-    # usr.lib.snapd.snap-confine.real everywhere else
-    rm -f "$UNPACK_DIR"/etc/apparmor.d/*
+    if ! os.query is-pc-amd64 || ! cp "${PROJECT_PATH}/built-snap"/snapd_1337.*.snap.keep "/tmp/snapd_from_snapcraft.snap"; then


it is not clear why:
! cp "${PROJECT_PATH}/built-snap"/snapd_1337.*.snap.keep "/tmp/snapd_from_snapcraft.snap"

If running outside the github workflow, "${PROJECT_PATH}/built-snap" will not exist. cp will fail. And then we fallback to building the snap.

I'd rather have an explicit environment variable controlling whether the build through snapcraft is (dis)allowed, otherwise there's a chance that everything will seem to be set up correctly, but the prepare will keep building snapd with snapcraft. , eg TESTS_USE_PREBUILT_SNAPD_SNAP: "$(HOST: echo PREBUILT_SNAPD)" in spread.yaml and then fail hard here?

That makes total sense.

tests/lib/prepare.sh

tests/nested/manual/cloud-init-nocloud-not-vuln/task.yaml

sergiocazzolato · 2024-06-03T14:48:36Z

tests/nested/manual/core20-boot-config-update/task.yaml

@@ -21,9 +21,11 @@ prepare: |
      exit
  fi

-  "$TESTSTOOLS"/snaps-state repack-snapd-deb-into-snap snapd
+  # shellcheck source=tests/lib/prepare.sh


I think in a following pr we should move out all the build/repack stuff from prepare.sh
I'll work on that once this is merged

bboozzoo · 2024-06-17T10:08:58Z

.github/workflows/test.yaml

+              echo "Checking ${snap}"
+              rm -rf squashfs-root
+              unsquashfs "${snap}" meta/snap.yaml usr/lib/snapd/
+              if cat squashfs-root/meta/snap.yaml | grep -q "version:.*dirty.*"; then


it's probably ok to just grep both files

Suggested change

if cat squashfs-root/meta/snap.yaml | grep -q "version:.*dirty.*"; then

if grep -q "version:.*dirty.*" squashfs-root/meta/snap.yaml squashfs-root/usr/lib/snapd/info; then

bboozzoo · 2024-06-17T10:19:30Z

tests/lib/nested.sh

+        if [ "${NESTED_FORCE_SNAKEOIL_KEYS:-false}" = "true" ]; then
+            OVMF_VARS="snakeoil"
+        fi


That was probably not needed anymore.

bboozzoo · 2024-06-17T10:32:41Z

tests/lib/prepare.sh

-    # file in place. Its called usr.lib.snapd.snap-confine on 14.04 but
-    # usr.lib.snapd.snap-confine.real everywhere else
-    rm -f "$UNPACK_DIR"/etc/apparmor.d/*
+    if ! os.query is-pc-amd64 || ! cp "${PROJECT_PATH}/built-snap"/snapd_1337.*.snap.keep "/tmp/snapd_from_snapcraft.snap"; then


I'd rather have an explicit environment variable controlling whether the build through snapcraft is (dis)allowed, otherwise there's a chance that everything will seem to be set up correctly, but the prepare will keep building snapd with snapcraft. , eg TESTS_USE_PREBUILT_SNAPD_SNAP: "$(HOST: echo PREBUILT_SNAPD)" in spread.yaml and then fail hard here?

bboozzoo · 2024-06-17T10:48:51Z

tests/lib/prepare.sh

+    mkdir -p "${snapd_snap_cache}"
+    for snap in "${snapd_snap_cache}"/snapd_*.snap; do
+        if ! [ -f "${snap}" ]; then
+            if os.query is-pc-amd64 && cp "${PROJECT_PATH}/built-snap"/snapd_1337.*.snap.keep "${snapd_snap_cache}/snapd_from_ci.snap"; then


see my comment in build_snapd_snap_with_run_mode_firstboot_tweaks

bboozzoo

LGTM with tiny fixes

bboozzoo · 2024-06-19T07:36:17Z

.github/workflows/test.yaml

@@ -592,7 +621,7 @@ jobs:
        verbose: true

  spread:
-    needs: [unit-tests]
+    needs: [unit-tests, snap-builds]


we may want to revisit this later, there'd be 4 snap builds to complete now before spread tests even start

since I don't think github supports adding needs on matrix defined jobs, perhaps we can split the snap builds into separate jobs and share some code through composite action, such that we can then organize it all as:

snap-build - regular snapd snap build for anyone to test locally

snap-build-test - which spread jobs could wait on

snap-build-fips - for local testing

snap-build-test-fips - which eg. FIPS spread jobs could wait on

in such setup only snap-build*-test would be needed for blocking the spread test jobs

just to be clear, this would be a followup once we see that the current setup is an issue and FIPS testing is added

bboozzoo · 2024-06-19T10:26:45Z

tests/lib/prepare.sh

+build_snapd_snap() {
+    local TARGET
+    local snapd_snap_cache
+    TARGET="${1}"


I'd keep the quotes just in case, in general almost everything should be quoted unless there's clear reason to not do it eg. when one wants splitting to take place.

bboozzoo · 2024-06-19T10:29:03Z

tests/main/preseed-core20/task.yaml

@@ -18,8 +18,8 @@ environment:
 prepare: |
  #shellcheck source=tests/lib/prepare.sh
  . "$TESTSLIB"/prepare.sh
-  snap download "--channel=${SNAPD_CHANNEL}" snapd
-  repack_snapd_snap_with_deb_content_and_run_mode_firstboot_tweaks /tmp
+  [ -d /tmp/teaked-snapd-snap ] || mkdir -p /tmp/teaked-snapd-snap


s/teaked/tweaked/ ?

bboozzoo · 2024-06-19T10:29:40Z

tests/main/preseed-core20/task.yaml

@@ -18,8 +18,8 @@ environment:
 prepare: |
  #shellcheck source=tests/lib/prepare.sh
  . "$TESTSLIB"/prepare.sh
-  snap download "--channel=${SNAPD_CHANNEL}" snapd
-  repack_snapd_snap_with_deb_content_and_run_mode_firstboot_tweaks /tmp
+  [ -d /tmp/teaked-snapd-snap ] || mkdir -p /tmp/teaked-snapd-snap


restore should probably rm -rf this directory

bboozzoo · 2024-06-19T10:56:01Z

tests/main/preseed-core20/task.yaml

@@ -47,7 +47,13 @@ execute: |
  export SNAPPY_FORCE_SAS_URL=http://$STORE_ADDR

  echo "Running pre-seeding"
-  SNAPD_SNAP=$(ls /tmp/snapd*.snap)
+  for f in /tmp/teaked-snapd-snap/snapd*.snap; do


sergiocazzolato · 2024-06-25T16:16:21Z

.github/workflows/test.yaml


    - name: Uploading snapd snap artifact
      uses: actions/upload-artifact@v3
      with:
-        name: snap-files-${{ matrix.toolchain }}
+        name: snap-files-${{ matrix.toolchain }}-${{ matrix.version }}


is it possible to add a comment somewhere to explain what are those snap artifacts used for?

something like Maciej did below

sergiocazzolato

Thanks. Please add an explanation explaining the different snapd snaps created.

Now we build also the test version of snapd snap in `snap-builds` workflow job. We copy this into the spread tests. And we use that snap, which we only instrument instead of copying the snapd deb build. If the snap is not available, then we build it in spread. On CI, this happens on arm since the workflow does not build it. It will also happen when triggering test manually.

…fresh

github-actions bot added the Run Nested -auto- Label automatically added in case nested tests need to be executed label Jan 24, 2024

valentindavid changed the title ~~PoC snapd snap spread tests~~ many: poc snapd snap spread tests Jan 24, 2024

valentindavid changed the title ~~many: poc snapd snap spread tests~~ tests: poc snapd snap spread tests Jan 24, 2024

valentindavid force-pushed the valentindavid/with-patchelf-run-tests branch 4 times, most recently from fbb31b8 to 1482ddf Compare January 31, 2024 12:38

valentindavid force-pushed the valentindavid/with-patchelf-run-tests branch 11 times, most recently from 8c2edc6 to 9a469fe Compare February 15, 2024 15:51

valentindavid force-pushed the valentindavid/with-patchelf-run-tests branch 8 times, most recently from 9cecdfc to 2500bd5 Compare February 21, 2024 09:45

valentindavid mentioned this pull request Feb 21, 2024

snapcraft.yaml: build on 22.04 #13370

Merged

valentindavid force-pushed the valentindavid/with-patchelf-run-tests branch 2 times, most recently from d98beda to fa566af Compare March 13, 2024 13:01

valentindavid mentioned this pull request May 31, 2024

tests: add test to refresh from old base #14031

Open

valentindavid marked this pull request as ready for review May 31, 2024 14:33

sergiocazzolato reviewed Jun 3, 2024

View reviewed changes

sergiocazzolato added the Run nested The PR also runs tests inluded in nested suite label Jun 3, 2024

sergiocazzolato requested changes Jun 3, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/with-patchelf-run-tests branch from bd87468 to 3349476 Compare June 13, 2024 12:03

bboozzoo reviewed Jun 17, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/with-patchelf-run-tests branch from 3137b4c to c73512d Compare June 18, 2024 08:40

valentindavid requested review from bboozzoo and sergiocazzolato June 18, 2024 14:25

valentindavid mentioned this pull request Jun 19, 2024

snapdtool, cmd: switch to FIPS mode when needed #13978

Merged

bboozzoo approved these changes Jun 19, 2024

View reviewed changes

valentindavid mentioned this pull request Jun 24, 2024

many: do not use nss when looking up for users/groups from snapd snap #13776

Merged

sergiocazzolato reviewed Jun 25, 2024

View reviewed changes

sergiocazzolato self-requested a review June 25, 2024 16:23

sergiocazzolato approved these changes Jun 25, 2024

View reviewed changes

valentindavid force-pushed the valentindavid/with-patchelf-run-tests branch from ffb295e to bb418ff Compare June 26, 2024 10:48

valentindavid added 9 commits June 28, 2024 10:01

fixup! tests: properly build snapd snap

e6d999b

fixup! tests: properly build snapd snap

7d6701e

fixup! tests: properly build snapd snap

27d219a

fixup! tests: properly build snapd snap

d17236c

fixup! tests: properly build snapd snap

7f41680

tests/nested/manual/cloud-init-nocloud-not-vuln: prevent automatic re…

5d66546

…fresh

spread.yaml: increase disk size for ubuntu arm machines

501c6e1

fixup! tests: properly build snapd snap

9b95b5c

valentindavid force-pushed the valentindavid/with-patchelf-run-tests branch from 165be0f to 9b95b5c Compare June 28, 2024 08:01

Meulengracht self-assigned this Jun 28, 2024

valentindavid closed this Jun 28, 2024

valentindavid mentioned this pull request Jun 28, 2024

tests: properly build snapd snap #14141

Merged

	if cat squashfs-root/meta/snap.yaml \| grep -q "version:.dirty."; then
	if grep -q "version:.dirty." squashfs-root/meta/snap.yaml squashfs-root/usr/lib/snapd/info; then

tests: properly build snapd snap #13517

tests: properly build snapd snap #13517

Conversation

valentindavid commented Jan 24, 2024 • edited Loading

codecov-commenter commented Jan 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

sergiocazzolato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiocazzolato left a comment

Choose a reason for hiding this comment

valentindavid commented Jan 24, 2024 •

edited

Loading

codecov-commenter commented Jan 29, 2024 •

edited

Loading