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

core: new feature MountImages #16321

Merged
merged 4 commits into from
Aug 6, 2020
Merged

core: new feature MountImages #16321

merged 4 commits into from
Aug 6, 2020

Conversation

bluca
Copy link
Member

@bluca bluca commented Jun 30, 2020

Picking up from #14451 as discussed there, and applying the review suggestions:

  • use dissect-image logic rather than mount() syscall
  • renamed from PathImages to MountImages
  • removed override of RootImage to avoid duplication
  • removed mount options (to be sorted separately, have a commit ready if agreement on the same feature for RootImage in service: add new RootImageOptions feature #16308 is reached)

(note that normally I'd do a single commit for something like this, but wanted to keep the original commit from #14451 to give full credit to @topimiettinen in the git history)

@bluca

This comment has been minimized.

Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

Looks pretty OK, but the string parsing code is very complex. I think it'd be much nicer is the string parsing part was factored out.

src/core/dbus-execute.c Outdated Show resolved Hide resolved
src/core/namespace.c Outdated Show resolved Hide resolved
src/core/dbus-execute.c Outdated Show resolved Hide resolved
src/core/execute.c Outdated Show resolved Hide resolved
src/core/load-fragment.c Outdated Show resolved Hide resolved
src/core/namespace.c Outdated Show resolved Hide resolved
src/core/load-fragment.c Show resolved Hide resolved
@bluca
Copy link
Member Author

bluca commented Jul 7, 2020

Looks pretty OK, but the string parsing code is very complex. I think it'd be much nicer is the string parsing part was factored out.

Done, let me know if it looks like what you had in mind or there's some more improvements to make. Thanks!

@bluca
Copy link
Member Author

bluca commented Jul 7, 2020

Where's the log for the sephamoreci? I'm clicking around on the webpage interface but can't see it

@bluca
Copy link
Member Author

bluca commented Jul 7, 2020

centos failure looks unrelated, it's in the udev selftest:

sda: Failed to rename '/dev/..tmp-b8:0' to '/dev/.': Device or resource busy

@mrc0mmand
Copy link
Member

centos failure looks unrelated, it's in the udev selftest:

sda: Failed to rename '/dev/..tmp-b8:0' to '/dev/.': Device or resource busy

Yep, that's #15978...

@bluca
Copy link
Member Author

bluca commented Jul 8, 2020

Can anybody kick semaphoreci? There seems to be no logs available, as far as I can see

@mrc0mmand
Copy link
Member

Can anybody kick semaphoreci? There seems to be no logs available, as far as I can see

I've already sent an email to the Semaphore CI support, it's been happening for the past few days pretty constantly. Just ignore it for now.

@evverx
Copy link
Member

evverx commented Jul 8, 2020

FWIW I switched back to Ubuntu Trusty there and to judge from https://semaphoreci.com/systemd/systemd/branches/pull-request-16321/builds/4 it seems to be working in the sense that the logs are there at least. Probably the new platform is still kind of experimental.

@evverx
Copy link
Member

evverx commented Jul 8, 2020

I've just switched to Ubuntu Xenial and judging by https://semaphoreci.com/systemd/systemd/branches/pull-request-16321/builds/5 the tests are going to pass so let's stick to it for now. @mrc0mmand could you switch it to Ubuntu Bionic once they reply to you?

@mrc0mmand
Copy link
Member

I've just switched to Ubuntu Xenial and judging by https://semaphoreci.com/systemd/systemd/branches/pull-request-16321/builds/5 the tests are going to pass so let's stick to it for now. @mrc0mmand could you switch it to Ubuntu Bionic once they reply to you?

Will do, thanks for looking into that!

src/basic/strv.c Outdated Show resolved Hide resolved
src/core/namespace.c Outdated Show resolved Hide resolved
src/systemctl/systemctl.c Outdated Show resolved Hide resolved
src/test/test-strv.c Outdated Show resolved Hide resolved
@bluca
Copy link
Member Author

bluca commented Jul 9, 2020

I think it's already too late give rc1 is out, but just in case: even if approved, I think this should be delayed until after v246 is out, as I'd like #16308 to be sorted one way or the other before this option makes its way into a release, so the same interface can be used for this option.

@mrc0mmand
Copy link
Member

mrc0mmand commented Jul 13, 2020

@evverx the Semaphore CI team got back to me:

A fix has been implemented for the server where you encountered this issue and you should not be experiencing it anymore. I'm truly sorry for the inconvenience.
Could you confirm the status is green on your end as well?

so I switched the Sempahore CI to Ubuntu Bionic to see how it fares.

@keszybz keszybz added this to the v247 milestone Jul 14, 2020
src/basic/strv.c Show resolved Hide resolved
man/systemd.exec.xml Show resolved Hide resolved
man/systemd.exec.xml Outdated Show resolved Hide resolved
src/core/load-fragment.c Outdated Show resolved Hide resolved
src/core/namespace.c Outdated Show resolved Hide resolved
src/core/namespace.c Outdated Show resolved Hide resolved
src/core/namespace.c Outdated Show resolved Hide resolved
src/core/namespace.c Outdated Show resolved Hide resolved
src/core/namespace.c Outdated Show resolved Hide resolved
src/core/namespace.c Outdated Show resolved Hide resolved
@poettering poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jul 14, 2020
@bluca

This comment has been minimized.

@mrc0mmand
Copy link
Member

Build CI has trouble installing packages:

The following packages have unmet dependencies:
 kbd : Depends: console-setup but it is not going to be installed or
                console-setup-mini but it is not going to be installed

There's a broken-deps pandemic, so just ignore it for now - #16453 (comment)

src/basic/strv.c Show resolved Hide resolved
src/core/dbus-execute.c Show resolved Hide resolved
src/core/dbus-execute.c Outdated Show resolved Hide resolved
src/core/namespace.h Outdated Show resolved Hide resolved
@bluca bluca force-pushed the mount_images branch 2 times, most recently from 3b6d5a1 to e151f0c Compare July 21, 2020 13:31
@bluca

This comment has been minimized.

@bluca

This comment has been minimized.

@bluca
Copy link
Member Author

bluca commented Jul 24, 2020

New push fixes the UNIT_WRITE_FLAGS_NOOP to make the runtime change not happen as well (previously it erroneously just wrapped the unit setting write, not the global data structure update)

@keszybz keszybz removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jul 26, 2020
src/basic/strv.c Outdated Show resolved Hide resolved
src/basic/strv.c Outdated Show resolved Hide resolved
src/test/test-strv.c Outdated Show resolved Hide resolved
src/core/dbus-execute.c Outdated Show resolved Hide resolved
src/core/namespace.c Outdated Show resolved Hide resolved
@bluca bluca force-pushed the mount_images branch 2 times, most recently from f6ce7b6 to 921318b Compare August 3, 2020 09:33
@bluca

This comment has been minimized.

@bluca

This comment has been minimized.

@keszybz keszybz mentioned this pull request Aug 3, 2020
Covers some functionality that we want to use for config tuples
keszybz and others added 2 commits August 5, 2020 21:29
This allows separators to be escaped, for example to allow
"a\:b:c", to be treated as "a:b", "c" with ":" as the separator.
Given a string in the format 'one:two three four:five', returns a string
vector with each word. If the second element of the tuple is not
present, an empty string is returned in its place, so that the vector
can be processed in pairs.

[zjs: use EXTRACT_UNESCAPE_SEPARATORS instead of EXTRACT_CUNESCAPE_RELAX.
This way we do escaping exactly once and in normal strict mode.]
Follows the same pattern and features as RootImage, but allows an
arbitrary mount point under / to be specified by the user, and
multiple values - like BindPaths.

Original implementation by @topimiettinen at:
systemd#14451
Reworked to use dissect's logic instead of bare libmount() calls
and other review comments.
Thanks Topi for the initial work to come up with and implement
this useful feature.
@keszybz
Copy link
Member

keszybz commented Aug 6, 2020

LGTM.

@keszybz keszybz merged commit f1cc283 into systemd:master Aug 6, 2020
@bluca bluca deleted the mount_images branch August 6, 2020 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants