Allow overriding RUNTIME_DIR with a cli flag.

[sdab]

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.

[stgraber]

Commits don't appear to be correctly signed off.

[sdab]

They are signed off with my personal email, but I guess it compares it to my git config: Expected "sdabdoub sdabdoub@netflix.com", but got "Sebastien Dabdoub sebastien.dabdoub@gmail.com".

Ive updated them, please re-run the checks.

[sdab]

@stgraber, friendly ping.

[stgraber]

@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?

[sdab]

I did a blank amend + force push. It looks like it needs approval to run the tests/checks.

[sdab]

Looks like my amend undid the sign-off? Should be good now

[stgraber]

@mihalicyn can you review this please?

[mihalicyn]

@sdab Hi, Sebastien!

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.

[stgraber]

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?

[sdab]

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)

[stgraber]

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.

[stgraber]

I have removed the runtime_path only export, unless you mean you want an more generic option struct as the function argument.

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.

[sdab]

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

[sdab]

@mihalicyn let me know if I need to do anything else

[stgraber]

@sdab looks like you have a small conflict and Github Actions failed to run the tests because of that. Can you rebase on main?

[sdab]

Rebased

[mihalicyn]

LGTM except small issue with a new field placement in the struct lxcfs_opts

Once this fixed I'll test this change locally and check that I can update LXCFS from the version in the main branch to the version in the sdab:main branch without crashes and issues.

[sdab]

Addressed the comments, thanks @mihalicyn

[mihalicyn]

LGTM, thanks for your work on this, Sebastien!

I'll make some small adjustments:

• squash 2 last commits into one
• small changes in the commit messages format to align with our current standards

[mihalicyn]

Updated version https://github.com/lxc/lxcfs/pull/645

[sdab]

btw @mihalicyn it would be useful to update the README or CONTRIBUTING files with:

• instructions on how to run the tests and a backwards compatibility test
• the requirement to be backwards compatible across the lib and binary

[mihalicyn]

• instructions on how to run the tests and a backwards compatibility test

That's a good point. Today, I started to work on adding a new CI test to check forward-compatibility. Hopefully, it will be ready for review tomorrow. ;-)