Skip to content

Remove stateless re-execution work-around#70

Merged
xnox merged 1 commit intocanonical:mainfrom
valentindavid:valentindavid/stateful-reexec
Feb 8, 2022
Merged

Remove stateless re-execution work-around#70
xnox merged 1 commit intocanonical:mainfrom
valentindavid:valentindavid/stateful-reexec

Conversation

@valentindavid
Copy link
Copy Markdown
Member

This depends on #68 #67 #66 #64 #61 #58. It will be put as ready for review as soon as all the other PRs are merged.

@valentindavid valentindavid force-pushed the valentindavid/stateful-reexec branch 7 times, most recently from 45aaa2e to 21ae25e Compare February 4, 2022 18:37
@valentindavid valentindavid force-pushed the valentindavid/stateful-reexec branch from 21ae25e to 11b2663 Compare February 7, 2022 09:00
@valentindavid valentindavid marked this pull request as ready for review February 7, 2022 09:19
Copy link
Copy Markdown
Member

@alfonsosanchezbeato alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@anonymouse64
Copy link
Copy Markdown
Contributor

LGTM, thank you for splitting all this work up and submitting individually, can we update one of your snapd PR's to reference this branch (or set of commits) to see all the snapd tests run against this version of core-initrd?

@valentindavid
Copy link
Copy Markdown
Member Author

LGTM, thank you for splitting all this work up and submitting individually, can we update one of your snapd PR's to reference this branch (or set of commits) to see all the snapd tests run against this version of core-initrd?

The main branch of core-initrd has since been updated for core22. Can we run snapd tests on core22?

@xnox xnox merged commit 0478fdd into canonical:main Feb 8, 2022
@xnox
Copy link
Copy Markdown
Contributor

xnox commented Feb 8, 2022

we have daily edge uc22 images and testing; i think we are good to upload this into ppa, and respin kernel snaps, and then we will get full test results the next day across the uc22 testing.

@anonymouse64
Copy link
Copy Markdown
Contributor

Is this change destined to only go into UC22 kernel snaps then? Or will it also be back-ported to UC20 kernel snaps?

ATM, I don't think we have any spread tests running against UC22 in snapd proper

@alfonsosanchezbeato
Copy link
Copy Markdown
Member

Is this change destined to only go into UC22 kernel snaps then? Or will it also be back-ported to UC20 kernel snaps?

UC22 for the moment, we can backport when we think things look safe.

@anonymouse64
Copy link
Copy Markdown
Contributor

Hmm, just not sure how you can declare that it looks safe given that the widest set of tests for snaps on ubuntu core is in the snapd spread suite which doesn't currently test uc22...

@alfonsosanchezbeato
Copy link
Copy Markdown
Member

alfonsosanchezbeato commented Feb 9, 2022

Hmm, just not sure how you can declare that it looks safe given that the widest set of tests for snaps on ubuntu core is in the snapd spread suite which doesn't currently test uc22...

I wrote "we can backport when we think things look safe", not that it looks safe...

@xnox
Copy link
Copy Markdown
Contributor

xnox commented Feb 9, 2022

Hmm, just not sure how you can declare that it looks safe given that the widest set of tests for snaps on ubuntu core is in the snapd spread suite which doesn't currently test uc22...

There are edge images for uc22 available now, is there a way to open an issue to request snapd spread suite to enable testing on uc22?

@anonymouse64
Copy link
Copy Markdown
Contributor

There are edge images for uc22 available now, is there a way to open an issue to request snapd spread suite to enable testing on uc22?

One issue I just found today testing something else is that we can no longer inject snap-bootstrap as built by the snapd snap into the initramfs of the uc22 kernel snap, because it depends on libpthread.so.0:

[    1.164972] the-tool[211]: /usr/lib/snapd/snap-bootstrap: error while loading shared libraries: libpthread.so.0: cannot open shared object file: No such file or directory

Fixing this would be nice and make it easier to enable spread tests inside snapd proper

@valentindavid
Copy link
Copy Markdown
Member Author

@anonymouse64 Has this been added recently? I suppose this is be added to the initrd once that version of snapd is released. Until then... Does it load libpthread.so.0 with dlopen or with a NEEDED in the dynamic section of the elf? In the former case, it has to be copied manually. In the latter, you can use dracut-install --ldd to copy binaries with all its dependencies.

@anonymouse64
Copy link
Copy Markdown
Contributor

@valentindavid this happens when you just take the snap-bootstrap binary out of the snapd snap and copy it into the initramfs.

It's dynamically linked to libpthread.so.0, and indeed it's in the NEEDED dynamic section:

$ objdump -x /snap/snapd/current/usr/lib/snapd/snap-bootstrap

...

Dynamic Section:
  NEEDED               libpthread.so.0
  NEEDED               libc.so.6

@anonymouse64
Copy link
Copy Markdown
Contributor

I suppose a fair point is that for these sort of tests we could just copy libpthread.so.0 into the initramfs too

@xnox
Copy link
Copy Markdown
Contributor

xnox commented Feb 16, 2022

@anonymouse64 this is maybe because you are building snap-bootstrap incorrectly in xenial? where as snap-bootstrap in core20-initrd must be built in focal; and core22-initrd must be built in jammy? since glibc 2.34 pthread functionality has moved into libc6 itself and thus builds in jammy against jammy's glibc no longer produce libpthread dependency, nor include pthread.so into the initrd, as it is now an empty library without any functionality. It is also small, because it is empty.

@xnox
Copy link
Copy Markdown
Contributor

xnox commented Feb 16, 2022

i.e. snap-bootstrap vendoring into core22-initrd should be done by extracting snap-bootstrap build from jammy.deb build; not from xenial.snap build.

@anonymouse64
Copy link
Copy Markdown
Contributor

I see your point, but having to build snap-bootstrap independently from building the rest of the snapd snap to test things / do development on UC22 adds quite a bit of friction. I would be fine if it just works to copy in the libpthread.so.0 library from the snapd snap as well, I haven't tested that yet, hopefully that works. Otherwise perhaps we could somehow build snap-bootstrap to not depend on libpthread.so.0

@xnox
Copy link
Copy Markdown
Contributor

xnox commented Feb 16, 2022

we could somehow build snap-bootstrap to not depend on libpthread.so.0

if you do that, it would fail to work in core20-initrd. Building against glibc 2.33 and older generates pthread linking; and actually need it at runtime. Builds against glibc 2.34+ do not generate pthread linking, and require glibc 2.34 or newer at runtime, and will fail to run against glibc 2.33.

building snap-bootstrap alone, with its unittests, is very quick. We don't need the rest of the snapd build, to get snap-bootstrap.

@xnox
Copy link
Copy Markdown
Contributor

xnox commented Feb 16, 2022

copying pthreads.so from your build is a bad idea; you really need the empty pthreads library if you want to run against newer glibc. As you will encounter symbols clash. Do not copy old xenial pthreads into core22-initrd.

@xnox
Copy link
Copy Markdown
Contributor

xnox commented Feb 16, 2022

i am actually failing to see how xenial builds of snap-bootstrap are at all valid to inject into and test core20-initrd/core22-initrd. Since core20-initrd/core22-initrd never use such builds in production. It is interesting to know that such a combination is being tested, but it is doesn't match production at all.

@anonymouse64
Copy link
Copy Markdown
Contributor

Thanks for my weekly reminder of how terrible glibc API breakage is :-(

building snap-bootstrap alone, with its unittests, is very quick. We don't need the rest of the snapd build, to get snap-bootstrap.

The most common development scenario for me as a snapd developer is usually not where I need to just test snap-bootstrap by itself, it's when I make changes to snapd codebase overall such that I need to test both userspace snapd snap changes as well as initramfs changes to snap-bootstrap, now instead of just spitting out a single snapcraft build of the snapd snap, I need to go fire up a jammy instance and build snap-bootstrap specifically there which adds to the number of steps I have to take to test things out and the number of build environments I need to maintain and use regularly.

@xnox
Copy link
Copy Markdown
Contributor

xnox commented Feb 21, 2022

Your usecase is not too dissimilar to somebody developing customized TPM hook binaries and including them in their builds. Which in similar fashion may want to build one binary and use it on both UC20 and UC22. Looking at /lib/x86_64-linux-gnu/libpthread.so.0 it is very small only 22kb and it does basically just exports version symbols only. It probably does make usability wise to just have it included.

snapd automated non-human CI, should have provisions to correctly build matching target userspace snap-bootstrap though.

And yes glibc improve/break API all the time, they limit their ABI stability only against old compiled binaries which sucks a lot. I.e. I wish on a new 2.34 glibc install I could say "compile it for at most 2.31" and for example link pthreads and only use 2.31 symbols, but alas it is not available. Despite most current 2.34 glibc having all the symbols available and exported of older glibc versions.

valentindavid added a commit to valentindavid/snapd that referenced this pull request Jul 31, 2024
core-initrd for UC20 is missing the following fixes:
 * canonical/core-initrd#64
 * canonical/core-initrd#70
 * canonical/core-initrd#97

Because of that, it still kills recovery-chooser-trigger from initrd,
and expects it to re-run from snapd snap. Until those fixes
are backported, we need to keep on building snap-bootstrap.

This is not needed by UC22+
ernestl pushed a commit to canonical/snapd that referenced this pull request Aug 2, 2024
core-initrd for UC20 is missing the following fixes:
 * canonical/core-initrd#64
 * canonical/core-initrd#70
 * canonical/core-initrd#97

Because of that, it still kills recovery-chooser-trigger from initrd,
and expects it to re-run from snapd snap. Until those fixes
are backported, we need to keep on building snap-bootstrap.

This is not needed by UC22+
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.

4 participants