ci: expose os and k8s version info in gke pipeline #2289
Conversation
WalkthroughThis pull request introduces modifications across three files: Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🪛 Shellcheck (0.10.0)pipelines/gke/scripts/longhorn-setup.sh[warning] 30-30: Quote this to prevent word splitting. (SC2046) [warning] 30-30: Quote this to prevent word splitting. (SC2046) ⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (1)
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
🧹 Nitpick comments (3)
pipelines/gke/scripts/longhorn-setup.sh (1)
39-40: Use specific Alpine version for better reproducibility.The Alpine image tag should be specified for better reproducibility and security.
- image: alpine - args: ["/bin/sh", "-c", "while true;do date;sleep 5; done"] + image: alpine:3.19 + command: ["/bin/sh"] + args: ["-c", "while true; do date; sleep 5; done"]manager/integration/tests/common.py (2)
4051-4091: Add docstring and type hints for better code maintainability.The function would benefit from a docstring explaining its purpose and parameters, and type hints for better code maintainability.
-def check_longhorn(core_api): +def check_longhorn(core_api: k8sclient.CoreV1Api) -> None: + """ + Wait for all Longhorn components to be ready. + + Args: + core_api: The Kubernetes CoreV1Api client + + Raises: + AssertionError: If components are not ready within the retry period + """🧰 Tools
🪛 Ruff (0.8.2)
4091-4091: f-string without any placeholders
Remove extraneous
fprefix(F541)
🪛 GitHub Actions: build
[warning] 4060-4060: Line length exceeds 79 characters (E501)
[error] 4091-4091: f-string is missing placeholders (F541)
4060-4060: Fix line length to comply with PEP 8.The line exceeds the 79-character limit. Split it for better readability.
- elif labels.get('longhorn.io/component', '') == 'engine-image' \ + elif (labels.get('longhorn.io/component', '') == 'engine-image' + and item.status.phase == "Running"): - and item.status.phase == "Running":🧰 Tools
🪛 GitHub Actions: build
[warning] 4060-4060: Line length exceeds 79 characters (E501)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
manager/integration/tests/common.py(1 hunks)pipelines/gke/scripts/longhorn-setup.sh(2 hunks)pipelines/gke/terraform/main.tf(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
manager/integration/tests/common.py
4091-4091: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 GitHub Actions: build
manager/integration/tests/common.py
[warning] 4060-4060: Line length exceeds 79 characters (E501)
[error] 4091-4091: f-string is missing placeholders (F541)
🪛 Shellcheck (0.10.0)
pipelines/gke/scripts/longhorn-setup.sh
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (2)
pipelines/gke/terraform/main.tf (1)
45-45: Verify GKE version compatibility and availability.The GKE version update from 1.31.1 to 1.31.4 looks good, but let's verify its availability and release notes.
pipelines/gke/scripts/longhorn-setup.sh (1)
289-289: LGTM: Function call placement is correct.The placement of
print_out_cluster_infocall in the main function is appropriate, executing before any cluster operations.
| @@ -26,6 +26,32 @@ set_kubeconfig_envvar(){ | |||
| gcloud container clusters get-credentials `terraform -chdir=${TF_VAR_tf_workspace}/terraform output -raw cluster_name` --zone `terraform -chdir=${TF_VAR_tf_workspace}/terraform output -raw cluster_zone` --project ${TF_VAR_gcp_project} | |||
| } | |||
|
|
|||
| print_out_cluster_info(){ | |||
| gcloud container clusters describe `terraform -chdir=${TF_VAR_tf_workspace}/terraform output -raw cluster_name` --zone `terraform -chdir=${TF_VAR_tf_workspace}/terraform output -raw cluster_zone` --format="value(currentNodeVersion)" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix word splitting in gcloud command.
The gcloud command needs proper quoting to prevent word splitting issues.
- gcloud container clusters describe `terraform -chdir=${TF_VAR_tf_workspace}/terraform output -raw cluster_name` --zone `terraform -chdir=${TF_VAR_tf_workspace}/terraform output -raw cluster_zone` --format="value(currentNodeVersion)"
+ gcloud container clusters describe "$(terraform -chdir="${TF_VAR_tf_workspace}/terraform" output -raw cluster_name)" --zone "$(terraform -chdir="${TF_VAR_tf_workspace}/terraform" output -raw cluster_zone)" --format="value(currentNodeVersion)"📝 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.
| gcloud container clusters describe `terraform -chdir=${TF_VAR_tf_workspace}/terraform output -raw cluster_name` --zone `terraform -chdir=${TF_VAR_tf_workspace}/terraform output -raw cluster_zone` --format="value(currentNodeVersion)" | |
| gcloud container clusters describe "$(terraform -chdir="${TF_VAR_tf_workspace}/terraform" output -raw cluster_name)" --zone "$(terraform -chdir="${TF_VAR_tf_workspace}/terraform" output -raw cluster_zone)" --format="value(currentNodeVersion)" |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
| kubectl wait --for=condition=Ready pod/print-os-release --timeout=60s | ||
| kubectl exec -it print-os-release -- cat /mnt/host/etc/os-release | ||
| kubectl delete pod/print-os-release |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for pod operations.
The pod operations lack proper error handling and could leave resources behind if commands fail.
- kubectl wait --for=condition=Ready pod/print-os-release --timeout=60s
- kubectl exec -it print-os-release -- cat /mnt/host/etc/os-release
- kubectl delete pod/print-os-release
+ if ! kubectl wait --for=condition=Ready pod/print-os-release --timeout=60s; then
+ echo "Error: Pod failed to become ready"
+ kubectl delete pod/print-os-release
+ return 1
+ fi
+
+ if ! kubectl exec print-os-release -- cat /mnt/host/etc/os-release; then
+ echo "Error: Failed to read OS release info"
+ kubectl delete pod/print-os-release
+ return 1
+ fi
+
+ if ! kubectl delete pod/print-os-release; then
+ echo "Error: Failed to delete pod"
+ return 1
+ fi📝 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.
| kubectl wait --for=condition=Ready pod/print-os-release --timeout=60s | |
| kubectl exec -it print-os-release -- cat /mnt/host/etc/os-release | |
| kubectl delete pod/print-os-release | |
| if ! kubectl wait --for=condition=Ready pod/print-os-release --timeout=60s; then | |
| echo "Error: Pod failed to become ready" | |
| kubectl delete pod/print-os-release | |
| return 1 | |
| fi | |
| if ! kubectl exec print-os-release -- cat /mnt/host/etc/os-release; then | |
| echo "Error: Failed to read OS release info" | |
| kubectl delete pod/print-os-release | |
| return 1 | |
| fi | |
| if ! kubectl delete pod/print-os-release; then | |
| echo "Error: Failed to delete pod" | |
| return 1 | |
| fi |
| print_out_cluster_info(){ | ||
| gcloud container clusters describe `terraform -chdir=${TF_VAR_tf_workspace}/terraform output -raw cluster_name` --zone `terraform -chdir=${TF_VAR_tf_workspace}/terraform output -raw cluster_zone` --format="value(currentNodeVersion)" | ||
| kubectl create -f - <<EOF | ||
| apiVersion: v1 | ||
| kind: Pod | ||
| metadata: | ||
| name: print-os-release | ||
| spec: | ||
| containers: | ||
| - name: print-os-release | ||
| image: alpine | ||
| args: ["/bin/sh", "-c", "while true;do date;sleep 5; done"] | ||
| volumeMounts: | ||
| - name: host | ||
| mountPath: /mnt/host | ||
| volumes: | ||
| - name: host | ||
| hostPath: | ||
| path: / | ||
| type: Directory | ||
| EOF | ||
| kubectl wait --for=condition=Ready pod/print-os-release --timeout=60s | ||
| kubectl exec -it print-os-release -- cat /mnt/host/etc/os-release | ||
| kubectl delete pod/print-os-release | ||
| } |
There was a problem hiding this comment.
Security: Restrict host mount access and remove interactive terminal usage.
The current implementation has several security concerns:
- Mounting the entire host root directory is excessive when only
/etc/os-releaseis needed - Using
-itflags forkubectl execis unnecessary and may cause issues in non-interactive environments
Apply these changes to improve security:
- volumeMounts:
- - name: host
- mountPath: /mnt/host
- volumes:
- - name: host
- hostPath:
- path: /
- type: Directory
+ volumeMounts:
+ - name: os-release
+ mountPath: /mnt/host/etc/os-release
+ readOnly: true
+ volumes:
+ - name: os-release
+ hostPath:
+ path: /etc/os-release
+ type: File
- kubectl exec -it print-os-release -- cat /mnt/host/etc/os-release
+ kubectl exec print-os-release -- cat /mnt/host/etc/os-release📝 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.
| print_out_cluster_info(){ | |
| gcloud container clusters describe `terraform -chdir=${TF_VAR_tf_workspace}/terraform output -raw cluster_name` --zone `terraform -chdir=${TF_VAR_tf_workspace}/terraform output -raw cluster_zone` --format="value(currentNodeVersion)" | |
| kubectl create -f - <<EOF | |
| apiVersion: v1 | |
| kind: Pod | |
| metadata: | |
| name: print-os-release | |
| spec: | |
| containers: | |
| - name: print-os-release | |
| image: alpine | |
| args: ["/bin/sh", "-c", "while true;do date;sleep 5; done"] | |
| volumeMounts: | |
| - name: host | |
| mountPath: /mnt/host | |
| volumes: | |
| - name: host | |
| hostPath: | |
| path: / | |
| type: Directory | |
| EOF | |
| kubectl wait --for=condition=Ready pod/print-os-release --timeout=60s | |
| kubectl exec -it print-os-release -- cat /mnt/host/etc/os-release | |
| kubectl delete pod/print-os-release | |
| } | |
| print_out_cluster_info(){ | |
| gcloud container clusters describe `terraform -chdir=${TF_VAR_tf_workspace}/terraform output -raw cluster_name` --zone `terraform -chdir=${TF_VAR_tf_workspace}/terraform output -raw cluster_zone` --format="value(currentNodeVersion)" | |
| kubectl create -f - <<EOF | |
| apiVersion: v1 | |
| kind: Pod | |
| metadata: | |
| name: print-os-release | |
| spec: | |
| containers: | |
| - name: print-os-release | |
| image: alpine | |
| args: ["/bin/sh", "-c", "while true;do date;sleep 5; done"] | |
| volumeMounts: | |
| - name: os-release | |
| mountPath: /mnt/host/etc/os-release | |
| readOnly: true | |
| volumes: | |
| - name: os-release | |
| hostPath: | |
| path: /etc/os-release | |
| type: File | |
| EOF | |
| kubectl wait --for=condition=Ready pod/print-os-release --timeout=60s | |
| kubectl exec print-os-release -- cat /mnt/host/etc/os-release | |
| kubectl delete pod/print-os-release | |
| } |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
[warning] 30-30: Quote this to prevent word splitting.
(SC2046)
manager/integration/tests/common.py
Outdated
|
|
||
| assert ready | ||
| assert ready, f"Failed to wait for Longhorn components ready" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary f-string prefix.
The string doesn't contain any placeholders, so the f-string prefix is unnecessary.
- assert ready, f"Failed to wait for Longhorn components ready"
+ assert ready, "Failed to wait for Longhorn components ready"📝 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.
| assert ready, f"Failed to wait for Longhorn components ready" | |
| assert ready, "Failed to wait for Longhorn components ready" |
🧰 Tools
🪛 Ruff (0.8.2)
4091-4091: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 GitHub Actions: build
[error] 4091-4091: f-string is missing placeholders (F541)
Signed-off-by: Yang Chiu <[email protected]>
yangchiu
force-pushed
the
expose-os-gke
branch
from
77a0860 to
33d0088
Compare
Which issue(s) this PR fixes:
Issue longhorn/longhorn#10285
What this PR does / why we need it:
expose os and k8s version info in gke pipeline
Special notes for your reviewer:
Additional documentation or context
Summary by CodeRabbit
Tests
Chores