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

Mountpoint order is not preserved in current manifest syntax #23

Closed
mkow opened this issue Mar 8, 2021 · 3 comments
Closed

Mountpoint order is not preserved in current manifest syntax #23

mkow opened this issue Mar 8, 2021 · 3 comments
Labels
enhancement New feature or request P: 2

Comments

@mkow
Copy link
Member

mkow commented Mar 8, 2021

Description of the problem

Currently we don't use proper TOML syntax for declaring mountpoints, instead, we use a syntax which resembles the pre-TOML one used in Graphene. As a result, the entries are not ordered, but Graphene actually relies on the specific mounting order (e.g. you can't mount /lib/asdf first and then /lib, but the other way around works). The problem is, that TOML structure is just a dictionary, so the order of keys is not preserved.

This will be solved by finally migrating to a proper TOML syntax for everything, which will be eventually done (after this, mount points list should be a TOML array). Cleaning up mounting code should also help here - I think that in Graphene mount order shouldn't matter, we only need to ensure that there are no duplicated mounts.

Steps to reproduce

Swap the order of entries in e.g. manifest.template in LibOS regression, so that "$(ARCH_LIBDIR)" is listed before "/lib".

Expected results

Everything still works.

Actual results

[P6098:T1:bootstrap_cpp] Mount /lib already exists, verify that there are no duplicate mounts in manifest
[P6098:T1:bootstrap_cpp] (note that /proc and /dev are automatically mounted in Graphene).
[P6098:T1:bootstrap_cpp] Mounting file:../../../../Runtime on /lib (type=chroot) failed (17)
[P6098:T1:bootstrap_cpp] Error during shim_init() in init_mount (-17)

Additional information

gramineproject/graphene#2210 contains an ugly workaround, but we need a proper solution.

@dimakuv
Copy link

dimakuv commented Mar 8, 2021

True. We will need to change the manifest syntax for this...

Notice that this only happens on gramineproject/graphene#2210, which adds TOML parsing to our Python scripts. The default TOML parser in Python doesn't preserve the order.

Interestingly, the TOML parser that we use in Graphene itself (in our C codebase) preserves the order of TOML entries. That's why we haven't seen this error manifesting itself in Graphene itself.

UPDATE: Ok, so I looked at gramineproject/graphene#2210. My comment above is wrong. We do modify the C code. But I'm not sure why this is needed currently, @mkow what tests actually break? I understand that our current code is brittle, but the C implementation worked, so what has changed?

@mkow
Copy link
Member Author

mkow commented Mar 8, 2021

I answered the same question under gramineproject/graphene#2210. TL;DR: migrating Python scripts to a proper TOML library shuffled the order (which is perfectly correct) and broke the incorrect assumptions our C code has.

@mkow mkow transferred this issue from gramineproject/graphene Sep 10, 2021
@mkow mkow added enhancement New feature or request P: 2 labels Sep 10, 2021
@dimakuv
Copy link

dimakuv commented Mar 9, 2023

This should be fixed now, as we moved to a proper TOML syntax with fs.mounts being a TOML array (which preserves the order).

@dimakuv dimakuv closed this as completed Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P: 2
Projects
None yet
Development

No branches or pull requests

2 participants