tests: new approach to run tests in ubuntu and debian using snapd deb from the repo #14496

sergiocazzolato · 2024-09-12T12:33:59Z

The idea of this pr is to change de default behavior for the deb
package used in our spread tests. To simulate a more realistic scenario, and
having snapd snap build from local, this change is adding as default a
new scenario to have the snapd deb installed from the deb repository.

Then the deb package is saved in the same place as we do when we build it
during the test, making easy to have different scenarios working together.

It is a requirement also so be able to run the tests using a built snapd
deb as we are currently doing.

to run the tests building snapd deb, do:
SPREAD_SNAPD_DEB_FROM_REPO=false spread google:ubuntu-22.04-64:tests/

… from the repo The idea of this change is to change de default behaviour for the deb pacage used in our spread tests. To simulate a more real scenario, and having snapd snap build from local, this change is adding as default a new scenario to have snapd installed from the deb repository. Then the deb package is saved in the same place as it was built in during the test, making easy to have both scenarios working together. It is a requirement also so be able to run the tests using a built snapd deb as we are currently doing.

…b77e1..33c1d672c6 33c1d672c6 update os.query test 5a428a71ea fix shellcheck 8b423d177a support opensuse 15.6 ab1fa59719 support tests.pkgs download 1e39b307b6 udpate image names for openstack 2a85365d51 remove support for fedora-38 and ubuntu mantic 112bc0a315 append in log-filter 1d4350a818 Make sure the tools are at the begining of the path 45c2f45004 address comments 8743ceaf8c tools: fix support for multi-arch packages on apt-based systems (canonical#54) 33aa8d1e53 fix details in remote.retry test e43b9e314d rename test libraries to avoid static check errors in snapd 4ab8b00afb New spread-filter tool (canonical#53) cdf5cfd47b Remove centos-7 support f3996cc3fa change the spread label 1e309f41c6 change how legacy parameter is determine in remote.pull c43c35f7e3 run remote refresh and wait-for for xenial (skip bionic) 5262d30da7 make sure the test jobs are executed in runners with the spread label cb74259b7a add openstack systems 0b41fd40d3 fix tests.pkgs on arch-linux 558e109793 run fedora-40 spread tests in openstack 6f6187416d fix list implementation b4a5439c9b added more type annotatios to log_helper 58da1e36c3 mypy cleaned 1ff651e680 update wording of remote.pull 18615b1667 just usc scp -O when ssh version is -ge 9 cc68c9868b Added type annotations for log-filter 66f90d10cd Adding -O to scp command to make it compatible in uc24 tests 496cb7b5b3 removing support for centos-8 f2eef30db4 Updated the log helper and log parser 5a375ebf73 Formatting for python utils d3eed3faa5 fix codespell in CODE_OF_CONDUCT.md 18bcca6b14 new log helper d60381fcd9 add run number to filtered filename 5dde2d67b8 consider the tests execution in main 6b9a3aabcc change filtered log name b2756aa579 default file is .filtered.log 500b9dace4 Fix tests workflow 45db26a3d2 fix shellcheck error in log-filter fe45c27b7d create a var to store filter params 5a9b66d7dc filter spread results 51f9b055af New tool used to filter the spread.log output b8d20c1d5b fix snaps.name test with correct siffix spelling f640ac72e3 Add missing test details f0754df304 Filter the error y debug output in log-parser fc10196efd Add suggestions to details 94ac5ffe58 Add details on tests 501578c719 add more checks in os.query to check is-core_xx e8929207ff fix os-query for ubuntu comparing with core 226114641f os.query won't check SPREAD_SYSTEM anymore to compare core systems b89ec98b23 use local variables in os.query tool dacfd81de9 fix is_core functions 1db5214d5f Improve the remote docs (canonical#36) 2e4a3153a2 1 more comment 3a0fc57e1e add explanation about why we check for ( Do | Doing ) 4cf8e635bf fix os.query test after merge b89b4f8647 fix artifacts name d30cee6da0 Merge remote-tracking branch 'upstream/main' 5ef5dcbe8f Tests use artifacts in spread tests (canonical#51) 555c43d2ab Support auto-refresh with Do instead of Doing 96c2b0c19c remove tests support for ubuntu 23.04 (EoL) 74082c0c34 Tests improve remote wait (canonical#49) 5121bfb659 remove support for opensuse leap 15.4 (canonical#48) 30df700d08 Add new systems support (canonical#47) 1f08938925 Support check amazon linux version (canonical#46) 43533bdd97 Change the exit value checking for test formats (canonical#45) 3c88244c04 Update check-test-format to support a dir and a list of files (canonical#44) 510d95f429 add extra check for error in auto-refresh detection function 3289d4031b Try open the log with latin-1 encoding when utf-8 is not working 9db785499f improved how the tools are waiting for system reboot 2a5c4414a3 fix shellcheck errors 5e7b63883d Fixes for osquery and tests pkgs (canonical#43) 4c9145e2ac support reboot waiting for auto-refresh 45768f5188 show changes in unknown status after refresh 8013c30c2a Remove support for ubuntu 22.10 b32b80bf54 Fix remote.rait-for test in bionic 5675c625e9 Enable fedora 38 55f4471957 Support for new oss f2e88b357c New tool used to query spread json reports cacd35ede0 utils/spread-shellcheck: explain disabled warnings (canonical#42) c82afb2dee Support --no-install-recommends parameter when installing dependencies with tests.pkgs b84eea92e2 spread-shellcheck: fix quotes in environment variables (canonical#41) ab1e51c29f New comparison in os-query for core systems (canonical#40) e5ae22a5d4 systemd units can be overwritten 63540b845a Fix error messages in remote pull and push 75e8a426a5 make sure the unit is removed in tests.systemd test 9089ff5c02 Update tests to use the new tests.systemd stop-unit 44ecd5e56a Move tests.systemd stop-units to stop-unit 01a2a83b4b Update tests.systemd to have stop units as systemd.sh 162e93bd35 update tests.systemd CLI options to be the same than retry command 14aa43a405 new feature to re-run failed spread tests (canonical#39) 604cb782db Fix shellcheck in systemd tool bfc71082c8 Update the tests.systemd to allow parameters waiting for service status 8a2d0a99df Adding quiet tool and removing set +-x from tests.pkgs d90935d2a4 A comment explaining about the default values for wait-for 3232c5dba7 Add support for ubuntu 23.04 a7164fba07 remove fedora 35 support, add fedora 37 support 89b9eb5301 Update systems supported 92bb6a0664 Include snap-sufix in the snaps.name tool git-subtree-dir: tests/lib/external/snapd-testing-tools git-subtree-split: 33c1d672c6512b23892481eeb013bd01e5eb389a

…date-snapd-package

sergiocazzolato · 2024-09-12T16:18:34Z

I see the deb from the repo is not built with testkeys, and this is making fail too many tests.

bboozzoo · 2024-09-16T09:13:39Z

the no-reexec tests will be broken as 2.63.1 (currently in the Ubuntu archive), does not have this fix 0165d14

this tool is used to check for re-exec and to know the source of the snapd package

…the repo

zyga

I did a full pass and left some comments. Most of my comments are purely technical in nature. I think overall this looks excellent and we shoud discuss next steps as to how to complete this work in the next pulse.

spread.yaml

tests/lib/external/snapd-testing-tools/spread.yaml

tests/lib/external/snapd-testing-tools/tools/tests.pkgs.apt.sh

tests/lib/external/snapd-testing-tools/tools/tests.pkgs.dnf-yum.sh

tests/lib/external/snapd-testing-tools/tools/tests.pkgs.zypper.sh

tests/lib/prepare-restore.sh

tests/lib/tools/tests.info

tests/main/classic-prepare-image-no-core/task.yaml

…date-snapd-package

…672c6..912131f2a6 912131f2a6 fix another shellcheck d27cc72346 update tests.pkgs to fix shellchecks git-subtree-dir: tests/lib/external/snapd-testing-tools git-subtree-split: 912131f2a6a7d3c807c890d94fe56367aca67039

…1f2a6..bae7faf8cb bae7faf8cb Update the quoting used to download packages git-subtree-dir: tests/lib/external/snapd-testing-tools git-subtree-split: bae7faf8cb3f04296bac9552ac0d1aa89d4e9139

…date-snapd-package

codecov · 2024-09-23T15:29:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.88%. Comparing base (ac897ee) to head (b50e533).
Report is 45 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14496      +/-   ##
==========================================
+ Coverage   78.85%   78.88%   +0.02%     
==========================================
  Files        1079     1082       +3     
  Lines      145615   146058     +443     
==========================================
+ Hits       114828   115215     +387     
- Misses      23601    23646      +45     
- Partials     7186     7197      +11

Flag	Coverage Δ
unittests	`78.88% <ø> (+0.02%)`	⬆️

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.

Changes considering the changes done in the re-exec workflow

Also we are inclugin some minor changes to address review comments

zyga

I did a full pass and I have a few comments.

We should not hard-code /snap/snapd/current. I get that in some places we have a symbolic link to make tests easier, but I think we also have all the other helpers that give the right value. Let's use them.
Some tests are skipped but I'm unsure why they need to be skipped. Is it because we need test keys or for some other reason. I have a hunch we can actually use test keys just fine - just not from the distribution snapd (since that is built without test keys).
I left a few requests for comments in the code so that the next time we look some of the logic is clearer and better documented.
If we have to skip a test, let's touch .skip in prepare and test -f .skip && exit in execute, restore and debug so that we don't have to repeat the condition of the skip. Less copy-pasted code that may drift over time to worry about.

I think I'm not ready to +1 this based on lack of confidence of skip due to testkeys. Let's sync tomorrow.

spread.yaml

tests/lib/prepare-restore.sh

tests/lib/tools/snapd.tool

zyga · 2024-09-24T20:19:06Z

tests/main/classic-custom-device-reg/task.yaml

@@ -15,7 +15,8 @@ environment:
    SEED_DIR: /var/lib/snapd/seed

 prepare: |
-    if [ "$TRUST_TEST_KEYS" = "false" ]; then
+    # In this scenario, the keys from the snapd pkg are used
+    if [ "$TRUST_TEST_KEYS" = "false" ] || tests.info is-snapd-pkg-repo; then


I was thinking if this is really the case. Snapd classic will re-exec into snapd snap that may be built with test keys. Does it really fail?

there are some scenarios where we don't have snapd snap installed, so it uses the keys from the deb.
I'll add a comment there

Thanks, this sounds good. If We identify all the cases that don't have snapd snap installed then we can work on fixing them. I assume that the point of the test was not to test that specific behavior and that we can fix them over time.

right, my idea is to create a following pr to cleanup some scenarios and fix other tests

zyga · 2024-09-24T20:20:31Z

tests/main/debug-execution/task.yaml

@@ -8,6 +8,11 @@ debug: |
    grep -n '' snap-*.out || true

 execute: |
+    # TODO: remove this check once snapd 2.65 is released


I think this todo cannot be removed because we will always have a distribution with old snapd.

I think the test needs more nuance to execute the SNAP_REEXEC=1 without other conditions.

tests/main/snap-seccomp/task.yaml

tests/main/snapd-homedirs-vendored/task.yaml

zyga · 2024-09-24T20:25:19Z

tests/main/store-state/task.yaml

-    # Teardown staging store
-    "$TESTSTOOLS"/store-state teardown-staging-store
-    snap info core | MATCH "store-url:.*https://snapcraft.io"
+    # Staging store cannot be used with snapd deb from the repository


Is this really true? Don't we re-exec into a snapd that trusts the staging store?

I tried but staging keys are not used when we re-exec into snapd snap.

tests/main/user-session-env/task.yaml

This is needed because the check in prepare-restore is done before we have an ubuntu-core image

Use snap-mount-dir to determine where snaps are mounted

zyga · 2024-09-30T08:06:09Z

Thank you for good progress. I see some tests are failing because they meet unexpected conditions:

For instance google:ubuntu-24.04-64:tests/main/snapd-reexec:core

+ echo 'Ensure we re-exec by default'
Ensure we re-exec by default
+ MATCH 'DEBUG: restarting into "/snap/core/current/usr/bin/snap"'
+ /usr/bin/env SNAPD_DEBUG=1 snap list
grep error: pattern not found, got:
logger.go:93: DEBUG: snap (at "/snap/core/current") is older ("2.63+git5348.e45449bd5~ubuntu16.04.1") than distribution package ("2.63.1+24.04")
Name  Version                    Rev    Tracking     Publisher    Notes
core  16-2.63+git5348.e45449bd5  17195  latest/edge  canonical**  core

The test should accept this as valid outcome (core snap is indeed older than distro package).

Then google-distro-1:debian-11-64:tests/main/user-session-env fails on a missing environment value:

+ MATCH 'XDG_DATA_DIRS=.*[:]?/var/lib/snapd/desktop[:]?.*'
grep error: pattern not found, got:
HOME=/home/test-fish
LANG=C.UTF-8
LOGNAME=test-fish
MAIL=/var/mail/test-fish
PATH=/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games
***
SHELL=/usr/bin/fish
SHLVL=1
USER=test-fish

I don't know if this is supposed to pass. Debian 11 has pretty old snapd and perhaps this part relies on updated data files that do not participate in re-execution. Perhaps we should mark it as known failure.

Other than that I consider this a +0.98. Let get it green and land it.

zyga

Heh, and obviously my comment was not counted as the review. Please see my longer comment just above this one.

spread.yaml

tests/lib/tools/tests.info

bboozzoo

Thanks for working on this and apologies for the delay in reviewing the code.

bboozzoo · 2024-10-01T07:08:03Z

tests/main/try/task.yaml

@@ -15,15 +15,21 @@ environment:
    SERVICE_NAME: "test-service"

 prepare: |
+    test -f .skip && exit 0


does it need touch .skip somewhere?

it is done in the spread.yaml. It is done in the prepare-each of the fips suite.

Hmmmmmm

I wonder if we can exit 0 from a prepare-each to effectively skip in prepare in the test? How does spread assemble sections of code together?

tests/lib/tools/tests.info

bboozzoo · 2024-10-01T07:13:32Z

tests/lib/tools/tests.info

+    echo "usage: tests.info <CMD>"
+    echo
+    echo "Supported commands:"
+    echo "    is-snapd-pkg-repo: indicates when the snap packages is from the repository"


should this be called is-snapd-deb-from-repo since it only works for debs? or is there a missing TODO to make this work on other systems as well?

well, if at some point the re-execution is implemented for rpms, then we could make other systems download the rpm as we do with the deb.

I think the name is fine and should not include the word "deb" -- we will soon enable this for RPMs in tests. My only personal preference would be to call it "is-snapd-from-archive" as it reads better than abbreviated repository IMO. No need to change this though :)

