fix(preflight): check the system default NFS version in mount config #129
Conversation
ref: longhorn/longhorn 9830 Signed-off-by: Raphanus Lo <[email protected]>
WalkthroughThe pull request includes updates to the documentation files for the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
pkg/local/preflight/checker.go (1)
421-464: LGTM with a minor suggestion for error handling.The kernel config check implementation is thorough, checking both built-in and module-based NFS v4 support. Consider adding error logging when module check fails to help with troubleshooting.
moduleLoaded, err := utils.IsModuleLoaded(option[0]) if err != nil { + logrus.Debugf("Failed to check if module %s is loaded: %v", option[0], err) continue }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (17)
docs/longhornctl.md(1 hunks)docs/longhornctl_check.md(1 hunks)docs/longhornctl_check_preflight.md(1 hunks)docs/longhornctl_doc.md(1 hunks)docs/longhornctl_export.md(1 hunks)docs/longhornctl_export_replica.md(1 hunks)docs/longhornctl_export_replica_stop.md(1 hunks)docs/longhornctl_get.md(1 hunks)docs/longhornctl_get_replica.md(1 hunks)docs/longhornctl_global-options.md(1 hunks)docs/longhornctl_install.md(1 hunks)docs/longhornctl_install_preflight.md(1 hunks)docs/longhornctl_install_preflight_stop.md(1 hunks)docs/longhornctl_trim.md(1 hunks)docs/longhornctl_trim_volume.md(1 hunks)docs/longhornctl_version.md(1 hunks)pkg/local/preflight/checker.go(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/longhornctl.md
[uncategorized] ~29-~29: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...d) - Print longhornctl version ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_check.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...md) - Check Longhorn preflight ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_check_preflight.md
[uncategorized] ~60-~60: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... - Longhorn checking operations ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_doc.md
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_export.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ....md) - Export Longhorn replica ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_export_replica.md
[uncategorized] ~65-~65: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Stop exporting Longhorn replica ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_export_replica_stop.md
[uncategorized] ~34-~34: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ....md) - Export Longhorn replica ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_get.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...et Longhorn replica information ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_get_replica.md
[uncategorized] ~63-~63: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...nformation gathering operations ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_global-options.md
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_install.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...) - Install Longhorn preflight ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_install_preflight.md
[uncategorized] ~48-~48: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...op Longhorn preflight installer ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_install_preflight_stop.md
[uncategorized] ~35-~35: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...) - Install Longhorn preflight ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_trim.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...e.md) - Trim a Longhorn volume ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_trim_volume.md
[uncategorized] ~45-~45: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... - Longhorn trimming operations ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_version.md
[uncategorized] ~27-~27: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 25-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
docs/longhornctl.md
29-29: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_check.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_check_preflight.md
60-60: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_doc.md
31-31: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_export.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_export_replica.md
65-65: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_export_replica_stop.md
34-34: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_get.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_get_replica.md
63-63: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_global-options.md
31-31: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_install.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_install_preflight.md
48-48: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_install_preflight_stop.md
35-35: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_trim.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_trim_volume.md
45-45: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_version.md
27-27: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🔇 Additional comments (3)
pkg/local/preflight/checker.go (3)
465-468: LGTM!
Proper error handling with descriptive error wrapping.
499-505: LGTM!
The logging implementation provides clear and helpful messages, especially the warning that guides users to the manual page when NFS4 isn't the default version.
469-505: Verify behavior with different nfsmount.conf configurations.
Let's verify the implementation handles various nfsmount.conf configurations correctly.
…onfig Signed-off-by: Raphanus Lo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (15)
docs/longhornctl_trim.md (1)
19-19: Documentation structure: Consider adjusting the heading levelThe auto-generated section uses h6 (######) but should use h4 (####) to maintain proper heading hierarchy, as headings should only increment by one level at a time.
-###### Auto generated by spf13/cobra on 26-Nov-2024 +#### Auto generated by spf13/cobra on 26-Nov-2024🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...e.md) - Trim a Longhorn volume ###### Auto generated by spf13/cobra on 26-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
docs/longhornctl_export.md (1)
19-19: LGTM - Auto-generated documentation updateThis is an automatically generated documentation file, and the timestamp update is expected. No manual changes are needed as this file is managed by spf13/cobra.
Note: While there are some minor markdown formatting inconsistencies (heading levels), these are part of the auto-generation process and should be addressed at the generator level if needed.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ....md) - Export Longhorn replica ###### Auto generated by spf13/cobra on 26-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
docs/longhornctl_get.md (1)
19-19: Consider improving documentation formattingFor better documentation consistency:
- The heading level should increment by only one level (use
####instead of######)- "Auto generated" should be hyphenated as "Auto-generated"
-###### Auto generated by spf13/cobra on 26-Nov-2024 +#### Auto-generated by spf13/cobra on 26-Nov-2024🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...et Longhorn replica information ###### Auto generated by spf13/cobra on 26-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
docs/longhornctl_install.md (2)
19-19: Fix auto-generated footer styleThe auto-generated footer has two style issues:
- Incorrect heading level (h6)
- Missing hyphen in "Auto generated"
Apply this diff to fix the style issues:
-###### Auto generated by spf13/cobra on 26-Nov-2024 +#### Auto-generated by spf13/cobra on 26-Nov-2024🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...) - Install Longhorn preflight ###### Auto generated by spf13/cobra on 26-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
Documentation needs to be updated to reflect NFS version configuration checks
The codebase shows that NFS version checks have been implemented in
pkg/local/preflight/checker.go, including verification of/etc/nfsmount.confand protocol version requirements. However, this important information is not documented in eitherlonghornctl_install.mdorlonghornctl_check_preflight.md. The preflight documentation should be updated to:
- Mention the NFS version requirements (v4, 4.1, or 4.2)
- Document the
/etc/nfsmount.confconfiguration check- Include the new warning message that appears when NFS version is not properly configured
🔗 Analysis chain
Line range hint
1-18: Documentation updates for NFS version checks are missingThe PR objectives mention updating the documentation to reflect NFS version checking in
/etc/nfsmount.conf, but these changes are not present in this documentation file.Let's check if NFS-related documentation exists elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for NFS-related documentation files # Expected: Find documentation files that might need updates for NFS version checks # Search for NFS-related markdown files fd -e md -x grep -l "NFS" {} # Search for NFS-related documentation in other text files fd -e txt -e rst -x grep -l "NFS" {}Length of output: 107
Script:
#!/bin/bash # Let's check the content of the NFS-related documentation file cat ./docs/longhornctl_check_preflight.mdLength of output: 2015
Script:
#!/bin/bash # Let's check for any NFS version or mount configuration related changes in the codebase rg -i "nfs.*vers|nfsmount\.conf" -g '!{*.md,*.txt}'Length of output: 489
🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...) - Install Longhorn preflight ###### Auto generated by spf13/cobra on 26-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
16-16: Column: 32
Hard tabs(MD010, no-hard-tabs)
17-17: Column: 68
Hard tabs(MD010, no-hard-tabs)
docs/longhornctl_version.md (1)
27-27: Fix documentation formatting issues.There are two minor formatting improvements needed:
- "Auto generated" should be hyphenated as "Auto-generated"
- The heading level should be h4 (####) instead of h6 (######)
Apply this diff:
-###### Auto generated by spf13/cobra on 26-Nov-2024 +#### Auto-generated by spf13/cobra on 26-Nov-2024🧰 Tools
🪛 LanguageTool
[uncategorized] ~27-~27: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 26-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
27-27: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
docs/longhornctl_doc.md (1)
31-31: Documentation formatting improvements needed.There are two formatting issues that should be addressed:
- "Auto generated" should be hyphenated as "Auto-generated"
- The heading level (###### h6) violates markdown hierarchy - it should be #### (h4) to maintain proper heading structure
Apply this diff to fix both issues:
-###### Auto generated by spf13/cobra on 26-Nov-2024 +#### Auto-generated by spf13/cobra on 26-Nov-2024🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 26-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
31-31: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
docs/longhornctl_global-options.md (1)
31-31: Fix markdown formatting issuesThere are two formatting issues to address:
- "Auto generated" should be hyphenated to "Auto-generated"
- The heading level should be h4 (####) instead of h6 (######)
-###### Auto generated by spf13/cobra on 26-Nov-2024 +#### Auto-generated by spf13/cobra on 26-Nov-2024🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 26-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
31-31: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
docs/longhornctl_export_replica_stop.md (1)
34-34: Note: Auto-generated documentation with minor formatting issuesThis is an auto-generated documentation file. There are some minor formatting issues in the generated content:
- "Auto generated" should be hyphenated as "Auto-generated"
- The heading level jumps from h1 to h6, which violates markdown best practices
Since these issues are in auto-generated content, they should be addressed in the upstream spf13/cobra project rather than fixed manually here.
Consider opening an issue in the spf13/cobra repository to improve the documentation generation formatting.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~34-~34: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ....md) - Export Longhorn replica ###### Auto generated by spf13/cobra on 26-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
34-34: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
docs/longhornctl_install_preflight_stop.md (1)
35-35: Fix markdown formatting issuesThe auto-generated timestamp line has some formatting issues:
- The heading level jumps from h3 to h6 (######). It should use h4 (####) for proper hierarchy.
- "Auto generated" should be hyphenated as "Auto-generated".
Apply this diff to fix the formatting:
-###### Auto generated by spf13/cobra on 26-Nov-2024 +#### Auto-generated by spf13/cobra on 26-Nov-2024🧰 Tools
🪛 LanguageTool
[uncategorized] ~35-~35: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...) - Install Longhorn preflight ###### Auto generated by spf13/cobra on 26-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
35-35: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
docs/longhornctl_trim_volume.md (1)
45-45: Consider fixing markdown formatting issues.
- Add a hyphen to "Auto-generated" for consistency
- Use the correct heading level (h4) instead of h6
Apply this diff to fix the formatting:
-###### Auto generated by spf13/cobra on 26-Nov-2024 +#### Auto-generated by spf13/cobra on 26-Nov-2024🧰 Tools
🪛 LanguageTool
[uncategorized] ~45-~45: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... - Longhorn trimming operations ###### Auto generated by spf13/cobra on 26-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
45-45: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
docs/longhornctl_check_preflight.md (1)
Line range hint
1-59: Documentation needs update regarding NFS version checkingAccording to the PR objectives, the documentation should be updated to reflect the changes in NFS version checking functionality, particularly regarding the
/etc/nfsmount.conffile. Consider adding this information to the command's documentation to help users understand how the preflight checks verify NFS version support.Would you like me to help draft the additional documentation content about NFS version checking?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~60-~60: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... - Longhorn checking operations ###### Auto generated by spf13/cobra on 26-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
60-60: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
58-58: Column: 44
Hard tabs(MD010, no-hard-tabs)
docs/longhornctl_install_preflight.md (1)
48-48: Consider fixing the hyphenation in "Auto generated"The phrase "Auto generated" should be hyphenated as it's used as a compound adjective.
-###### Auto generated by spf13/cobra on 26-Nov-2024 +###### Auto-generated by spf13/cobra on 26-Nov-2024However, since this is auto-generated by cobra, you might want to raise this upstream with the cobra project instead.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~48-~48: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...op Longhorn preflight installer ###### Auto generated by spf13/cobra on 26-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
48-48: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
docs/longhornctl_export_replica.md (1)
65-65: Fix hyphenation in the footer textThe term "Auto generated" should be hyphenated as "Auto-generated" to follow proper English conventions.
-###### Auto generated by spf13/cobra on 26-Nov-2024 +###### Auto-generated by spf13/cobra on 26-Nov-2024🧰 Tools
🪛 LanguageTool
[uncategorized] ~65-~65: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Stop exporting Longhorn replica ###### Auto generated by spf13/cobra on 26-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
65-65: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
pkg/local/preflight/checker.go (1)
481-497: Enhance config parsing robustnessThe current implementation could be improved in several ways:
- Add comment handling (lines starting with #)
- Make section parsing more robust
- Add validation for version format
for nfsMountConfigScanner.Scan() { line := strings.TrimSpace(nfsMountConfigScanner.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } if strings.HasPrefix(line, "[") { - section = strings.TrimSpace(strings.Trim(line, "[]")) + section = strings.Trim(strings.TrimSpace(line), "[]") + if section == "" { + continue + } } else if section == "NFSMount_Global_Options" { if key, val, isParsable := strings.Cut(line, "="); isParsable { + key = strings.TrimSpace(key) + val = strings.TrimSpace(val) switch key { case "Defaultvers": verParts := strings.SplitN(val, ".", 2) + if len(verParts) == 0 { + continue + } isDefaultV4 = verParts[0] == "4" if len(verParts) == 2 && isDefaultV4 { isDefaultV4 = verParts[1] == "0" || verParts[1] == "1" || verParts[1] == "2" } } } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (17)
docs/longhornctl.md(1 hunks)docs/longhornctl_check.md(1 hunks)docs/longhornctl_check_preflight.md(1 hunks)docs/longhornctl_doc.md(1 hunks)docs/longhornctl_export.md(1 hunks)docs/longhornctl_export_replica.md(1 hunks)docs/longhornctl_export_replica_stop.md(1 hunks)docs/longhornctl_get.md(1 hunks)docs/longhornctl_get_replica.md(1 hunks)docs/longhornctl_global-options.md(1 hunks)docs/longhornctl_install.md(1 hunks)docs/longhornctl_install_preflight.md(1 hunks)docs/longhornctl_install_preflight_stop.md(1 hunks)docs/longhornctl_trim.md(1 hunks)docs/longhornctl_trim_volume.md(1 hunks)docs/longhornctl_version.md(1 hunks)pkg/local/preflight/checker.go(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/longhornctl.md
[uncategorized] ~29-~29: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...d) - Print longhornctl version ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_check.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...md) - Check Longhorn preflight ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_check_preflight.md
[uncategorized] ~60-~60: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... - Longhorn checking operations ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_doc.md
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_export.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ....md) - Export Longhorn replica ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_export_replica.md
[uncategorized] ~65-~65: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Stop exporting Longhorn replica ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_export_replica_stop.md
[uncategorized] ~34-~34: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ....md) - Export Longhorn replica ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_get.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...et Longhorn replica information ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_get_replica.md
[uncategorized] ~63-~63: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...nformation gathering operations ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_global-options.md
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_install.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...) - Install Longhorn preflight ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_install_preflight.md
[uncategorized] ~48-~48: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...op Longhorn preflight installer ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_install_preflight_stop.md
[uncategorized] ~35-~35: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...) - Install Longhorn preflight ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_trim.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...e.md) - Trim a Longhorn volume ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_trim_volume.md
[uncategorized] ~45-~45: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... - Longhorn trimming operations ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_version.md
[uncategorized] ~27-~27: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
docs/longhornctl.md
29-29: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_check.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_check_preflight.md
60-60: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_doc.md
31-31: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_export.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_export_replica.md
65-65: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_export_replica_stop.md
34-34: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_get.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_get_replica.md
63-63: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_global-options.md
31-31: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_install.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_install_preflight.md
48-48: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_install_preflight_stop.md
35-35: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_trim.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_trim_volume.md
45-45: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_version.md
27-27: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🔇 Additional comments (11)
docs/longhornctl_check.md (1)
19-19: LGTM - Auto-generated documentation update
The timestamp update is correct and expected as part of the auto-generated documentation.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...md) - Check Longhorn preflight ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_get.md (1)
19-19: LGTM: Auto-generated timestamp update
The timestamp update is an expected automated change from the cobra CLI tool.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...et Longhorn replica information ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_version.md (1)
27-27: LGTM: Timestamp update is consistent.
The timestamp update aligns with the changes mentioned in the AI summary.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~27-~27: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
27-27: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_doc.md (1)
31-31: LGTM: Timestamp update is correct.
The auto-generated timestamp has been updated appropriately.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
31-31: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_global-options.md (1)
Line range hint 1-31: Documentation updates needed for NFS version checking feature
The documentation doesn't reflect the changes mentioned in the PR objectives regarding NFS version checking in mount config. Consider adding information about:
- The importance of checking
/etc/nfsmount.conf - How to verify the NFS version configuration
- Impact on backup-related features
Would you like me to help draft the documentation updates for the NFS version checking feature?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
31-31: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
29-29: Column: 32
Hard tabs
(MD010, no-hard-tabs)
docs/longhornctl.md (1)
29-29: LGTM: Auto-generated timestamp update
The timestamp update is correct and consistent with the documentation regeneration.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...d) - Print longhornctl version ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
29-29: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_trim_volume.md (1)
45-45: LGTM on the timestamp update.
The auto-generated timestamp has been updated appropriately.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~45-~45: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... - Longhorn trimming operations ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
45-45: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_check_preflight.md (1)
60-60: LGTM!
The auto-generated timestamp update is correct.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~60-~60: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... - Longhorn checking operations ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
60-60: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_get_replica.md (1)
63-63: LGTM!
The timestamp update is a routine change for auto-generated documentation.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~63-~63: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...nformation gathering operations ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
63-63: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_export_replica.md (1)
65-65: LGTM on the timestamp update!
The auto-generated timestamp has been updated as expected.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~65-~65: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Stop exporting Longhorn replica ###### Auto generated by spf13/cobra on 26-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
65-65: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
pkg/local/preflight/checker.go (1)
507-513: LGTM! Clear and informative status messages
The result handling provides clear feedback with appropriate error levels and helpful guidance for users.
pkg/local/preflight/checker.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| kernelConfigPath := "/boot/config-" + kernelVersion | ||
| kernelConfigPath = filepath.Join(consts.VolumeMountHostDirectory, kernelConfigPath) | ||
| configFile, err := os.Open(kernelConfigPath) | ||
| kernelConfigFile, err := os.Open(kernelConfigPath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer func(configFile *os.File) { | ||
| _ = configFile.Close() | ||
| }(configFile) | ||
| }(kernelConfigFile) | ||
|
|
||
| scanner := bufio.NewScanner(configFile) | ||
| kernelConfigScanner := bufio.NewScanner(kernelConfigFile) | ||
|
|
||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| for kernelConfigScanner.Scan() { | ||
| line := kernelConfigScanner.Text() | ||
| if strings.HasPrefix(line, "CONFIG_NFS_V4_2=") || | ||
| strings.HasPrefix(line, "CONFIG_NFS_V4_1=") || | ||
| strings.HasPrefix(line, "CONFIG_NFS_V4=") { | ||
| option := strings.Split(line, "=") | ||
| if len(option) == 2 { | ||
| if option[1] == "y" { | ||
| local.collection.Log.Info = append(local.collection.Log.Info, "NFS4 is supported") | ||
| return nil | ||
| isKernelSupport = true | ||
| break | ||
| } else if option[1] == "m" { | ||
| // Check if the module is loaded | ||
| moduleLoaded, err := utils.IsModuleLoaded(option[0]) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| if moduleLoaded { | ||
| local.collection.Log.Info = append(local.collection.Log.Info, "NFS4 is supported") | ||
| return nil | ||
| isKernelSupport = true | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if err := scanner.Err(); err != nil { | ||
| if err := kernelConfigScanner.Err(); err != nil { | ||
| return errors.Wrap(err, "failed to check NFS4 support") | ||
| } | ||
|
|
||
| local.collection.Log.Error = append(local.collection.Log.Error, "NFS4 is not supported") | ||
| // NFS default mount settings. Reference: man 5 nfsmount.conf | ||
| nfsMountConfigPath := "/etc/nfsmount.conf" | ||
| nfsMountConfigPath = filepath.Join(consts.VolumeMountHostDirectory, nfsMountConfigPath) | ||
| nfsMountConfigFile, err := os.Open(nfsMountConfigPath) | ||
| if err == nil { | ||
| defer func(configFile *os.File) { | ||
| _ = configFile.Close() | ||
| }(nfsMountConfigFile) | ||
|
|
||
| nfsMountConfigScanner := bufio.NewScanner(nfsMountConfigFile) | ||
| var section = "" | ||
|
|
||
| for nfsMountConfigScanner.Scan() { | ||
| line := strings.TrimSpace(nfsMountConfigScanner.Text()) | ||
| if strings.HasPrefix(line, "[") { | ||
| section = strings.TrimSpace(strings.Trim(line, "[]")) | ||
| } else if section == "NFSMount_Global_Options" { | ||
| if key, val, isParsable := strings.Cut(line, "="); isParsable { | ||
| switch key { | ||
| case "Defaultvers": | ||
| verParts := strings.SplitN(val, ".", 2) | ||
| isDefaultV4 = verParts[0] == "4" | ||
| if len(verParts) == 2 && isDefaultV4 { | ||
| isDefaultV4 = verParts[1] == "0" || verParts[1] == "1" || verParts[1] == "2" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if err = nfsMountConfigScanner.Err(); err != nil { | ||
| return errors.Wrap(err, "failed to scan NFS mount config") | ||
| } | ||
| } | ||
| if err != nil && !os.IsNotExist(err) { | ||
| return errors.Wrap(err, "failed to check NFS default mount configurations") | ||
| } | ||
|
|
||
| if !isKernelSupport { | ||
| local.collection.Log.Error = append(local.collection.Log.Error, "NFS4 is not supported") | ||
| } else if !isDefaultV4 { | ||
| local.collection.Log.Warn = append(local.collection.Log.Warn, "NFS4 is supported, but default protocol version is not 4, 4.1, or 4.2. Please refer to the NFS mount configuration manual page for more information: man 5 nfsmount.conf") | ||
| } else { | ||
| local.collection.Log.Info = append(local.collection.Log.Info, "NFS4 is supported") | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for NFS version configuration checks
The code changes introduce new functionality to check NFS version support and configuration, but there are no corresponding test files in the codebase. This functionality requires testing of:
- Kernel configuration parsing for NFS version support
- NFS mount configuration file parsing
- Different combinations of kernel support and mount configurations
- Error handling for missing or malformed configuration files
Please add unit tests to cover:
- Different NFS version settings in nfsmount.conf
- Missing nfsmount.conf scenarios
- Various kernel configurations (built-in vs module)
- Error cases for file operations and parsing
🔗 Analysis chain
Verify testing coverage for different NFS configurations
Please ensure that the changes are tested with various NFS configurations:
- Different NFS version settings in nfsmount.conf
- Missing nfsmount.conf file
- Different kernel configurations (built-in vs module)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for test files covering NFS version checks
rg -l "TestCheckNFSv4Support" -g "**/*_test.go"
# Check for existing NFS version tests in the codebase
ast-grep --pattern 'func Test$$$NFSv4$$$($$$) {
$$$
}'
Length of output: 255
Script:
#!/bin/bash
# Let's try a broader search for test files
fd "_test.go$" | xargs rg -l "nfs|NFS"
# Check for any test functions related to preflight checks
fd "_test.go$" | xargs rg "func Test.*Preflight" -l
# Look for test files in the preflight package directory
fd "_test.go$" "pkg/local/preflight"
Length of output: 4777
Script:
#!/bin/bash
# Let's check the content of the preflight package for any existing tests
fd "_test.go$" "pkg/local/preflight" -x cat {}
# Look for any test utilities or mock data related to kernel/NFS configuration
rg -l "TestData|testdata|mock" "pkg/local/preflight"
# Check if there are any test utilities for kernel version or module checks
rg -l "TestGetKernelVersion|TestIsModuleLoaded" "pkg/utils"
Length of output: 161
…check Signed-off-by: Raphanus Lo <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (12)
docs/longhornctl_export.md (1)
19-19: Consider standardizing heading hierarchyThe auto-generated heading uses h6 (######) which skips several heading levels. While this is auto-generated content, it would be good to update the template to use a more appropriate heading level (h4) for better document structure.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ....md) - Export Longhorn replica ###### Auto generated by spf13/cobra on 27-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
docs/longhornctl_get.md (1)
19-19: LGTM: Timestamp update is consistentThe auto-generated timestamp has been updated appropriately.
Consider these formatting improvements:
- Use heading level 4 (
####) instead of level 6 (######) for consistency- Add a hyphen to "Auto-generated" for proper formatting
-###### Auto generated by spf13/cobra on 27-Nov-2024 +#### Auto-generated by spf13/cobra on 27-Nov-2024🧰 Tools
🪛 Markdownlint (0.35.0)
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
🪛 LanguageTool
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...et Longhorn replica information ###### Auto generated by spf13/cobra on 27-Nov-2024(AUTO_HYPHEN)
docs/longhornctl_doc.md (1)
31-31: Note: This is an auto-generated fileThe changes look good. However, since this is auto-generated documentation, any improvements should be made to the generator configuration rather than directly editing this file.
Consider updating the spf13/cobra generator configuration to:
- Add a hyphen in "Auto-generated"
- Use consistent heading levels (h4 instead of h6 for the timestamp)
🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 27-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
31-31: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
docs/longhornctl_global-options.md (1)
31-31: Fix markdown heading level and hyphenation
- The heading level should be h4 (####) instead of h6 (######) to maintain proper hierarchy
- "Auto generated" should be hyphenated as "Auto-generated"
Apply this diff:
-###### Auto generated by spf13/cobra on 27-Nov-2024 +#### Auto-generated by spf13/cobra on 27-Nov-2024🧰 Tools
🪛 Markdownlint (0.35.0)
31-31: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
🪛 LanguageTool
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 27-Nov-2024(AUTO_HYPHEN)
docs/longhornctl_export_replica_stop.md (1)
34-34: Consider addressing auto-generation formatting in upstream cobra.The auto-generated footer has some formatting issues that should be addressed in the upstream spf13/cobra generator:
- The heading level jumps from h1 to h6
- "Auto generated" should be hyphenated as "Auto-generated"
Since this is auto-generated content, these issues should be fixed in the generator rather than manually.
Would you like me to help create upstream issues for these formatting improvements in the spf13/cobra repository?
🧰 Tools
🪛 Markdownlint (0.35.0)
34-34: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
🪛 LanguageTool
[uncategorized] ~34-~34: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ....md) - Export Longhorn replica ###### Auto generated by spf13/cobra on 27-Nov-2024(AUTO_HYPHEN)
docs/longhornctl_trim_volume.md (1)
45-45: Fix markdown heading level and hyphenation
- The heading level jumps from h1 to h6 which violates markdown best practices
- "Auto generated" should be hyphenated as "Auto-generated"
Apply this diff to fix both issues:
-###### Auto generated by spf13/cobra on 27-Nov-2024 +#### Auto-generated by spf13/cobra on 27-Nov-2024🧰 Tools
🪛 Markdownlint (0.35.0)
45-45: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
🪛 LanguageTool
[uncategorized] ~45-~45: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... - Longhorn trimming operations ###### Auto generated by spf13/cobra on 27-Nov-2024(AUTO_HYPHEN)
docs/longhornctl_check_preflight.md (2)
60-60: Fix markdown formatting issues.Please address the following formatting issues:
- Add a hyphen to "Auto-generated" as it's a compound adjective
- Use the correct heading level (h4) instead of h6 for consistency
-###### Auto generated by spf13/cobra on 27-Nov-2024 +#### Auto-generated by spf13/cobra on 27-Nov-2024🧰 Tools
🪛 LanguageTool
[uncategorized] ~60-~60: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... - Longhorn checking operations ###### Auto generated by spf13/cobra on 27-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
60-60: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
Line range hint
1-60: Documentation needs to be updated to reflect the new NFS version checking behavior.The documentation should be updated to reflect the enhanced NFS version checking that considers the system's default NFS version from
/etc/nfsmount.conf. This will help users understand:
- How the NFS version check works
- What configuration settings are checked
- How to interpret the preflight check results
The example output should also be updated to show the more detailed NFS version check results.
Would you like me to help draft the updated documentation that includes these details?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~60-~60: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... - Longhorn checking operations ###### Auto generated by spf13/cobra on 27-Nov-2024(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
60-60: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
58-58: Column: 44
Hard tabs(MD010, no-hard-tabs)
docs/longhornctl_install_preflight.md (1)
Documentation update needed for NFS version configuration
The preflight documentation should be updated to include NFS version configuration details since:
- The code checks
/etc/nfsmount.conffor NFS mount settings- Current documentation only shows "NFS4 is supported" in the output but lacks configuration guidance
- No other documentation files contain information about NFS version configuration
Suggested additions to
docs/longhornctl_install_preflight.md:
- Add a note about NFS version configuration under "Synopsis"
- Include information about
/etc/nfsmount.confin the documentation- Explain that NFS4 is the default version when the config file is absent
🔗 Analysis chain
Line range hint
1-48: Consider adding NFS version check documentation.Given that this PR introduces changes to NFS version checking functionality, consider adding documentation about:
- The importance of checking
/etc/nfsmount.conf- How to verify and configure the correct NFS version
- The impact on backup operations
This aligns with the PR objective to "Update the official documentation to reflect the need to check the
/etc/nfsmount.conffile".Let's check if NFS-related documentation exists elsewhere:
Let me check the content of the found files to better understand the NFS documentation coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for NFS-related documentation # Expected: Documentation about NFS version checks and configuration # Search for NFS-related markdown files echo "Searching for NFS documentation files..." fd -e md | xargs rg -l 'nfs|NFS' # Search for specific mentions of nfsmount.conf echo "Searching for nfsmount.conf mentions..." rg '/etc/nfsmount.conf'Length of output: 349
Script:
#!/bin/bash # Check content of NFS-related documentation and code echo "Content of longhornctl_check_preflight.md:" cat docs/longhornctl_check_preflight.md echo -e "\nContent of relevant code section:" rg -A 10 -B 10 '/etc/nfsmount.conf' pkg/local/preflight/checker.goLength of output: 2990
🧰 Tools
🪛 Markdownlint (0.35.0)
48-48: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
45-45: Column: 48
Hard tabs(MD010, no-hard-tabs)
46-46: Column: 78
Hard tabs(MD010, no-hard-tabs)
🪛 LanguageTool
[uncategorized] ~48-~48: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...op Longhorn preflight installer ###### Auto generated by spf13/cobra on 27-Nov-2024(AUTO_HYPHEN)
docs/longhornctl_export_replica.md (1)
65-65: Fix documentation formatting issues.Please address the following minor documentation issues:
- The heading level should be h4 (####) instead of h6 (######)
- "Auto generated" should be hyphenated as "Auto-generated"
Apply this diff:
-###### Auto generated by spf13/cobra on 27-Nov-2024 +#### Auto-generated by spf13/cobra on 27-Nov-2024🧰 Tools
🪛 Markdownlint (0.35.0)
65-65: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time(MD001, heading-increment)
🪛 LanguageTool
[uncategorized] ~65-~65: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Stop exporting Longhorn replica ###### Auto generated by spf13/cobra on 27-Nov-2024(AUTO_HYPHEN)
pkg/local/preflight/checker.go (2)
440-464: Consider enhancing error handling and logging for module checks.While the logic is correct, the error handling for module checks could be improved to provide better diagnostics.
Apply this change to improve error handling:
moduleLoaded, err := utils.IsModuleLoaded(option[0]) if err != nil { + local.collection.Log.Warn = append(local.collection.Log.Warn, + fmt.Sprintf("Failed to check if module %s is loaded: %v", option[0], err)) continue }
473-517: Enhance config parsing robustness.While the version validation is thorough, the section parsing could be more robust:
Apply these changes:
- if key, val, isParsable := strings.Cut(line, "="); isParsable { + key, val, isParsable := strings.Cut(line, "=") + if isParsable { + key = strings.TrimSpace(key) + val = strings.TrimSpace(val) switch key { case "Defaultvers": verParts := strings.SplitN(val, ".", 2)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (17)
docs/longhornctl.md(1 hunks)docs/longhornctl_check.md(1 hunks)docs/longhornctl_check_preflight.md(1 hunks)docs/longhornctl_doc.md(1 hunks)docs/longhornctl_export.md(1 hunks)docs/longhornctl_export_replica.md(1 hunks)docs/longhornctl_export_replica_stop.md(1 hunks)docs/longhornctl_get.md(1 hunks)docs/longhornctl_get_replica.md(1 hunks)docs/longhornctl_global-options.md(1 hunks)docs/longhornctl_install.md(1 hunks)docs/longhornctl_install_preflight.md(1 hunks)docs/longhornctl_install_preflight_stop.md(1 hunks)docs/longhornctl_trim.md(1 hunks)docs/longhornctl_trim_volume.md(1 hunks)docs/longhornctl_version.md(1 hunks)pkg/local/preflight/checker.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
pkg/local/preflight/checker.go (1)
Learnt from: COLDTURNIP
PR: longhorn/cli#129
File: pkg/local/preflight/checker.go:421-423
Timestamp: 2024-11-26T10:35:55.842Z
Learning: In `/etc/nfsmount.conf`, the `Defaultvers` parameter is set to `4` by default.
🪛 LanguageTool
docs/longhornctl.md
[uncategorized] ~29-~29: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...d) - Print longhornctl version ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_check.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...md) - Check Longhorn preflight ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_check_preflight.md
[uncategorized] ~60-~60: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... - Longhorn checking operations ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_doc.md
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_export.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ....md) - Export Longhorn replica ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_export_replica.md
[uncategorized] ~65-~65: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Stop exporting Longhorn replica ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_export_replica_stop.md
[uncategorized] ~34-~34: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ....md) - Export Longhorn replica ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_get.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...et Longhorn replica information ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_get_replica.md
[uncategorized] ~63-~63: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...nformation gathering operations ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_global-options.md
[uncategorized] ~31-~31: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_install.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...) - Install Longhorn preflight ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_install_preflight.md
[uncategorized] ~48-~48: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...op Longhorn preflight installer ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_install_preflight_stop.md
[uncategorized] ~35-~35: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...) - Install Longhorn preflight ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_trim.md
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...e.md) - Trim a Longhorn volume ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_trim_volume.md
[uncategorized] ~45-~45: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... - Longhorn trimming operations ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_version.md
[uncategorized] ~27-~27: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
docs/longhornctl.md
29-29: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_check.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_check_preflight.md
60-60: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_doc.md
31-31: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_export.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_export_replica.md
65-65: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_export_replica_stop.md
34-34: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_get.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_get_replica.md
63-63: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_global-options.md
31-31: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_install.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_install_preflight.md
48-48: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_install_preflight_stop.md
35-35: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_trim.md
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_trim_volume.md
45-45: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_version.md
27-27: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🔇 Additional comments (15)
docs/longhornctl_trim.md (1)
19-19:
This change appears unrelated to the PR objectives
This documentation update only changes an auto-generated timestamp and has no relation to the PR's objective of improving NFS version checking functionality. Consider excluding this auto-generated change from the PR to maintain a focused scope.
Let's verify if this is truly an auto-generated change:
🧰 Tools
🪛 Markdownlint (0.35.0)
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🪛 LanguageTool
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...e.md) - Trim a Longhorn volume ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_export.md (1)
19-19: LGTM: Auto-generated timestamp update
The timestamp update is an expected change from the documentation auto-generation process.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ....md) - Export Longhorn replica ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_check.md (1)
19-19: No review needed for auto-generated changes.
This line is auto-generated by spf13/cobra. While there are minor formatting issues (missing hyphen in "Auto-generated" and incorrect heading level hierarchy), these should be reported upstream to spf13/cobra rather than modified directly in this file.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...md) - Check Longhorn preflight ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_install.md (1)
Line range hint 1-19: Documentation update needed for NFS version checking
Based on the PR objectives, this documentation should be updated to include information about the NFS version checking functionality, specifically regarding the /etc/nfsmount.conf configuration.
Let's check if there are any existing NFS-related sections in the documentation:
Would you like me to help draft the documentation updates to include:
- Information about NFS version checking
- The importance of
/etc/nfsmount.confconfiguration - How to verify and configure the correct NFS version?
🧰 Tools
🪛 Markdownlint (0.35.0)
19-19: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
16-16: Column: 32
Hard tabs
(MD010, no-hard-tabs)
17-17: Column: 68
Hard tabs
(MD010, no-hard-tabs)
🪛 LanguageTool
[uncategorized] ~19-~19: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...) - Install Longhorn preflight ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_version.md (1)
27-27: Auto-generated changes look good
The updated timestamp is an auto-generated change and doesn't require further review.
🧰 Tools
🪛 Markdownlint (0.35.0)
27-27: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🪛 LanguageTool
[uncategorized] ~27-~27: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Longhorn commandline interface. ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_export_replica_stop.md (1)
34-34: LGTM! Documentation timestamp update is as expected.
The updated timestamp reflects the recent documentation regeneration.
🧰 Tools
🪛 Markdownlint (0.35.0)
34-34: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🪛 LanguageTool
[uncategorized] ~34-~34: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ....md) - Export Longhorn replica ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_install_preflight_stop.md (1)
35-35: Documentation changes look good!
The timestamp update is an auto-generated change and doesn't affect the functionality.
🧰 Tools
🪛 Markdownlint (0.35.0)
35-35: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🪛 LanguageTool
[uncategorized] ~35-~35: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...) - Install Longhorn preflight ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl.md (1)
29-29: Consider improving the auto-generated section formatting
The auto-generated section still has formatting inconsistencies that were previously identified:
- "Auto-generated" should be hyphenated
- The heading level (h6) doesn't follow the proper hierarchy
As previously suggested, these formatting issues can be fixed by setting DisableAutoGenTag in the cobra command configuration. This will allow either removing the auto-generated line completely or implementing a consistent format.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...d) - Print longhornctl version ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
29-29: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_check_preflight.md (1)
60-60: LGTM on the timestamp update!
The auto-generated timestamp has been updated correctly.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~60-~60: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ... - Longhorn checking operations ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
🪛 Markdownlint (0.35.0)
60-60: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
docs/longhornctl_install_preflight.md (1)
48-48: LGTM!
The auto-generated timestamp update is expected and correct.
🧰 Tools
🪛 Markdownlint (0.35.0)
48-48: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🪛 LanguageTool
[uncategorized] ~48-~48: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...op Longhorn preflight installer ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_get_replica.md (1)
63-63: LGTM! Auto-generated timestamp update.
The timestamp update is consistent with the PR's changes.
🧰 Tools
🪛 Markdownlint (0.35.0)
63-63: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🪛 LanguageTool
[uncategorized] ~63-~63: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...nformation gathering operations ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
docs/longhornctl_export_replica.md (1)
65-65: LGTM on the timestamp update.
The auto-generated timestamp has been updated correctly.
🧰 Tools
🪛 Markdownlint (0.35.0)
65-65: Expected: h4; Actual: h6
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🪛 LanguageTool
[uncategorized] ~65-~65: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’).
Context: ...Stop exporting Longhorn replica ###### Auto generated by spf13/cobra on 27-Nov-2024
(AUTO_HYPHEN)
pkg/local/preflight/checker.go (3)
421-438: LGTM! Clean initialization of variables and proper file handling.
The code properly initializes the boolean flags and handles the kernel config file path construction correctly.
465-471: LGTM! Proper error handling and early return pattern.
The code correctly handles scanner errors and provides appropriate feedback when NFSv4 is not supported.
519-521: LGTM! Clear success reporting.
The code correctly reports success only when all checks pass, providing clear feedback to users.
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| // NFS version is 4 by default, nothing to check. | ||
| } else { | ||
| return errors.Wrap(err, "failed to check NFS default mount configurations") | ||
| } |
There was a problem hiding this comment.
| if err != nil { | |
| if os.IsNotExist(err) { | |
| // NFS version is 4 by default, nothing to check. | |
| } else { | |
| return errors.Wrap(err, "failed to check NFS default mount configurations") | |
| } | |
| if err != nil && !os.IsNotExist(err) { | |
| local.collection.Log.Error = append(local.collection.Log.Error, fmt.Sprintf("Failed to check NFS default mount configurations: %v", err)) | |
| return nil |
There was a problem hiding this comment.
This part would be refactored to longhorn/go-common-libs#88 so that could be reused both in preflight and node controller.
| } else { | ||
| return errors.Wrap(err, "failed to check NFS default mount configurations") | ||
| } | ||
| } else { |
There was a problem hiding this comment.
This else looks redundant. Do we need it?
|
|
||
| if err = nfsMountConfigScanner.Err(); err != nil { | ||
| local.collection.Log.Error = append(local.collection.Log.Error, "failed to scan NFS mount config") | ||
| isAllCheckPassed = false |
There was a problem hiding this comment.
| isAllCheckPassed = false | |
| return nil |
| isAllCheckPassed = false | ||
| } else if !isDefaultV4 { | ||
| local.collection.Log.Warn = append(local.collection.Log.Warn, "NFS4 is supported, but default protocol version is not 4, 4.1, or 4.2. Please refer to the NFS mount configuration manual page for more information: man 5 nfsmount.conf") | ||
| isAllCheckPassed = false |
There was a problem hiding this comment.
| isAllCheckPassed = false | |
| return nil |
| @@ -418,51 +418,106 @@ func (local *Checker) checkModulesLoaded(spdkDependent bool) error { | |||
| func (local *Checker) checkNFSv4Support() error { | |||
There was a problem hiding this comment.
I think we could remove the error return and instead log all errors to the collection logs. This approach could allow the checker to continue without blocking other checkups.
We should probably have a separate issue to apply this approach to other checkups as well. cc @derekbit
Which issue(s) this PR fixes:
Issue longhorn/longhorn#9830
What this PR does / why we need it:
Though the node supports NFSv4, the NFS client picks the protocol version by
Defaultversdefined in nfsmount.conf. This may cause problem while using backup-related features.Special notes for your reviewer:
Additional documentation or context