Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

remove specload plugin #4444

Closed
kodebach opened this issue Aug 24, 2022 · 16 comments
Closed

remove specload plugin #4444

kodebach opened this issue Aug 24, 2022 · 16 comments
Assignees
Labels

Comments

@kodebach
Copy link
Member

kodebach commented Aug 24, 2022

The current specload plugin is not really recommended, but it could be improved on the new-backend branch.

I propose 3 changes:

  1. Turn specload into a standalone backend plugin
  2. Remove the modifications/overrides feature.
  3. Remove file mode
  4. Add symbol mode, in addition to app/"executable" mode

Standalone backend plugin

Instead of being a storage plugin for the file-based backend plugin backend, the specload plugin should be backend plugin itself. This makes the various restrictions it imposes a lot more natural and gets rid of some problems, e.g. in relation to the resolver.

To do this we need to do a few things:

  1. specload should be a very minimal and restricted backend plugin similar to modules.
  2. specload can only be mounted on spec:/ parent keys.
  3. Everything that can currently be configured via the plugin config, should instead be configured via the mountpoint definition and be processed in init instead of open.

Remove modifications/overrides

specload will be read-only, i.e. like modules it doesn't provide a set function at all. All code related to modifications or the override file shall be removed.

The functionality implemented by specload should only be:

  • in open create an instance of the plugin
  • in close free all resources
  • in init parse the mountpoint definition (previously done from config in open) and return ELEKTRA_PLUGIN_STATUS_NO_UPDATE (read-only)
  • in get:
    • during resolver phase: return ELEKTRA_PLUGIN_STATUS_SUCCESS without doing anything
    • during storage phase: load the spec as configured in the mountpoint definition and return
    • during any other phase: return ELEKTRA_PLUGIN_STATUS_NO_UPDATE without doing anything

Remove file mode

Remove file mode since, without the modifications feature, it doesn't provide any benefit over a normal file-based backend with quickdump as storage.

New symbol mode

Note system:/ keynames below refer to the KeySet * definition passed to the init function.

The existing modes of the specload plugin are:

  • "executable" mode: An executable specified as system:/app is called with the arguments defined in system:/app/args/#. The spec is read in quickdump format from the stdout of this executable.
  • "file" mode: specload directly instructs quickdump to load the file defined in system:/file (removed above).

To better support the mode where an application loads it's own spec, I propose adding a new "symbol" mode. In this mode, system:/symbol defines the name of a symbol exported by the current program or by a library defined in system:/symbol/library. More specifically, this symbol must be a zero-arg function that returns a KeySet *.

specload will then use something like this to load the spec data:

// Note: error handling omitted

const char * symbol; // value of system:/symbol from definition (parsed during init)
const char * library; // value of system:/symbol/library from definition, or NULL (parsed during init)

typedef KeySet * (*specFunc)(void);

void * handle = dlopen (library, RTLD_LAZY);
specFunc loadSpec;

// workaround for void* to func cast, as per dlsym(3)
*(void **) (&loadSpec) = dlsym (handle, symbol);

KeySet * spec = loadSpec ();

For an example of using this see #4442. The basic idea is that a standalone application would export the symbol and then use the kdbOpen contract to define the mountpoint. To make the spec externally accessible a specload mountpoint in "executable" mode would be created that calls the application with a special command-line argument. If an application consists of one (or more) shared libraries in addition to the main executable, then the same mountpoint could be used internally and externally. Both would set system:/symbol/library to the library that contains the exported symbol.

@markus2330
Copy link
Contributor

What exactly makes the symbol mode support #4442 better?

Does the "file" mode make any sense if we remove the override/modification features?

@kodebach
Copy link
Member Author

The "file" mode still make sense, if somebody doesn't want to embed the whole spec into the application because of size concerns (e.g. embedded system with separate storage for executables (small) and data (large)).

If you use specload with #4442 to add a mountpoint just for this process, without "symbol" mode, you'd have to either have an external file ("file" mode) or run a second instance of the process ("app" mode). With "symbol" mode you can do everything within a single process and file/executable.

