Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

sergiocazzolato
Copy link
Collaborator

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
@sergiocazzolato sergiocazzolato marked this pull request as draft September 12, 2024 14:15
@sergiocazzolato
Copy link
Collaborator Author

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

@bboozzoo
Copy link
Contributor

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

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

@zyga zyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
tests/lib/external/snapd-testing-tools/spread.yaml Outdated Show resolved Hide resolved
tests/lib/prepare-restore.sh Outdated Show resolved Hide resolved
tests/lib/tools/tests.info Show resolved Hide resolved
tests/lib/tools/tests.info Show resolved Hide resolved
tests/main/classic-prepare-image-no-core/task.yaml Outdated Show resolved Hide resolved
…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
Copy link

codecov bot commented Sep 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.89%. Comparing base (ac897ee) to head (38959dc).
Report is 61 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #14496      +/-   ##
==========================================
+ Coverage   78.85%   78.89%   +0.04%     
==========================================
  Files        1079     1083       +4     
  Lines      145615   146377     +762     
==========================================
+ Hits       114828   115488     +660     
- Misses      23601    23688      +87     
- Partials     7186     7201      +15     
Flag Coverage Δ
unittests 78.89% <ø> (+0.04%) ⬆️

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
Copy link
Contributor

zyga commented Sep 30, 2024

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.

Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

spread.yaml Outdated Show resolved Hide resolved
tests/lib/tools/tests.info Show resolved Hide resolved
Copy link
Contributor

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

prepare: |
test -f .skip && exit 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it need touch .skip somewhere?

Copy link
Collaborator Author

@sergiocazzolato sergiocazzolato Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not possible, prepare-each is executed before than the prepare section of the test, and if exit 0 then the prepare section of the test is executed (it is not skipped).

tests/lib/tools/tests.info Outdated Show resolved Hide resolved
echo "usage: tests.info <CMD>"
echo
echo "Supported commands:"
echo " is-snapd-pkg-repo: indicates when the snap packages is from the repository"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

spread.yaml Outdated Show resolved Hide resolved
tests/lib/prepare.sh Outdated Show resolved Hide resolved
tests/lib/prepare.sh Outdated Show resolved Hide resolved
tests/lib/prepare.sh Show resolved Hide resolved
tests/lib/tools/tests.info Outdated Show resolved Hide resolved
tests/lib/tools/tests.info Outdated Show resolved Hide resolved
tests/main/classic-prepare-image/task.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@zyga zyga self-requested a review October 2, 2024 06:42
Copy link
Contributor

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

@bboozzoo @zyga Please, could do make a last pass here? I updated the failing tests.

Copy link
Contributor

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

fi
fi
if [ "$IS_REEXEC" = true ]; then
/usr/bin/env SNAPD_DEBUG=1 snap list 2>&1 | MATCH "DEBUG: restarting into \"$SNAPD_MOUNT_DIR/current/usr/bin/snap\""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would work too, wouldn't it?

Suggested change
/usr/bin/env SNAPD_DEBUG=1 snap list 2>&1 | MATCH "DEBUG: restarting into \"$SNAPD_MOUNT_DIR/current/usr/bin/snap\""
SNAPD_DEBUG=1 snap list 2>&1 | MATCH "DEBUG: restarting into \"$SNAPD_MOUNT_DIR/current/usr/bin/snap\""

fi
sleep 1
done
lxc exec my-ubuntu -- snap install --dangerous "$GOHOME/$CURRENT_SNAPD_SNAP_NAME"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes sense now. the binaries request an interpreter at /snap/snapd/current/. When preseed is invoked through snap prepare-image, it will set up a temporary bind mount so that the interpreter can be found.

Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a mini-review on internal chat but to recap. Remaining test failures indicate that the test runs a somehow skewed copy where new snapd emits securirty profiles but old snap-confile fails to find them (snap-seccomp transition from .bin to .bin2). We should see if those tests need adjusting or if they are no longer sensible.

I had a look at failures of:

  • google:ubuntu-16.04-64:tests/main/auto-refresh-pre-download:close_mid_restart
  • google:ubuntu-16.04-64:tests/main/snap-run-devmode-classic:core_first
  • google:ubuntu-16.04-64:tests/main/snap-run-devmode-classic:snapd_first
  • google:ubuntu-16.04-64:tests/main/snapd-update-services

Some of those had insufficient information on failure, but I suspect the reason is common across them.

Fix tests that needed to be updated after snapd snap is build from
current
Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to approve this but there are three issues and one potential bug.

To unblock this we musts fix the logic that finds the core snap by revision. I left a partial suggestion that should be useful as a starting point.

We must also do something else for the udev and systemd generator tests - those are not measuring what will really happen on a normal system with snapd snap installed. Neither of the two programs participates in re-execution and the test is only measuring what happens when a classic package is updated.

Lastly the case about test keys is really bugging me as I think it's either indicative of a bug or the test is failing for another reason (I didn't check yet, sorry). If classic snapd is build with normal production keys but snapd snap is built with test keys, when we re-execute we should honor test keys without issues. If we are not doing that then something is fishy/wrong and warrants investigation.

CC @bboozzoo as you've approved this and I think you want to re-read.

tests/lib/prepare.sh Outdated Show resolved Hide resolved
tests/lib/prepare.sh Outdated Show resolved Hide resolved
tests/lib/tools/tests.info Outdated Show resolved Hide resolved
tests/lib/tools/tests.info Show resolved Hide resolved
tests/main/snapd-snap-transition-auto-install/task.yaml Outdated Show resolved Hide resolved
# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @bboozzoo do you have any idea why this would be the case? I think this is a bug somewhere.

@@ -54,13 +54,18 @@ execute: |
su -c 'sh -c "exec env"' -l "$TEST_FISH_USER" > "${TEST_FISH_USER}-child-env"

SNAP_MOUNT_DIR="$(os.paths snap-mount-dir)"
LOCAL_PATH=
if tests.info is-snapd-pkg-repo; then
MOUNT_DIR="$(os.paths snap-mount-dir)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is similar to the udev test that is just not measuring anything real. The test should be skipped on distributions where the systemd generators are too old or they give false sense of security where the reality is that it just doesn't work this way. Those helpers do not re-execute.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this as-is and add this disclaimer next to this if statement:

# NOTE: This test doesn't measure what happens with a system using old classic
# package and a new snapd snap. Instead it measures what happens when the classic
# package is updated to contain the code in the tested branch. The test is valid
# but does not give confidence of the behavior during re-execution, as systemd
# generators do not participate in re-execution.

Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the new comment I've suggested inline for the test working with systemd generators.

Once this lands we need to look at the test keys and re-execution, as that IMO is still wrong. I would like to try to re-enable that logic and see what really happens.

Since there will be other follow-ups, I think this is in a state good for landing and further iteration.

EDIT: For clarity, @bboozzoo figured out why it fails and it seems we fail before we re-execute due to global initialization happening before main.

@sergiocazzolato sergiocazzolato merged commit f63a4a8 into canonical:master Oct 10, 2024
57 of 58 checks passed
@sergiocazzolato sergiocazzolato deleted the tests-update-snapd-package branch October 10, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants