ci: support sle-micro for hal cluster #2150
Conversation
|
Warning Rate limit exceeded@yangchiu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request consist of modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (2)
test_framework/terraform/harvester/sle-micro/variables.tf (1)
30-33: Consider future-proofing OS version constraints.To maintain long-term maintainability as new SLE Micro versions are released, consider:
- Adding a list of supported versions in a local variable
- Documenting the version support policy
- Adding validation against supported versions
Example implementation:
locals { supported_sle_micro_versions = ["5.5", "6.0"] } variable "os_distro_version" { type = string description = "SLE Micro OS version to be deployed (supported versions: ${join(", ", local.supported_sle_micro_versions)})" default = "6.0" validation { condition = contains(local.supported_sle_micro_versions, var.os_distro_version) error_message = "Unsupported SLE Micro version. Must be one of: ${join(", ", local.supported_sle_micro_versions)}" } }pipelines/e2e/scripts/longhorn-setup.sh (1)
57-60: Consider enhancing error handling for unsupported distributions.The error message in the comment suggests improving the error handling for unsupported distributions.
Consider these improvements:
- Add explicit distribution validation at the start of the script
- Create a dedicated function for distribution checks
- Maintain a list of supported/unsupported distributions in a configuration file
- Provide more informative error messages with supported alternatives
Would you like me to propose an implementation for any of these improvements?
🧰 Tools
🪛 Shellcheck
[warning] 58-58: TF_VAR_k8s_distro_name is referenced but not assigned.
(SC2154)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- pipelines/e2e/scripts/longhorn-setup.sh (1 hunks)
- test_framework/scripts/longhorn-setup.sh (1 hunks)
- test_framework/terraform/harvester/sle-micro/main.tf (1 hunks)
- test_framework/terraform/harvester/sle-micro/variables.tf (1 hunks)
🧰 Additional context used
🪛 Shellcheck
pipelines/e2e/scripts/longhorn-setup.sh
[warning] 58-58: TF_VAR_k8s_distro_name is referenced but not assigned.
(SC2154)
🔇 Additional comments (2)
pipelines/e2e/scripts/longhorn-setup.sh (1)
57-60: Verify TF_VAR_k8s_distro_name variable initialization.The changes correctly remove restrictions for SLE Micro while maintaining necessary exclusions. However, the
TF_VAR_k8s_distro_namevariable usage needs verification as it appears undefined.Let's verify the variable initialization:
🧰 Tools
🪛 Shellcheck
[warning] 58-58: TF_VAR_k8s_distro_name is referenced but not assigned.
(SC2154)
test_framework/scripts/longhorn-setup.sh (1)
526-528: LGTM! Changes align with SLE Micro support objective.The modification correctly updates the condition to support SLE Micro while maintaining the exclusion for EKS and Talos distributions. The error message update reflects this change in supported operating systems.
Let's verify the changes don't affect other OS distributions:
✅ Verification successful
Based on the search results, I can now generate a final response as I have sufficient information about OS-specific conditions across the codebase.
Changes are consistent with existing OS-specific handling patterns
The modification to support SLE Micro aligns well with the existing OS-specific handling patterns in the codebase:
- The condition
[[ "${TF_VAR_k8s_distro_name}" != "eks" ]] && [[ "${DISTRO}" != "talos" ]]maintains consistency with other OS-specific checks in the codebase- Other scripts in various pipelines (e2e, helm, fleet, etc.) show similar patterns for OS-specific handling, particularly for:
- SELinux workarounds for RHEL, Rocky Linux, and Oracle
- Special namespace handling for Talos
- Specific installation methods for SLES
The changes don't introduce any inconsistencies with existing OS-specific logic and maintain the established pattern of explicitly handling different distributions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other OS-specific conditions in the codebase # that might need to be updated for consistency # Search for OS-specific conditions rg -A 5 "DISTRO.*==" rg -A 5 "operating.*systems.*not.*supported"Length of output: 9038
| variable "registration_code" { | ||
| type = string | ||
| sensitive = true | ||
| } |
There was a problem hiding this comment.
Add documentation and validation for registration code.
The registration code variable needs proper documentation and validation since it's crucial for SLE Micro subscription management.
Apply these improvements:
variable "registration_code" {
type = string
sensitive = true
+ description = "SLE Micro subscription registration code"
+ validation {
+ condition = length(var.registration_code) > 0
+ error_message = "Registration code cannot be empty"
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| variable "registration_code" { | |
| type = string | |
| sensitive = true | |
| } | |
| variable "registration_code" { | |
| type = string | |
| sensitive = true | |
| description = "SLE Micro subscription registration code" | |
| validation { | |
| condition = length(var.registration_code) > 0 | |
| error_message = "Registration code cannot be empty" | |
| } | |
| } |
| variable "ssh_public_key_file_path" { | ||
| type = string | ||
| default = "~/.ssh/id_rsa.pub" | ||
| } |
There was a problem hiding this comment.
Fix SSH key path handling and add validation.
The current SSH key path has two potential issues:
- Using tilde (~) for home directory might not work consistently in Terraform
- Missing validation for file existence
Apply this fix:
variable "ssh_public_key_file_path" {
type = string
- default = "~/.ssh/id_rsa.pub"
+ default = "/home/ubuntu/.ssh/id_rsa.pub" # Update with appropriate default path
+ validation {
+ condition = fileexists(var.ssh_public_key_file_path)
+ error_message = "SSH public key file does not exist at the specified path"
+ }
}Committable suggestion was skipped due to low confidence.
| variable "arch" { | ||
| type = string | ||
| description = "available values (amd64, arm64)" | ||
| default = "amd64" | ||
| } | ||
|
|
||
| variable "os_distro_version" { | ||
| type = string | ||
| default = "6.0" | ||
| } |
There was a problem hiding this comment.
Enhance system configuration validation and documentation.
The system configuration variables need better validation and documentation, especially since this is for SLE Micro support.
Apply these improvements:
variable "arch" {
type = string
description = "available values (amd64, arm64)"
default = "amd64"
+ validation {
+ condition = contains(["amd64", "arm64"], var.arch)
+ error_message = "Architecture must be one of: amd64, arm64"
+ }
}
variable "os_distro_version" {
type = string
+ description = "SLE Micro OS version to be deployed"
default = "6.0"
+ validation {
+ condition = can(regex("^\\d+\\.\\d+$", var.os_distro_version))
+ error_message = "OS version must be in format X.Y (e.g., 6.0)"
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| variable "arch" { | |
| type = string | |
| description = "available values (amd64, arm64)" | |
| default = "amd64" | |
| } | |
| variable "os_distro_version" { | |
| type = string | |
| default = "6.0" | |
| } | |
| variable "arch" { | |
| type = string | |
| description = "available values (amd64, arm64)" | |
| default = "amd64" | |
| validation { | |
| condition = contains(["amd64", "arm64"], var.arch) | |
| error_message = "Architecture must be one of: amd64, arm64" | |
| } | |
| } | |
| variable "os_distro_version" { | |
| type = string | |
| description = "SLE Micro OS version to be deployed" | |
| default = "6.0" | |
| validation { | |
| condition = can(regex("^\\d+\\.\\d+$", var.os_distro_version)) | |
| error_message = "OS version must be in format X.Y (e.g., 6.0)" | |
| } | |
| } |
| variable "lab_url" { | ||
| type = string | ||
| description = "LAB URL" | ||
| sensitive = true | ||
| } | ||
|
|
||
| variable "lab_access_key" { | ||
| type = string | ||
| description = "LAB ACCESS_KEY" | ||
| sensitive = true | ||
| } | ||
|
|
||
| variable "lab_secret_key" { | ||
| type = string | ||
| description = "LAB SECRET_KEY" | ||
| sensitive = true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation blocks for lab credentials.
The lab credential variables should include validation to ensure proper formatting and prevent configuration errors.
Add validation blocks:
variable "lab_url" {
type = string
description = "LAB URL"
sensitive = true
+ validation {
+ condition = can(regex("^https?://", var.lab_url))
+ error_message = "Lab URL must start with http:// or https://"
+ }
}
variable "lab_access_key" {
type = string
description = "LAB ACCESS_KEY"
sensitive = true
+ validation {
+ condition = length(var.lab_access_key) > 0
+ error_message = "Lab access key cannot be empty"
+ }
}
variable "lab_secret_key" {
type = string
description = "LAB SECRET_KEY"
sensitive = true
+ validation {
+ condition = length(var.lab_secret_key) > 0
+ error_message = "Lab secret key cannot be empty"
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| variable "lab_url" { | |
| type = string | |
| description = "LAB URL" | |
| sensitive = true | |
| } | |
| variable "lab_access_key" { | |
| type = string | |
| description = "LAB ACCESS_KEY" | |
| sensitive = true | |
| } | |
| variable "lab_secret_key" { | |
| type = string | |
| description = "LAB SECRET_KEY" | |
| sensitive = true | |
| } | |
| variable "lab_url" { | |
| type = string | |
| description = "LAB URL" | |
| sensitive = true | |
| validation { | |
| condition = can(regex("^https?://", var.lab_url)) | |
| error_message = "Lab URL must start with http:// or https://" | |
| } | |
| } | |
| variable "lab_access_key" { | |
| type = string | |
| description = "LAB ACCESS_KEY" | |
| sensitive = true | |
| validation { | |
| condition = length(var.lab_access_key) > 0 | |
| error_message = "Lab access key cannot be empty" | |
| } | |
| } | |
| variable "lab_secret_key" { | |
| type = string | |
| description = "LAB SECRET_KEY" | |
| sensitive = true | |
| validation { | |
| condition = length(var.lab_secret_key) > 0 | |
| error_message = "Lab secret key cannot be empty" | |
| } | |
| } |
| variable "k8s_distro_name" { | ||
| type = string | ||
| default = "rke2" | ||
| description = "kubernetes distro version to install [rke, k3s] (default: rke2)" | ||
| } | ||
|
|
||
| variable "k8s_distro_version" { | ||
| type = string | ||
| default = "v1.27.13+rke2r1" | ||
| description = <<-EOT | ||
| kubernetes version that will be deployed | ||
| k3s: (default: v1.27.13+k3s1) | ||
| rke2: (default: v1.27.13+rke2r1) | ||
| EOT | ||
| } |
There was a problem hiding this comment.
Fix Kubernetes configuration inconsistencies and add validation.
There are several issues with the Kubernetes configuration variables:
- The k8s_distro_name description doesn't match the default value
- Missing version string validation
- The description could be more structured
Apply these fixes:
variable "k8s_distro_name" {
type = string
default = "rke2"
- description = "kubernetes distro version to install [rke, k3s] (default: rke2)"
+ description = "kubernetes distro version to install [rke2, k3s] (default: rke2)"
+ validation {
+ condition = contains(["rke2", "k3s"], var.k8s_distro_name)
+ error_message = "Kubernetes distribution must be one of: rke2, k3s"
+ }
}
variable "k8s_distro_version" {
type = string
default = "v1.27.13+rke2r1"
description = <<-EOT
kubernetes version that will be deployed
- k3s: (default: v1.27.13+k3s1)
- rke2: (default: v1.27.13+rke2r1)
+ Supported formats:
+ - k3s: v<major>.<minor>.<patch>+k3s1 (e.g., v1.27.13+k3s1)
+ - rke2: v<major>.<minor>.<patch>+rke2r1 (e.g., v1.27.13+rke2r1)
EOT
+ validation {
+ condition = can(regex("^v\\d+\\.\\d+\\.\\d+\\+(rke2r1|k3s1)$", var.k8s_distro_version))
+ error_message = "Invalid Kubernetes version format. Must match v1.27.13+rke2r1 or v1.27.13+k3s1 pattern"
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| variable "k8s_distro_name" { | |
| type = string | |
| default = "rke2" | |
| description = "kubernetes distro version to install [rke, k3s] (default: rke2)" | |
| } | |
| variable "k8s_distro_version" { | |
| type = string | |
| default = "v1.27.13+rke2r1" | |
| description = <<-EOT | |
| kubernetes version that will be deployed | |
| k3s: (default: v1.27.13+k3s1) | |
| rke2: (default: v1.27.13+rke2r1) | |
| EOT | |
| } | |
| variable "k8s_distro_name" { | |
| type = string | |
| default = "rke2" | |
| description = "kubernetes distro version to install [rke2, k3s] (default: rke2)" | |
| validation { | |
| condition = contains(["rke2", "k3s"], var.k8s_distro_name) | |
| error_message = "Kubernetes distribution must be one of: rke2, k3s" | |
| } | |
| } | |
| variable "k8s_distro_version" { | |
| type = string | |
| default = "v1.27.13+rke2r1" | |
| description = <<-EOT | |
| kubernetes version that will be deployed | |
| Supported formats: | |
| - k3s: v<major>.<minor>.<patch>+k3s1 (e.g., v1.27.13+k3s1) | |
| - rke2: v<major>.<minor>.<patch>+rke2r1 (e.g., v1.27.13+rke2r1) | |
| EOT | |
| validation { | |
| condition = can(regex("^v\\d+\\.\\d+\\.\\d+\\+(rke2r1|k3s1)$", var.k8s_distro_version)) | |
| error_message = "Invalid Kubernetes version format. Must match v1.27.13+rke2r1 or v1.27.13+k3s1 pattern" | |
| } | |
| } |
| cpu_count = "4" | ||
| memory_size = "8" |
There was a problem hiding this comment.
Use numeric values for cpu_count and memory_size
The cpu_count and memory_size should be specified as numbers, not strings. Remove the quotes to ensure they are interpreted as integers.
Apply this diff to fix the values:
- cpu_count = "4"
- memory_size = "8"
+ cpu_count = 4
+ memory_size = 8Also applies to: 90-91
|
|
||
| output "kube_config" { | ||
| value = rancher2_cluster_v2.e2e-cluster.kube_config | ||
| sensitive = "true" |
There was a problem hiding this comment.
Fix the syntax error: Set sensitive attribute to boolean
In the output block for kube_config, the sensitive attribute should be a boolean value, not a string. Remove the quotes around true.
Apply this diff to fix the syntax:
- sensitive = "true"
+ sensitive = true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sensitive = "true" | |
| sensitive = true |
There was a problem hiding this comment.
@yangchiu Looks like a reasonable review comment.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
|
||
| output "kube_config" { | ||
| value = rancher2_cluster_v2.e2e-cluster.kube_config | ||
| sensitive = "true" |
There was a problem hiding this comment.
@yangchiu Looks like a reasonable review comment.
yangchiu
force-pushed
the
hal-sl-micro
branch
from
48f0dba
to
9e65e28
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- pipelines/e2e/scripts/longhorn-setup.sh (1 hunks)
- test_framework/scripts/longhorn-setup.sh (1 hunks)
- test_framework/terraform/harvester/sle-micro/main.tf (1 hunks)
- test_framework/terraform/harvester/sle-micro/variables.tf (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test_framework/scripts/longhorn-setup.sh
- test_framework/terraform/harvester/sle-micro/variables.tf
🧰 Additional context used
🪛 Shellcheck
pipelines/e2e/scripts/longhorn-setup.sh
[warning] 58-58: TF_VAR_k8s_distro_name is referenced but not assigned.
(SC2154)
🔇 Additional comments (3)
pipelines/e2e/scripts/longhorn-setup.sh (2)
57-60: LGTM: Successfully adds SLE Micro supportThe changes correctly implement SLE Micro support by removing it from the exclusion list, allowing the
longhornctl_checkto run on SLE Micro systems. This aligns with the PR objective of adding SLE Micro support for HAL cluster.🧰 Tools
🪛 Shellcheck
[warning] 58-58: TF_VAR_k8s_distro_name is referenced but not assigned.
(SC2154)
58-58:⚠️ Potential issueFix: TF_VAR_k8s_distro_name variable is undefined
The script uses
TF_VAR_k8s_distro_namein conditions but this variable is not assigned anywhere in the script. This could lead to unexpected behavior if the variable is not set in the environment.Consider one of these solutions:
- Add a default value:
+ : "${TF_VAR_k8s_distro_name:=}"
- Add validation:
+ if [ -z "${TF_VAR_k8s_distro_name+x}" ]; then + echo "Error: TF_VAR_k8s_distro_name is not set" + exit 1 + fiLet's verify if this variable is defined elsewhere:
🧰 Tools
🪛 Shellcheck
[warning] 58-58: TF_VAR_k8s_distro_name is referenced but not assigned.
(SC2154)
test_framework/terraform/harvester/sle-micro/main.tf (1)
124-124: Ensure 'registration_code' variable is defined and marked as sensitiveThe variable
${var.registration_code}used in the cloud-init script may contain sensitive information. Please verify thatregistration_codeis properly defined invariables.tfand marked assensitiveto prevent accidental exposure of confidential data.
| - touch /usr/local/bin/cron-reboot.sh | ||
| - chmod +x /usr/local/bin/cron-reboot.sh | ||
| - echo systemctl enable iscsid >> /usr/local/bin/cron-reboot.sh | ||
| - echo systemctl start iscsid >> /usr/local/bin/cron-reboot.sh | ||
| - echo "@reboot root /usr/local/bin/cron-reboot.sh" >> /etc/crontab |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use cloud-init modules to manage scripts and cron jobs
Directly echoing commands to files and appending to /etc/crontab can lead to syntax errors or unintended behavior. Consider using cloud-init's write_files module to create the /usr/local/bin/cron-reboot.sh script and the runcmd or bootcmd module to set up the cron job more reliably.
yangchiu
force-pushed
the
hal-sl-micro
branch
from
9e65e28
to
7488f20
Compare
Signed-off-by: Yang Chiu <yang.chiu@suse.com>
yangchiu
force-pushed
the
hal-sl-micro
branch
from
7488f20
to
889d4ac
Compare
Which issue(s) this PR fixes:
Issue longhorn/longhorn#9691
What this PR does / why we need it:
support sle-micro for hal cluster
Special notes for your reviewer:
Additional documentation or context
Summary by CodeRabbit
New Features
Bug Fixes
Documentation