@markus2330
Copy link
Contributor

The "file" mode still make sense, if somebody doesn't want to embed the whole spec into the application because of size concerns (e.g. embedded system with separate storage for executables (small) and data (large)).

Which advantage would the file mode of specload have compared to a normal quickdump mountpoint?

With "symbol" mode you can do everything within a single process and file/executable.

So it is simply about replacing the interface execv with dlopen to get the spec? It might be faster but then the executable also must be a shared-lib/exe hybrid, which is afaik, not implemented in every libc (musl etc?). Is there any advantage except of a (not benchmarked) speed gain?

@kodebach
Copy link
Member Author

Which advantage would the file mode of specload have compared to a normal quickdump mountpoint?

That's true, without the special modification checks, the "file" mode is probably no longer needed.

It might be faster but then the executable also must be a shared-lib/exe hybrid, which is afaik, not implemented in every libc

AFAICT from dlopen(3) calling dlopen (NULL, ...) should just return a handle for the symbols in the current executable. In fact the POSIX standard says basically the same thing.

I haven't actually tested how well this works, but if it works well in some cases, I don't see why we shouldn't support it. It certainly makes things a lot simpler. In case it didn't come across, "symbol" mode would work without the dependency on quickdump since we'd directly call a function that allocates and returns a KeySet * within our process.

@markus2330
Copy link
Contributor

That's true, without the special modification checks, the "file" mode is probably no longer needed.

I am still not convinced that we should get rid of the modifications/overrides feature. It sounds so harmless to give admins a way to have their own description/notes/other docu for their config settings.

Adding more type information, should be also be ok as our type system only allows to make checks more and more strict.

Which problems do we have with this feature?

Is it so difficult to implement correctly?

from dlopen(3) calling dlopen (NULL, ...) should just return a handle for the symbols in the current executable.

Now I understand what you mean. Yes, this makes sense. So it is not a type to be used for mounting the specload plugin but it is exclusively for usage with kdbOpen(contract).

@kodebach
Copy link
Member Author

So it is not a type to be used for mounting the specload plugin but it is exclusively for usage with kdbOpen(contract).

This mode yes. But it would be easy to add an option do specify a library name, then you could use the "symbol" mode for mounting too. Like you said, you'd either needed a special shared object & executable file or you need a separate shared object that exports the spec, which is linked into the main executable. We don't have to add this. I just mentioned it, because it would be pretty easy to do.

Which problems do we have with this feature?

IIRC there were some possible problems with modifying types. Even one of the simplest cases of going from long_long to long could be an issue, if the application expects that it can write a long_long value to the key.

We could keep allowing modifications to comments etc., but IMO it's not worth the added complexity.

Is it so difficult to implement correctly?

If specload is a separate backend plugin, the override feature does impose a significant overhead. We'd have to deal with using a resolver (at least the part for atomic writes) and storage plugins. I don't think it really is worth the effort right now.

@markus2330
Copy link
Contributor

Ok, so we agree. To make specload simpler:

  • we completely remove the "file" feature
  • we completely remove all override/update features of existing specifications, i.e., the backend (=plugin) is read-only.

Then there are two config options for specload: symbol and executable (I like this name much more than app). Alternatively, we could use "file", which is required and specifies either an exe or a lib. The config option "symbol" is then optional and to be used together with "file". Using "file" to specify the file would have the advantage that the mounting code for file-based-backends would work for the specload backend.

@hannes99 Are you interested in implementing this?

@kodebach Can you update the top-post?

@kodebach
Copy link
Member Author

Using "file" to specify the file would have the advantage that the mounting code for file-based-backends would work for the specload backend.

Not necessarily. This depends on what else the mounting code does. But I wouldn't really worry about that. This new specload is so simple that creating the mountpoint without tooling is feasible and also creating custom tooling wouldn't be a lot of work.

@markus2330
Copy link
Contributor

Without knowing how exactly mounting tooling will look like, I wouldn't say anything about how much effort is needed to write tooling for a new backend plugin.