bboozzoo · 2024-10-01T07:33:18Z

spread.yaml

@@ -1430,6 +1432,10 @@ suites:
            "$TESTSLIB"/prepare-restore.sh --prepare-suite
        prepare-each: |
            "$TESTSLIB"/prepare-restore.sh --prepare-suite-each
+            # When snapd.deb is from the repo, we just need to test when re-exec is enabled
+            if [ "$SNAP_REEXEC" = "0" ] && tests.info is-snapd-pkg-repo; then
+                touch .skip


hmm .skip won't show up by default if you're in a debug shell. Can we use something more explicit? eg tests.skip-stamp and record the reason for skipping eg.:

echo "skipped due to snapd package installed from the distro's repository" > tests.skip-stamp

FWIW, it'd be amazing if there was a way to skip tasks in spread, such that a message is recorded in its output.

Yeah, but we need to wait for new spread maintainer to start making changes and accepting proposals. I think this is great for now. The chief advantage is that we skip without duplicating the condition.

tests/lib/prepare.sh

tests/lib/tools/tests.info

tests/main/classic-prepare-image/task.yaml

zyga

Three short comments but please see my last comment (topmost in the diff). I'll repeat the essence here:

Can we skip from prepare-each by abusing the way exit is interpreted when suite-level prepare-each is combined with prepare in the individual tasks?

