Add module providing mount scripts for WEKA filesystems#3509
Conversation
|
/gcbrun |
c4db7e1 to
7faf6ae
Compare
|
@wiktorn you are marked as the assignee of this PR, if you are indeed ready for review, please feel free to assign it to me. |
cboneti
left a comment
There was a problem hiding this comment.
I have a few questions. PTAL.
Also, could you please consider adding a metadata.yaml file to this module? you can specify which APIs are required there and the toolkit will automatically verify these before attempting to deploy (look at the slurm modules for an example).
There was a problem hiding this comment.
Thanks for this contribution, @wiktorn! This module will be a great addition for WEKA users.
I've reviewed the code and have a few suggestions:
-
[optional] Shellcheck Disables: In
community/modules/file-system/weka-client/templates/mount-weka.sh.tftpl, could you add a comment above the block ofshellcheck disabledirectives? Just a brief note explaining that they are necessary because shellcheck doesn't recognize the Terraform template syntax (e.g.,${server_ip}). This will help future maintainers. -
README Index: Please add an entry for this new module to the main
modules/README.mdfile. Under the "### File System" section, add:* **[weka-client]** ![community-badge] ![experimental-badge] : Installs client and mounts [WEKA](https://www.weka.io/) filesystems.
And at the bottom of the "File System" links section, add:
[weka-client]: ../community/modules/file-system/weka-client/README.md
-
Testing: The level of detail in the README suggests this has been tested. Could you briefly confirm the scope of testing done against a live WEKA cluster?
The approach of using a systemd service for mounting seems correct given the dynamic information needed from the metadata server. The README is also very clear and helpful.
Review fixes: * add client_install_runner in Ansible to install WEKA * add mount_runner in Ansible to mount WEKA * update README - add timeout information * remove modules/file-system/filestore/scripts/mount.yaml as this file is unused
|
Thanks @cboneti , updated the README.md. As for the testing, I tested this module with a Slurm cluster on login nodes and compute nodes, as well as with compute-vm module. I did install a WEKA cluster version Tests were done using |
|
/gcbrun |
Support for mounting WEKA filesystems.
This module doesn't support creating WEKA storage, as this can be done by directly calling weka-gcp module.
Because WEKA require gathering environment information from Metadata server during mount it's not possible to create a fstab entry that will work on following mounts. Hence, the mount is done using SystemD unit.
Things for consideration / cleanup:
network_storage,client_install_runnerandmount_runneroutputs, or shall those be dropped, to prevent accidental useshellcheck disable, because shellcheck doesn't understand terraform templates, to be decided should shellcheck investigate templates as now, or whether add exclusion of template filesshould this PR include a blueprint that provisions the WEKA cluster and uses this module