Allow overriding RUNTIME_DIR with a cli flag. #621
Conversation
|
Commits don't appear to be correctly signed off. |
|
They are signed off with my personal email, but I guess it compares it to my git config: Ive updated them, please re-run the checks. |
|
@stgraber, friendly ping. |
|
@sdab not sure what's up with Github Actions on this one. Could you do a quick rebase or pointless amend of your last commit so you can force push and re-trigger the Github stuff? |
|
I did a blank amend + force push. It looks like it needs approval to run the tests/checks. |
|
Looks like my amend undid the sign-off? Should be good now |
|
@mihalicyn can you review this please? |
src/lxcfs.c
Outdated
| else | ||
| lxcfs_debug("Opened %s", lxcfs_lib_path); | ||
|
|
||
| good: | ||
| if (strlen(runtime_path) > 0 && !do_set_runtime_path(runtime_path)) { |
There was a problem hiding this comment.
Do we really have to introduce two new exported symbols set_runtime_path and lxcfslib_init in there?
If you changing existing constructor to be a normal exported function you can also add an argument to it like this:
void lxcfslib_init(const char *new_path)
{
//... handle new_path here
As far as I understand you can't call set_runtime_path anywhere except before the lxcfslib_init anyways. Which is a good sign that this function should be a part of lxcfslib_init.
==
Second thing is that we are trying to maintain compatibility, which means that old LXCFS daemon should be able to load and use a new LXCFS library gracefully. With this change old LXCFS daemon will try to reload LXCFS library, constructor is missing, and it's fine and no error will be generated. But LXCFS will remain uninitialized and then it will crash or at least give fuse request failures to the user.
My idea is to move lxcfs_init (constructor) call to the lxcfs_fuse_init(struct fuse_conn_info *conn, void *data), because it's always called almost at the same time as constructor should be called. In this case you don't need to add a new exported symbol lxcfslib_init. Just remove a __attribute__((constructor)) from the lxcfs_init and then add an explicit lxcfs_init call in the lxcfs_fuse_init.
Please, correct me if I'm wrong ;-)
There was a problem hiding this comment.
There was a problem hiding this comment.
I looked at this for a bit, but dont see a way around breaking backwards compatibility. Essentially the problem is that we need to send a new parameter (runtime_path) between the binary and the library.
I did however merge set_runtime_path into lxcfslib_init, so there is only one new symbol.
There was a problem hiding this comment.
I guess your current solution is correct and does not break compatibility against the recent LXCFS version.
|
@sdab Sorry for the delay with review. I have left some comments regarding a compatibility issue. In general, this looks good to me. But I would improve this a bit otherwise it will make troubles for existing Incus/LXD installations as they will have to restart all the container instances after the upgrade. |
|
I'll let @mihalicyn provided a more detailed answer here but speaking of backward compat, a normal upgrade path on most distros will be that the library will be updated to 6.0 with a daemon running 5.0, that daemon will be asked to reload the library. Doing so should not result in a crash as that would instantly break all containers :) So we basically need the new lib to be fine with not being provided the extra data and behave as it currently would. But then if the extra data is provided, operate with the custom runtime dir. Also, the new symbol you're introducing here is very specific to providing the runtime path, would it make sense to have it provide some kind of init struct so we don't need to add another symbol the next time we need something like this? |
|
ack, I wasnt sure how strict the backwards compatibility requirement was (I thought maybe on a major version, it was ok to break). I have removed the runtime_path only export, unless you mean you want an more generic option struct as the function argument. Ill have to think of a backwards compatible way to do this. A separate symbol might be helpful in that case and make it only called by a newer binary (is backwards compatibility only required for old binaries and not old libs? or is it both ways) |
We only care about the case where an old binary (already running) needs to talk to a new library, the other way around shouldn't be possible. |
Yeah, that. Having an init function which only takes a single string argument is a recipe for needing an initv2 function down the line when we need to pass something else, using a struct from the start saves us from having to keep adding new functions. |
|
Ok I reworked the code a bit to make it backwards compatible. @stgraber I made use of the existing (and versioned) lxcfs_opts struct and bumped the version value to 2. Thus if an old binary loads this new lib then the version will be set to 1 and we will not attempt to read the new field. PTAL |
|
@mihalicyn let me know if I need to do anything else |
|
@sdab looks like you have a small conflict and Github Actions failed to run the tests because of that. Can you rebase on main? |
|
Rebased |
|
LGTM except small issue with a new field placement in the Once this fixed I'll test this change locally and check that I can update LXCFS from the version in the |
|
Addressed the comments, thanks @mihalicyn |
|
LGTM, thanks for your work on this, Sebastien! I'll make some small adjustments:
|
|
Updated version #645 |
|
btw @mihalicyn it would be useful to update the README or CONTRIBUTING files with:
|
That's a good point. Today, I started to work on adding a new CI test to check forward-compatibility. |
This adds a --runtime-dir flag which can override the RUNTIME_PATH (renamed RUNTIME_DEFAULT_PATH). This ended up being kind of tricky because of how lxcfslib can be reloaded and its use of a library constructor which mounts paths under the runtime path. In order read the cli flag and then set a variable in the library before it starts mounting things, I removed the constructor attribute and made it an explicit init call in order to override the runtime dir in between loading the library and initializing lxcfslib.
This allows me to run multiple instances of lxcfs without them sharing mounts/paths. Unfortunately, I think this a breaking lib change so it will require a restart.