Parameterise mount path for longhorn volume #1800
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @garry-t, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the flexibility of the infrastructure setup by allowing the Longhorn volume mount path to be dynamically configured rather than being a fixed value. This change makes the deployment more adaptable to different environment requirements.
Highlights
- Longhorn Mount Path Parameterization: The hardcoded
/var/longhornmount path for Longhorn volumes has been replaced with a configurable variable,var.longhorn_mount_path, in the remote execution commands for agent configuration. - New Configuration Variable: A new optional string variable,
longhorn_mount_path, has been introduced to theagent_nodepoolsdefinition invariables.tf, providing a default value of/var/longhornfor backward compatibility.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to parameterize the mount path for Longhorn volumes, which is a valuable enhancement for flexibility. The changes in variables.tf correctly introduce the new longhorn_mount_path parameter as an optional attribute on agent nodepools, ensuring backward compatibility.
My review focuses on the agents.tf file, where I've identified a critical issue with how the new variable is referenced, which would cause the Terraform execution to fail. I've also provided suggestions to make the shell commands more robust and idempotent. Please note that for this feature to work as intended, you'll also need to update locals.tf to propagate the new variable, a change which is currently missing from this PR.
Overall, this is a good change that will be ready to merge after addressing the feedback.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to parameterize the Longhorn volume mount path. The changes involve modifications to agents.tf, locals.tf, and variables.tf to allow users to specify a custom mount path for Longhorn volumes. It's recommended to address the quoting of variables in shell commands to prevent potential command injection vulnerabilities.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the longhorn_mount_path variable, allowing parameterization of the Longhorn volume mount path. There's a critical issue in the validation logic and a high-severity issue in the script updating /etc/fstab. Suggestions have been provided to address these.
|
I found simpler option |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces parameterization of the Longhorn volume mount path. The implementation propagates the new longhorn_mount_path variable through the module's inputs, locals, and resources. I've provided feedback on the validation logic for the mount path and the idempotency of the command that updates /etc/fstab.
|
@garry-t why is this important to you? MicroOS is quite specific in the way that its file system works, allowing any other directory out of If you want to use volumes (shouldn't matter which kind, nvme, ...etc), you already can. Hence I'm closing this PR for now, please shout back at me if you really need it and explain why in details. |
|
@mysticaltech Never used MicroOS, but problem when we have quite big machine as storage, usually it comes with 100GB+ nvme disk which we can reuse as well in Longhorn. (Current implementation we can't , because stick to one path) |
|
@garry-t Ok, thanks for the detailed explanation. Then let's just add proper explanations in kube.tf.example along with the variable to let people know that they should stick to the /var directory. I've now reopened the PR, please feel free complete it whenever you can and I will merge if the review passes without issues. Thanks again for this! |
|
@mysticaltech understood will add instructions soon. |
|
@mysticaltech tell me if this doc format is ok. 10x |
|
@garry-t It's good, but please rebase with the current master and make sure the / gemini review passes please. |
added new variable
mount path now parameterised
added new variable
updated with suggested changes
fix syntax
commit suggestions
added variable validation
removed / from provisioner
fixed validation rule
updated validation rule for longhorn
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by allowing the Longhorn volume mount path to be parameterized. The changes are well-structured, including updates to Terraform logic, variables, example files, and new documentation.
My review has identified a critical issue in agents.tf where the idempotency check for /etc/fstab is broken, which would cause duplicate entries on every apply. I've also suggested a minor improvement to error visibility in the same file. Additionally, I've proposed a clarification in the new documentation to help users make more informed decisions about replica counts for their storage classes.
Overall, this is a great addition. Once the critical issue is addressed, this PR will be in excellent shape.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by allowing the Longhorn volume mount path to be parameterized via the longhorn_mount_path variable. The changes are well-implemented across the Terraform configuration, including updates to variables, local values, and the agent provisioning logic. The addition of input validation and documentation is also a great touch. My review focuses on improving the robustness of the shell command used for idempotency when updating /etc/fstab to prevent potential issues with special characters in file paths.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature to parameterize the Longhorn volume mount path. The implementation is robust, including idempotent shell commands in the remote-exec provisioner and comprehensive validation for the new path variable. The addition of documentation is also a great touch, making the feature easy to adopt. Overall, this is a high-quality contribution. I have one suggestion to improve the documentation example.
just to make gemini calm Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by allowing the Longhorn volume mount path to be parameterized. The changes to the provisioning scripts in agents.tf make them more robust and idempotent, which is a great improvement. The new documentation file is also very clear and helpful for users who want to leverage this new functionality.
However, I've identified a critical security vulnerability in the validation logic for the new longhorn_mount_path variable, which could allow for path traversal. I've also suggested an improvement to the example code in the new documentation to follow a more declarative Terraform pattern. Please see the detailed comments for specifics.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by allowing the Longhorn volume mount path to be parameterized. The implementation is robust, with good use of idempotent shell commands in the remote-exec provisioner and strong validation for the new path variable. The addition of documentation is also a great step. My main feedback is on the new documentation file, where an example could be clarified to avoid misleading users about the complexity of automating Longhorn node patching. Overall, this is a well-executed enhancement.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by allowing the Longhorn volume mount path to be parameterized. The implementation is robust, with solid improvements to the remote-exec scripts in agents.tf for idempotency and error handling, and a thorough validation rule for the new longhorn_mount_path variable. The addition of documentation is also a great touch.
I have one suggestion to improve the example in the new documentation to prevent potential user errors when dealing with more complex configurations. Overall, this is a well-executed change.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by allowing the Longhorn volume mount path to be parameterized. The changes are well-implemented across the Terraform configuration, including updates to variables, local values, and the agent provisioning script. The remote-exec script in agents.tf has been significantly improved for better idempotency and robustness by using mkdir -p, mountpoint, and an awk check for /etc/fstab entries. The addition of a new documentation file and validation for the mount path is also a great enhancement.
I have a couple of suggestions to further improve the code: one to make the new documentation example less error-prone for users, and another to refine the validation logic in variables.tf for better clarity and correctness.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to parameterize the mount path for Longhorn volumes, which is a great enhancement for flexibility. The implementation is solid, with robust shell commands for idempotency and new validation for the mount path. I've added a couple of suggestions: one to improve the new documentation by using a local variable in the example to prevent user error, and another to tighten the validation regex to prevent potentially dangerous configurations like mounting over the /var directory.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to parameterize the Longhorn volume mount path, a valuable feature for custom storage configurations. The implementation is excellent, not only adding the new parameter but also significantly improving the robustness and idempotency of the volume configuration script in agents.tf. The addition of strong input validation in variables.tf is a great measure to prevent configuration errors. Furthermore, the new documentation in docs/customize-mount-path-longhorn.md is clear, comprehensive, and will be very helpful for users. The changes are well-thought-out and of high quality. I have no specific issues to raise.
|
@mysticaltech gemini satisfied :) |
|
Thanks @garry-t ! I need to test it, will do ASAP |
2 participants
For #1799