bboozzoo

LGTM

bboozzoo · 2024-10-02T17:32:19Z

tests/main/snap-confine-tmp-mount/task.yaml

@@ -25,6 +25,9 @@ debug: |

 execute: |
    SNAP_CONFINE=$(os.paths libexec-dir)/snapd/snap-confine
+    if tests.info is-snapd-pkg-repo; then
+        SNAP_CONFINE="/snap/snapd/current$SNAP_CONFINE"


maybe needs a TODO or a comment, that the is-snapd-pkg-repo only works for deb at the moment, hence the distro where this scenario is executed all use /snap. But yeah, it should use mount dir if we want to avoid coming back to the test later

sergiocazzolato added 3 commits September 12, 2024 08:19

Merge commit 'a2637fda70a9d9f9fdc3d299712da9090735223f' into tests-up…

63b2c2f

…date-snapd-package

sergiocazzolato requested a review from zyga September 12, 2024 12:33

sergiocazzolato marked this pull request as draft September 12, 2024 14:15

download snapd snap-confine and ubuntu-core-launcher

d1f0f31

skip tests that trust on deb pkg testkeys

6d5d458

sergiocazzolato requested a review from bboozzoo September 17, 2024 19:09

sergiocazzolato added 4 commits September 17, 2024 16:16

fix shellcheck

ab0b03f

revert kill timeout

3c76431

fix tests using snapd.tool and store-state test

7d78737

new tests.info tool

87cbe02

this tool is used to check for re-exec and to know the source of the snapd package

sergiocazzolato marked this pull request as ready for review September 19, 2024 15:36

skip tests which are testing with re-exec:0 and snap is comming from …

1338735

…the repo

zyga reviewed Sep 19, 2024

View reviewed changes

sergiocazzolato added 9 commits September 19, 2024 15:15

update the systems where snapd comes from the repo

b7d5efb

skip another test when re-exec:0

d9aa6db

Merge branch 'master' into tests-update-snapd-package

3f2209b

Merge commit 'b0f4d8daff730868b7a60aebf75aac53c24b92c7' into tests-up…

5e50960

…date-snapd-package

Adjust failing test to new re-exec scenario

6cb8cb6

fix other 2 tests using correct paths

9ddfe30

Squashed 'tests/lib/external/snapd-testing-tools/' changes from 91213…

06f3a15

…1f2a6..bae7faf8cb bae7faf8cb Update the quoting used to download packages git-subtree-dir: tests/lib/external/snapd-testing-tools git-subtree-split: bae7faf8cb3f04296bac9552ac0d1aa89d4e9139

Merge commit '06f3a157d82cfa92350c7d9a0634690d05874764' into tests-up…

