ci: support sles for hal cluster #2149
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces a new Terraform configuration for managing a Rancher2 environment integrated with Harvester. It includes the setup of the Rancher2 provider, resource definitions for cloud credentials and machine configurations, and the creation of a new Rancher2 cluster. Additionally, several new variables are declared to support configuration management for the deployment. The changes aim to enhance the infrastructure setup for various operating systems in the HAL cluster. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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: 4
🧹 Outside diff range and nitpick comments (2)
test_framework/terraform/harvester/sles/main.tf (2)
159-159: Consider omitting 'chart_values' if not usedThe
chart_valuesparameter is set to an empty string. If it's not required, you can omit this parameter to simplify the configuration.Apply this diff if applicable:
- chart_values = ""
12-12: Ensure 'insecure = true' is appropriate for your environmentSetting
insecure = truedisables SSL certificate validation for the Rancher API connection. Ensure this is acceptable for your security requirements. In production environments, it's recommended to enable SSL verification.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- test_framework/terraform/harvester/sles/main.tf (1 hunks)
- test_framework/terraform/harvester/sles/variables.tf (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
test_framework/terraform/harvester/sles/variables.tf (1)
1-49: Verify SLES version compatibility with HAL cluster requirements.Let's ensure the SLES version constraints align with the supported versions in the HAL cluster.
✅ Verification successful
Let me gather more specific information about the SLES versions and configurations to verify compatibility.
The previous results showed multiple SLES configurations across AWS and Harvester environments. We need to verify the specific version constraints and ensure they align with HAL cluster requirements. This script will help us gather that information.
Let me try one more search to find specific version configurations and defaults:
The previous results didn't show the version configurations we need. This script will help us see all version-related configurations and the complete content of SLES-specific files to verify version compatibility.
Based on the comprehensive review of the codebase, I can now provide a final response regarding the SLES version compatibility:
SLES version configuration is appropriate for HAL cluster deployment
The configuration aligns well with the HAL cluster requirements:
- Default SLES version is set to "15-sp6" which is the latest service pack for SLES 15
- The configuration supports both AMD64 and ARM64 architectures
- Necessary system configurations are properly handled in the provisioning scripts:
- Required packages (iscsi, nfs-client, cryptsetup, device-mapper)
- Kernel modules (uio, vfio_pci, nvme-tcp, dm_crypt)
- System parameters (hugepages)
- Compatible with all supported Kubernetes distributions (k3s, rke2)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any SLES version references or requirements in the codebase echo "Searching for SLES version requirements..." rg -i "sles.*version|suse.*version" --type yaml --type tf # Look for any existing SLES configurations echo "Checking existing SLES configurations..." fd -e yaml -e tf . -x grep -l "sles"Length of output: 1457
Script:
#!/bin/bash # Check SLES version configurations across different environments echo "Checking SLES version configurations..." rg -A 3 "os_distro_version.*=.*sp" --type tf # Check for any version constraints or requirements in documentation echo "Checking documentation for version requirements..." fd README.md -x cat {} \; | rg -i "sles|suse" # Check for any HAL cluster specific SLES configurations echo "Checking HAL cluster SLES configurations..." rg -A 5 "harvester.*sles|hal.*sles" --type tfLength of output: 442
Script:
#!/bin/bash # Check all os_distro_version defaults and configurations echo "Checking os_distro_version configurations..." rg "os_distro_version.*=.*" --type tf -B 2 -A 2 # Check for any version-related configurations in the SLES directories echo "Checking SLES directory configurations..." fd . -t f "test_framework/terraform/harvester/sles" "test_framework/terraform/aws/sles" -x cat {}Length of output: 43870
test_framework/terraform/harvester/sles/main.tf (4)
81-86: Verify package names in cloud-init script for SLESPlease verify that the package names specified under
packagesare correct for SUSE Linux Enterprise Server. For example, the packagenfs-clientmay be namednfs-utilson SLES.
142-146: Verify 'cloud-provider-name' configuration in 'machine_selector_config'The
cloud-provider-nameis set to an empty string. Please verify whether this is intentional and whether it aligns with the desired cluster configuration.
156-157: Confirm the etcd snapshot scheduleThe
snapshot_schedule_cronis set to"0 */5 * * *", which schedules etcd snapshots every 5 hours at minute 0. Please confirm that this schedule matches your desired backup frequency.
166-166:⚠️ Potential issueFix 'sensitive' parameter to be a boolean
The
sensitiveargument in the outputkube_configshould be a boolean value, not a string. Remove the quotes aroundtrueto correct the syntax.Apply this diff to fix the issue:
- sensitive = "true" + sensitive = trueLikely invalid or redundant comment.
| variable "ssh_public_key_file_path" { | ||
| type = string | ||
| default = "~/.ssh/id_rsa.pub" | ||
| } |
There was a problem hiding this comment.
Improve SSH key path handling.
The current default path using ~ might not resolve correctly in all environments. Consider:
- Using absolute path or environment variable
- Adding path validation
Apply this fix:
variable "ssh_public_key_file_path" {
type = string
- default = "~/.ssh/id_rsa.pub"
+ default = "/home/ubuntu/.ssh/id_rsa.pub" # Or use environment variable
+ 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 "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
Enhance credential variables with validation and documentation.
While the sensitive flags are correctly set, consider adding:
- URL format validation for
lab_url - More detailed descriptions including expected formats and examples
Apply this enhancement:
variable "lab_url" {
type = string
- description = "LAB URL"
+ description = "URL of the lab environment (e.g., https://lab.example.com)"
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"
+ description = "Access key for lab environment authentication. Must be kept secret."
sensitive = true
}
variable "lab_secret_key" {
type = string
- description = "LAB SECRET_KEY"
+ description = "Secret key for lab environment authentication. Must be kept secret."
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.
| 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 = "URL of the lab environment (e.g., https://lab.example.com)" | |
| 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 = "Access key for lab environment authentication. Must be kept secret." | |
| sensitive = true | |
| } | |
| variable "lab_secret_key" { | |
| type = string | |
| description = "Secret key for lab environment authentication. Must be kept secret." | |
| sensitive = true | |
| } |
| variable "arch" { | ||
| type = string | ||
| description = "available values (amd64, arm64)" | ||
| default = "amd64" | ||
| } | ||
|
|
||
| variable "os_distro_version" { | ||
| type = string | ||
| default = "15-sp6" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and improve documentation for system configuration.
The architecture and OS version variables need better validation and documentation:
Apply these improvements:
variable "arch" {
type = string
- description = "available values (amd64, arm64)"
+ description = "Target architecture for the deployment. Supported values: amd64, arm64"
default = "amd64"
+ validation {
+ condition = contains(["amd64", "arm64"], var.arch)
+ error_message = "Architecture must be either amd64 or arm64"
+ }
}
variable "os_distro_version" {
type = string
+ description = "SLES version in format 'XX-spY'. Supported versions: 15-sp4, 15-sp5, 15-sp6"
default = "15-sp6"
+ validation {
+ condition = can(regex("^15-sp[4-6]$", var.os_distro_version))
+ error_message = "SLES version must be in format '15-spX' where X is 4, 5, or 6"
+ }
}📝 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 = "15-sp6" | |
| } | |
| variable "arch" { | |
| type = string | |
| description = "Target architecture for the deployment. Supported values: amd64, arm64" | |
| default = "amd64" | |
| validation { | |
| condition = contains(["amd64", "arm64"], var.arch) | |
| error_message = "Architecture must be either amd64 or arm64" | |
| } | |
| } | |
| variable "os_distro_version" { | |
| type = string | |
| description = "SLES version in format 'XX-spY'. Supported versions: 15-sp4, 15-sp5, 15-sp6" | |
| default = "15-sp6" | |
| validation { | |
| condition = can(regex("^15-sp[4-6]$", var.os_distro_version)) | |
| error_message = "SLES version must be in format '15-spX' where X is 4, 5, or 6" | |
| } | |
| } |
| 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 inconsistency in Kubernetes distribution options and add version validation.
There are several issues to address:
- The description lists [rke, k3s] but the default is rke2
- Missing version format validation
- Version constraints should be documented
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 either rke2 or 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)
EOT
+ validation {
+ condition = can(regex("^v\\d+\\.\\d+\\.\\d+\\+(rke2r|k3s)\\d+$", var.k8s_distro_version))
+ error_message = "Invalid Kubernetes version format. Must be like v1.27.13+rke2r1 or v1.27.13+k3s1"
+ }
}📝 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 either rke2 or 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) | |
| EOT | |
| validation { | |
| condition = can(regex("^v\\d+\\.\\d+\\.\\d+\\+(rke2r|k3s)\\d+$", var.k8s_distro_version)) | |
| error_message = "Invalid Kubernetes version format. Must be like v1.27.13+rke2r1 or v1.27.13+k3s1" | |
| } | |
| } |
yangchiu
force-pushed
the
hal-sles
branch
2 times, most recently
from
f819dbc
to
40005c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
test_framework/terraform/harvester/sles/variables.tf (1)
1-54: Consider adding variable grouping and comprehensive documentation.While these variables provide the necessary configuration for SLES support, consider:
- Adding variable grouping comments to organize related variables (e.g., "Lab Configuration", "System Configuration", "Kubernetes Configuration")
- Including a header comment block explaining the overall purpose of these variables in the context of SLES support for HAL cluster
test_framework/terraform/harvester/sles/main.tf (1)
133-134: Use 'systemctl enable --now' for starting servicesFor consistency and simplicity, use
systemctl enable --nowto start and enable services in one command. This approach was used forqemu-guest-agent.serviceand can be applied toiscsid.[consistency]
Apply this diff:
- - systemctl enable iscsid - - systemctl start iscsid + - systemctl enable --now iscsid
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- pipelines/e2e/Jenkinsfile (1 hunks)
- test_framework/terraform/harvester/sles/main.tf (1 hunks)
- test_framework/terraform/harvester/sles/variables.tf (1 hunks)
🔇 Additional comments (8)
test_framework/terraform/harvester/sles/variables.tf (4)
1-17: Previous review comment about credential variables is still valid.The earlier review comment suggesting URL validation and improved documentation for these variables has not been addressed.
19-22: Previous review comment about SSH key path handling is still valid.The earlier review comment suggesting improvements to SSH key path handling has not been addressed.
35-49: Previous review comment about Kubernetes distribution options is still valid.The earlier review comment addressing inconsistencies in Kubernetes distribution options and version validation has not been addressed.
24-33:⚠️ Potential issuePrevious review comment about architecture validation is still valid and missing OS version description.
- The earlier review comment suggesting validation for architecture remains valid.
- Additionally, the
os_distro_versionvariable lacks a description field explaining its purpose and valid values.Add description for os_distro_version:
variable "os_distro_version" { type = string + description = "SLES version to be installed. Supported versions: 15-sp4, 15-sp5, 15-sp6" default = "15-sp6" }pipelines/e2e/Jenkinsfile (1)
10-23: LGTM! Verify credential configuration in Jenkins.The registration code selection logic correctly handles both SLES and SLE-micro distributions with their respective architecture-specific registration codes. Please ensure that all required credential IDs are configured in Jenkins:
- REGISTRATION_CODE
- SLES_REGISTRATION_CODE
- SLES_REGISTRATION_CODE_ARM64
- REGISTRATION_CODE_ARM64
test_framework/terraform/harvester/sles/main.tf (3)
10-14: Ensure sensitive variables are properly securedThe provider configuration uses variables
var.lab_access_keyandvar.lab_secret_key. Ensure these variables are marked assensitivein your variables definition to prevent accidental exposure.[security]
74-75: Validate registration code variableThe command
SUSEConnect -r ${var.registration_code}relies onvar.registration_code. Ensure that this variable is correctly defined and securely handled.[security]
Also applies to: 126-128
128-128:⚠️ Potential issueReview the use of '--force-resolution' with zypper
Using
--force-resolutioninzypper -n install --force-resolution kernel-defaultmay lead to undesirable package changes. Verify if this option is necessary, or consider removing it to avoid potential conflicts.
| variable "registration_code" { | ||
| type = string | ||
| sensitive = true | ||
| } |
There was a problem hiding this comment.
Add description and fix formatting for registration_code variable.
The registration code variable needs a description explaining its purpose and usage for SLES registration. Also, there's inconsistent spacing in the type declaration.
Apply these improvements:
variable "registration_code" {
- type = string
+ type = string
+ description = "SLES registration code for system activation. Required for SLES deployment."
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.
| variable "registration_code" { | |
| type = string | |
| sensitive = true | |
| } | |
| variable "registration_code" { | |
| type = string | |
| description = "SLES registration code for system activation. Required for SLES deployment." | |
| sensitive = true | |
| } |
| disk_info = <<EOF | ||
| { | ||
| "disks": [{ | ||
| "imageName": "longhorn-qa/image-qn4cg", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize hardcoded 'imageName' values
The imageName is hardcoded as "longhorn-qa/image-qn4cg". Consider using a variable to parameterize this value for better flexibility and maintainability.
[maintainability]
Example change:
- "imageName": "longhorn-qa/image-qn4cg",
+ "imageName": "${var.image_name}",Remember to define the image_name variable in your variables.tf file.
Also applies to: 98-98
| network_info = <<EOF | ||
| { | ||
| "interfaces": [{ | ||
| "networkName": "longhorn-qa/vlan104" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize hardcoded 'networkName' values
The networkName is hardcoded as "longhorn-qa/vlan104". Consider parameterizing this value to allow configuration of different networks without code changes.
[maintainability]
Example change:
- "networkName": "longhorn-qa/vlan104"
+ "networkName": "${var.network_name}"Ensure you define the network_name variable accordingly.
Also applies to: 113-113
| - echo dm_crypt >> /etc/modules-load.d/modules.conf | ||
| - echo 1024 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages | ||
| - echo "vm.nr_hugepages=1024" >> /etc/sysctl.conf | ||
| - shutdown -r +5 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reboot immediately instead of after 5 minutes
The command shutdown -r +5 schedules a reboot after 5 minutes. Consider rebooting immediately to apply system changes without unnecessary delay.
Apply this diff to change the reboot command:
- - shutdown -r +5
+ - reboot📝 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.
| - shutdown -r +5 | |
| - reboot |
| required_providers { | ||
| rancher2 = { | ||
| source = "rancher/rancher2" | ||
| version = "~> 4.4.0" |
There was a problem hiding this comment.
Can we pivot to v5.1.0 latest version? v5.x has some fixes.
| vm_namespace = "longhorn-qa" | ||
|
|
||
| cpu_count = "4" | ||
| memory_size = "8" |
There was a problem hiding this comment.
| memory_size = "8" | |
| memory_size = "16" |
I recall @derekbit saying that if we want to use a v2 volume, it seems that 16 GB of host memory is more appropriate.
There was a problem hiding this comment.
@derekbit Where can we find documentation on this?
There was a problem hiding this comment.
8 GiB should be okay. Can we try it?
There was a problem hiding this comment.
8 GiB should be okay. Can we try it?
It's been tested.
Signed-off-by: Yang Chiu <yang.chiu@suse.com>
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
test_framework/terraform/harvester/sles/variables.tf (1)
51-54: Implement secure handling for SLES registration codes.While the variable is correctly marked as sensitive, consider additional security measures:
- Add validation to ensure the code format matches SLES registration code patterns
- Consider implementing rotation mechanisms
- Document secure practices for providing the registration code
Consider implementing these security enhancements:
variable "registration_code" { type = string description = "SLES registration code for system activation. Required for SLES deployment." sensitive = true + validation { + condition = can(regex("^[A-Z0-9]{8}(-[A-Z0-9]{4}){3}-[A-Z0-9]{12}$", var.registration_code)) + error_message = "Invalid SLES registration code format" + } }pipelines/e2e/Jenkinsfile (1)
Line range hint
108-109: Fix typo in debug message.There's a typo in the debug message: "coce" should be "code".
- echo "Using registration coce: $REGISTRATION_CODE_ID" + echo "Using registration code: $REGISTRATION_CODE_ID"
🛑 Comments failed to post (1)
pipelines/e2e/Jenkinsfile (1)
10-23:
⚠️ Potential issueAdd validation for unsupported distributions.
The current implementation lacks validation for the
DISTROparameter. This could lead to silent failures when an unsupported distribution is provided.Add validation at the beginning of the logic:
def REGISTRATION_CODE_ID = "REGISTRATION_CODE" +def SUPPORTED_DISTROS = ["sles", "sle-micro"] +if (!SUPPORTED_DISTROS.contains(params.DISTRO)) { + error("Unsupported distribution: ${params.DISTRO}") +} if (params.DISTRO == "sles") { // ... rest of the codeCommittable suggestion was skipped due to low confidence.
yangchiu
requested review from
khushboo-rancher,
roger-ryao,
a team and
derekbit
Which issue(s) this PR fixes:
Issue longhorn/longhorn#9691
What this PR does / why we need it:
support sles for hal cluster
Special notes for your reviewer:
Additional documentation or context
Summary by CodeRabbit
New Features
Bug Fixes