IH-397: Replace async with lwt in xapi-storage-script #6019
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
psafont
force-pushed
the
no-async
branch
2 times, most recently
from
2290f4d to
0fa5a1b
Compare
minglumlu
reviewed
Sep 27, 2024
minglumlu
reviewed
Sep 27, 2024
minglumlu
reviewed
Sep 27, 2024
edwintorok
reviewed
Sep 30, 2024
lindig
approved these changes
Sep 30, 2024
| Unix.environment () | ||
| |> Array.to_seq | ||
| |> Seq.map (fun kv -> | ||
| let k, v = Scanf.sscanf kv "%s@=%s" (fun k v -> (k, v)) in |
There was a problem hiding this comment.
This can fail with an exception that is not caught; however, Unix.environment should guarantee what we find this format.
There was a problem hiding this comment.
Reading the POSIX manual here is interesting. It is very careful with its wording, and says that by convention the environment has key=value pairs.
But nothing prevents you from doing this, in which case you don't get key=value pairs:
utop# Unix.create_process_env "./printenv" [||] [|"FOO"; "BAR=4"|] Unix.stdin Unix.stdout Unix.stderr;;
FOO
BAR=4
This is consistent with how /usr/bin/env works it also prints the environment without any sanitization or checks:
utop# Unix.create_process_env "/usr/bin/env" [||] [|"FOO"; "BAR=4"|] Unix.stdin Unix.stdout Unix.stderr;;
FOO
BAR=4
Python has a dictionary for os.environ and apparently it filters out environment variables that don't match the convention completely:
utop# Unix.create_process_env "/usr/bin/python3" [|"python3"; "-c"; "import os\nprint(os.environ)"|] [|"FOO";"BAR=4"|] Unix.stdin Unix.stdout Unix.stderr;;
environ({'BAR': '4', 'LC_CTYPE': 'C.UTF-8'})
Also note that putenv has a non-standard extension in Linux where calling putenv("FOO") removes FOO from the environment.
Probably not relevant in our case (since we are likely the ones calling these executables in the first place), but if we want to be fully generic then we can filter out or reject environment entries that are not key=value pairs.
I'd go with reject, that is a very unusual situation that is quite difficult to create (unless you deliberately call execve like I've done above), and probably an indication of a bug somewhere, and I think the current does that already.
robhoes
approved these changes
Sep 30, 2024
psafont
force-pushed
the
no-async
branch
3 times, most recently
from
fd8e63f to
ab54a08
Compare
Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Replace most of the usages with Stdlib, Sexplib0 or Base The String Hashtables, Sets, and Clocks will be replaced at a later time Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Replace it with Async_kernel wherever possible Useful to get familiar with the code and delimit which are its users Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
The error handling used in async is translated into Lwt_result type, this is because the base Lwt only allows exception, which isn't enough to code the errors produced. Logging has been changed to use logs with an asynchronous reporter. Now loglines print the loglevel. The setup of the loops / promises has been changed, the outer loop has been removed and its retry-on-error logic has been moved to the two inner watch loops. The inotify loop needed quite a few changes as async_inotify was doing complex handling of events, even consolidating them. This has been simplified by removing the watches on files and maintaining the directory one, and creating a list of commands, which the watch loops can act upon, like registering and deregistering plugins, for example. The file descriptor handling for communicating with launched processes needed workaround to be able to close stdin before the other channel without triggering an error at the end when all channels are closed. Error reporting was added to the smapiv2 rpc loop and will make the errors visible to xapi instead of failing silently. Co-developed-by: Edwin Török <edwin.torok@cloud.com> Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Avoid blowing up the stack when creating directories recursively. Now an optimistic approach is used: create the directory, and if if cannot be created try to create the parent. This avoid races in creation, but causes using 2 calls per directory created for all the directories that need to be created aside from the top-most. Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
use Fun.id instead Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Both client and server have code to pretty-print errors, and it's exactly the same as what was implemented here, reuse it instead. Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Removes async-only packages and changes the opam metadata to reflect the lack of dependencies on the libraries Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Separate the new functions into a private library, which can be tested Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
The number of actions for the plugin directories is reduced to two, reload all the directory's contents, or reload a file's contents. This means that when a file is modified is removed and readded to the plugins, and if the directory changed in any other way the whole directory is reloaded. The code also tries to reduce duplication in the plugin watchers as much as possible. Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
This allows for several events to accumulate between events to avoid doing too many reloads while files are being moved about, while still being responsive enough. Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
This is done by using Lwt_process.with_process_full, instead of manually managing them. Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows to remove all dependencies on async and core.
Compared to previous PRs, the new glue code has been put into a library and tests have been added, mainly Process.run. The directory watcher does not have unit tests.
This code has passed internal suites on Sept 20th:
Storage GFS2 BVT: 205277
Storage GFS2 Regression: 205278
Storage GFS2 Functional: 205279
Since then, I've rebased the branch and added more tests, but there were no conflicts to be resolved. The build made on top of latest xapi master is passed the toolstack suite:
Ring3 BST + BVT: 205582