c589280

…date-snapd-package

sergiocazzolato added 3 commits September 23, 2024 15:12

Merge branch 'master' into tests-update-snapd-package

73548a7

some tests fixes

744c89a

Changes considering the changes done in the re-exec workflow

New set of test fixes

2e32c65

Also we are inclugin some minor changes to address review comments

update tests info command to check re-execution

514dbb8

sergiocazzolato requested a review from zyga September 24, 2024 14:38

fix uc16 which preparing

ec96de4

zyga reviewed Sep 24, 2024

View reviewed changes

sergiocazzolato added 2 commits September 24, 2024 17:32

check SPREAD_SYSTEM is not core in tests.info

e048240

This is needed because the check in prepare-restore is done before we have an ubuntu-core image

use .skip in tests to determine if a phase has to be skipped

cb9b585

Use snap-mount-dir to determine where snaps are mounted

sergiocazzolato requested a review from zyga September 26, 2024 02:11

sergiocazzolato added 11 commits September 26, 2024 10:00

skip updating core snap when the snap pkg is not built from local

463424e

save installed core snap when snap deb is used from the repo

5758688

copy also .orig core snap

cd751ac

change how we copy the core snap

8e5bfcb

change the snapname used for core snap

d7f37b9

skip saving the core snap

1624ebf

Fix error using snapcraft for testing in arm

5e1e520

make sure fips tests dont run with reexec=0 and snapd.deb from the repo

37487c1

simplify how fips tests are skipped

c0ccc68

fix indentation on test

b14c80d

Make sure we save core snap in TESTSTMP/core_snap

cde70a7

zyga reviewed Sep 30, 2024

View reviewed changes

spread.yaml Outdated Show resolved Hide resolved

tests/lib/tools/tests.info Show resolved Hide resolved

bboozzoo reviewed Oct 1, 2024

View reviewed changes

use tests.exec tool to skip tests

b50e533

sergiocazzolato requested review from bboozzoo and zyga October 1, 2024 19:01

zyga reviewed Oct 2, 2024

View reviewed changes

zyga self-requested a review October 2, 2024 06:42

bboozzoo approved these changes Oct 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: new approach to run tests in ubuntu and debian using snapd deb from the repo #14496

tests: new approach to run tests in ubuntu and debian using snapd deb from the repo #14496

sergiocazzolato commented Sep 12, 2024

sergiocazzolato commented Sep 12, 2024

bboozzoo commented Sep 16, 2024

zyga left a comment

codecov bot commented Sep 23, 2024 •

edited

Loading

zyga left a comment

zyga Sep 24, 2024

sergiocazzolato Sep 25, 2024

zyga Sep 30, 2024

sergiocazzolato Sep 30, 2024

zyga Sep 24, 2024

zyga Sep 24, 2024

sergiocazzolato Sep 30, 2024

zyga commented Sep 30, 2024

zyga left a comment

bboozzoo left a comment

bboozzoo Oct 1, 2024

sergiocazzolato Oct 1, 2024 •

edited

Loading

zyga Oct 2, 2024

bboozzoo Oct 1, 2024

sergiocazzolato Oct 1, 2024

zyga Oct 2, 2024

bboozzoo Oct 1, 2024

sergiocazzolato Oct 1, 2024

zyga Oct 2, 2024

zyga left a comment

bboozzoo left a comment

bboozzoo Oct 2, 2024

tests: new approach to run tests in ubuntu and debian using snapd deb from the repo #14496

Are you sure you want to change the base?

tests: new approach to run tests in ubuntu and debian using snapd deb from the repo #14496

Conversation

sergiocazzolato commented Sep 12, 2024

sergiocazzolato commented Sep 12, 2024

bboozzoo commented Sep 16, 2024

zyga left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 23, 2024 • edited Loading

Codecov Report

zyga 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

zyga commented Sep 30, 2024

zyga left a 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

sergiocazzolato Oct 1, 2024 • edited Loading

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

zyga left a 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

codecov bot commented Sep 23, 2024 •

edited

Loading

sergiocazzolato Oct 1, 2024 •

edited

Loading