Skip to content

CA-409431: Use an Observer forwarder for xapi-storage-script #6488

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

snwoods
Copy link
Contributor

@snwoods snwoods commented May 27, 2025

Currently, xapi-storage-script uses the presence/absence of a smapi observer config file to determine whether it should create traces. This only happens on startup which means smapiv3 traces will often not be created when they should be (e.g. if the smapi component is enabled after startup, tracing for smapiv3 will not be)

This commit updates the Smapi Observer forwarder to use an RPC client to send messages to xapi-storage-script, updating it on any relevant changes to the Observer. This is done in a generic way so that any future components could use also listen to this queue to call its own Observer functions.

Also move the Observer RPC declarations to a common file to reduce code duplication and make some debug logs more helpful.

snwoods added 2 commits May 27, 2025 11:22
Besides the errors, Xapi_cluster and Xenopsd use the exact same Observer
RPC definitions. Add a new Observer error (as the unique errors for
cluster/xenops are not applicable to the Observer functions anyway) and
use common code to remove this duplication.

Signed-off-by: Steven Woods <[email protected]>
@snwoods snwoods force-pushed the private/stevenwo/CA-409431 branch from 8e7ab20 to f63fbea Compare May 27, 2025 10:44
Currently, xapi-storage-script uses the presence/absence of a smapi
observer config file to determine whether it should create traces. This
only happens on startup which means smapiv3 traces will often not be
created when they should be.

This commit updates the Smapi Observer forwarder to use an RPC client to
send messages to xapi-storage-script, updating it on any relevant
changes to the Observer.

Signed-off-by: Steven Woods <[email protected]>
@snwoods snwoods force-pushed the private/stevenwo/CA-409431 branch from f63fbea to 4ab193a Compare May 27, 2025 10:49
@@ -25,7 +25,7 @@ verify-cert () {
}

mli-files () {
N=467
N=469
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can auto-generate an .mli file using Merlin/LSP server in your favourite editor.
Compile the ML file (or well at least dune build @check I think).
Create an empty .mli file, then use the 'Insert inferred interface' code action (how you trigger that depends on your editor). You can then refine the autogenerated interface by deleting values/modules/types that were not meant to be exposed if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure whether it needed an mli as I was following the example of storage_interface.ml and storage_skeleton.ml which don't have them. Is there a particular reason why they don't?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because historically generating mlis for long modules was difficult


let default_path = Filename.concat default_sockets_dir service_name

let uri () = "file:" ^ default_path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan; how about using the URI module? There is always some confusion around the // part in file URLs.

let rec retry_econnrefused f =
try f () with
| Unix.Unix_error (Unix.ECONNREFUSED, "connect", _) ->
(* debug "Caught ECONNREFUSED; retrying in 5s"; *)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the debug message would be useful; or are we seeing too many of these?

let create ctx ~dbg ~uuid ~name_label ~attributes ~endpoints ~enabled =
u "Observer.create"

let destroy ctx ~dbg ~uuid = u "Observer.destroy"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use u __FUNCTION__ but would prefer unimplemented __FUNCTION__

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