I hope our tooling handles the "file-resolving" and "noresolver" backends without any code. But let us see what @flo91 proposes.

@kodebach
Copy link
Member Author

kodebach commented Sep 9, 2022

Since specload will not support loading additional plugins, there is very little configuration possibility:

  1. Some way of choosing mode between symbol and executable
  2. Defining the name of the symbol/executable
  3. If symbol was chosen optionally, specify library from the symbol is loaded (if we even support this).

With so few options, I'm quite confident writing any tooling code should be very manageable.

However, what I was really referring to is, that specload is so simple that it can be configured without any kind of mount tooling just by using a kdb import system:/elektra/mountpoints/spec:\/mymountpoint ni with something like this:

plugins/backend/name = specload
definition/mode = symbol
definition/symbol = loadSpec
definition/symbol/library = libmyapp-spec.so

or

plugins/backend/name = specload
definition/mode = executable
definition/executable = /usr/bin/myapp

Note: The stuff below is not directly relevant to this issue, but I put it here, because it fits with the example above. If @flo91 aggrees the stuff below is a good approach, we should move it to a new issue to discuss it further.

Ideally, the foundation of the mount tooling would be based on this kind of import but do some extra validation. For example:

echo 'plugins/backend/name = specload
definition/mode = executable
definition/executable = /usr/bin/myapp' | kdb mount --raw=ni spec:/mymountpoint

This kdb mount --raw could mean:

  1. read the raw mountpoint definition from stdin with the ni plugin with parent key system:/elektra/mountpoints/spec:\/mymountpoint
  2. Check that the result is a theoretically valid mountpoint definition (e.g. contains plugins and definition sections, has a correctly set plugins/backend/name, etc.)
  3. Check that the definition is actually valid, i.e. call the init function of the defined backend plugin just like libelektra-kdb would, if we actually wrote the keys to system:/elektra/mountpoints
  4. If everything works, actually persist the mountpoint

With this foundation, any backend plugin can be mounted immediately without any special code. In most cases it would be quite annoying to do, but it would be possible. To make things more user friendly, backend plugins could export functions that are used to extend the kdb mount CLI. This could be based on elektraGetOpts (i.e. one exported function provides additional spec, another receives the parsed KeySet, similar to what was proposed in #4438) or it could be based on some other API, if you find something suitable.

Most importantly, I think, kdb mount --raw would be enough to merge new-backend into master. I would simply keep the old kdb mount (which works only with backend as the backend plugin), but rename it to kdb mount-old. This would give you more time to properly develop the API for the extensions to make the CLI more user-friendly. When the new kdb mount is complete, I would of course remove the kdb mount-old command.

@markus2330 markus2330 mentioned this issue Sep 28, 2022
20 tasks
@markus2330 markus2330 changed the title [new-backend] updates to specload remove specload plugin May 2, 2023
@markus2330
Copy link
Contributor

@hannes99 as discussed, specload should be removed.

@kodebach
Copy link
Member Author

kodebach commented May 2, 2023

May I ask why you want to remove entirely remove the plugin?

IMO (a biased opinion of course) the changes outlined at the top of this issue would have a fairly good use case and would also further the backend plugin system.

If the reason is simply time constraints, I would suggest excluding the plugin from packages and make install, but keeping the code in the repo until somebody can update the plugin. That could be done with a new flag for add_plugin that disables this install call in CMake:

install (
TARGETS ${PLUGIN_NAME}
DESTINATION lib${LIB_SUFFIX}/${TARGET_PLUGIN_FOLDER}
COMPONENT "${HAS_COMPONENT}")

The flag could be used for other unmaintained plugins too.

@markus2330
Copy link
Contributor

Simply removing non-maintained parts, if @hannes99 will use it in ckdb we can also keep it.

@hannes99 hannes99 mentioned this issue May 11, 2023
19 tasks
@markus2330
Copy link
Contributor

Seems like ckdb internally has spec, then it makes sense to keep it. (@hannes99)

Copy link

I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

@github-actions github-actions bot added the stale label May 14, 2024
Copy link

I closed this now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants