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

CA-395512: process SMAPIv3 API calls concurrently #5807

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

edwintorok
Copy link
Contributor

By default message-switch calls are serialized for backwards compatibility reasons in the Lwt and Async modes. (We tried enabling parallel actions by default but got some hard to debug failures in the CI).

This causes very long VM start times when multiple VBDs are plugged/unplugged concurrently: the operations are seen concurrently by message-switch, but xapi-storage-script only retrieves and dispatches them sequentially, so any opportunity for parallel execution is lost. Even though the actions themselves only take seconds, due to serialization, a VM start may take minutes.

Enable parallel processing explicitly here (instead of for all message-switch clients). SMAPIv3 should expect to be called concurrently (on different hosts even), so in theory this change should be safe and improve performance.

However it'll require extensive testing as it may expose latent race conditions in SMAPIv3 implementations.

@edwintorok
Copy link
Contributor Author

A quick test shows that VM start with 10 disks takes 15s now instead of 59s. This will need a lot more testing, hence a draft PR (it may expose bugs elsewhere)l.

We should also figure out why turning this on by default everywhere breaks things (IIRC it only ever reproduced in the CI).

@edwintorok
Copy link
Contributor Author

We may want to introduce a config file flag here so we can more easily revert this if we find bugs in the field.

@lindig
Copy link
Contributor

lindig commented Jul 11, 2024

xapi-storage-script uses the Arg module for options - so this should be easy to add; an alternative would be to use an environment variable such that it would be easy to change this at runtime in case we want to turn it off. Otherwise we will need some config file entry for this.

@edwintorok
Copy link
Contributor Author

We have a config file already:

cat /etc/xapi-storage-script.conf
# Place to look for plugins
root = /usr/libexec/xapi-storage-script
state = /var/run/nonpersistent/xapi-storage-script/state.db

By default message-switch calls are serialized for backwards compatibility reasons in the Lwt and Async modes.
(We tried enabling parallel actions by default but got some hard to debug failures in the CI).

This causes very long VM start times when multiple VBDs are plugged/unplugged concurrently: the operations are seen concurrently by message-switch,
but xapi-storage-script only retrieves and dispatches them sequentially, so any opportunity for parallel execution is lost.
Even though the actions themselves only take seconds, due to serialization, a VM start may take minutes.

Enable parallel processing explicitly here (instead of for all message-switch clients).
SMAPIv3 should expect to be called concurrently (on different hosts even), so in theory this change should be safe
and improve performance, but there are some known bugs in SMAPIv3 plugins currently.

So introduce a config file flag 'concurrent' for now, that defaults to false,
but that can be turned to 'true' for testing purposes.
When all SMAPIv3 concurrency bugs are believed to be fixed we can flip the default,
and eventually remove this flag once no more bugs are reported.
The configuration value is done as a global to simplify integrating intot he Lwt port,
instead of changing a lot of functions to thread through an argument.

This doesn't change the behaviour of xapi-storage-script in its default configuration.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
@edwintorok edwintorok marked this pull request as ready for review July 16, 2024 16:50
@psafont
Copy link
Member

psafont commented Jul 22, 2024

This seems ready to merge, is anything missing?

@lindig
Copy link
Contributor

lindig commented Jul 22, 2024

I agree; would like to see this merged @edwintorok

@edwintorok edwintorok merged commit e60aeb5 into xapi-project:master Jul 22, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants