-
Notifications
You must be signed in to change notification settings - Fork 587
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: fix fsck on boot on arm devices #9655
tests: fix fsck on boot on arm devices #9655
Conversation
sergio@cachiomachine:~/workspace/snapcore/snapd$ spread -debug external:ubuntu-core-18-arm-32:tests/core/fsck-on-boot |
sergio@cachiomachine:~/workspace/snapcore/snapd$ spread -debug external:ubuntu-core-20-arm-64:tests/core/fsck-on-boot |
Left the logs for arm devices which were tested for this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, but I think we need to keep the tests.cleanup restore
in the restore section
also a couple comments about simplification of the shell script to be more readable using environment variables
tests/core/fsck-on-boot/task.yaml
Outdated
@@ -54,7 +71,11 @@ execute: | | |||
# Use offset 65 as FAT32 kicks in for devices larger than 32MB | |||
printf "\x01" > one | |||
tests.cleanup defer rm -f one | |||
dd if=one of=/dev/sda2 seek=65 bs=1 count=1 conv=notrunc | |||
if os.query is-core16 || os.query is-core18; then | |||
dd if=/dev/disk/by-label/system-boot of=dirty skip=65 bs=1 count=1 conv=notrunc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nitpick, but instead of duplicating the whole dd command, you could instead just have the if
set an environment variable for the label like this:
if os.query is-core16 || os.query is-core18; then
LABEL=system-boot
elif os.query is-core20; then
LABEL=ubuntu-seed
else
echo "unknown core system, please update test"
exit 1
fi
dd if="/dev/disk/by-label/$LABEL" of=dirty skip=65 bs=1 count=1 conv=notrunc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can skip the else
branch given that the check for uc22 et al is also done above too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/me notes that this also would be a good use for canonical/spread#83 if that ever becomes a thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally agree, should be great to be able to define vars by system
tests/core/fsck-on-boot/task.yaml
Outdated
@@ -76,12 +97,14 @@ execute: | | |||
# chain. | |||
unmount_vfat | |||
cat /proc/self/mountinfo >boot1-after-umount.log | |||
dd if=/dev/sda2 of=dirty skip=65 bs=1 count=1 conv=notrunc | |||
if os.query is-core16 || os.query is-core18; then | |||
dd if=/dev/disk/by-label/system-boot of=dirty skip=65 bs=1 count=1 conv=notrunc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment about LABEL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Tests after fixes: sergio@cachiomachine:~/workspace/snapcore/snapd$ spread -debug external:ubuntu-core-20-arm-64:tests/core/fsck-on-boot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems the arguments to dd have been flipped incorrectly
tests/core/fsck-on-boot/task.yaml
Outdated
@@ -54,7 +87,7 @@ execute: | | |||
# Use offset 65 as FAT32 kicks in for devices larger than 32MB | |||
printf "\x01" > one | |||
tests.cleanup defer rm -f one | |||
dd if=one of=/dev/sda2 seek=65 bs=1 count=1 conv=notrunc | |||
dd if="/dev/disk/by-label/$LABEL" of=dirty skip=65 bs=1 count=1 conv=notrunc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is now wrong, it was
dd \
if=one \
of=/dev/... \
seek=65 \
...
and it has now become:
dd \
if=/dev/...
of=dirty \
skip=65 \
...
perhaps this line was mixed up with the one below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thanks for checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anonymouse64 updated
@@ -76,12 +109,10 @@ execute: | | |||
# chain. | |||
unmount_vfat | |||
cat /proc/self/mountinfo >boot1-after-umount.log | |||
dd if=/dev/sda2 of=dirty skip=65 bs=1 count=1 conv=notrunc | |||
dd if="/dev/disk/by-label/$LABEL" of=dirty skip=65 bs=1 count=1 conv=notrunc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now thanks for fixing it up
@@ -54,7 +87,7 @@ execute: | | |||
# Use offset 65 as FAT32 kicks in for devices larger than 32MB | |||
printf "\x01" > one | |||
tests.cleanup defer rm -f one | |||
dd if=one of=/dev/sda2 seek=65 bs=1 count=1 conv=notrunc | |||
dd if=one of="/dev/disk/by-label/$LABEL" seek=65 bs=1 count=1 conv=notrunc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is good now too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
Looks good but the test fails now with: https://paste.ubuntu.com/p/GV7wDKdnpj/ on google:ubuntu-core-20-64:tests/core/fsck-on-boot But it seems that failure is unrelated to the PR I saw it failing in other PRs too. |
Please squash merge this for easier 2.48 cherry picks :) |
Add support for arm devices for the test fsck-on-boot