Husam/use los csi #67
Conversation
Signed-off-by: husam-lb <[email protected]>
WalkthroughThis pull request introduces a comprehensive update to the build and deployment configuration for the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 1
🔭 Outside diff range comments (1)
Makefile (1)
Line range hint
150-385: Consider reducing duplication in Helm template commands.The Helm template commands are repeated for each Kubernetes version with minimal differences. This could lead to maintenance issues.
Consider using a loop or function to generate templates:
KUBE_VERSIONS := v1.17 v1.18 v1.19 v1.20 v1.21 v1.22 v1.23 v1.24 v1.25 v1.26 v1.27 v1.28 v1.29 v1.30 define generate_helm_template helm template deploy/helm/lb-csi/ \ --namespace=kube-system \ --set allowExpandVolume=true \ --set enableSnapshot=true \ --set kubeVersion=$(1) \ --set imageRegistry=$(DOCKER_REGISTRY) \ --set sidecarImageRegistry=$(SIDECAR_DOCKER_REGISTRY) \ --set image=$(DOCKER_TAG) > deploy/k8s/lb-csi-plugin-k8s-$(1).yaml helm template deploy/helm/lb-csi/ \ --namespace=kube-system \ --set allowExpandVolume=true \ --set enableSnapshot=true \ --set discoveryClientInContainer=true \ --set kubeVersion=$(1) \ --set imageRegistry=$(DOCKER_REGISTRY) \ --set sidecarImageRegistry=$(SIDECAR_DOCKER_REGISTRY) \ --set image=$(DOCKER_TAG) \ --set discoveryClientImage=$(DISCOVERY_CLIENT_DOCKER_TAG) > deploy/k8s/lb-csi-plugin-k8s-$(1)-dc.yaml endef lb-csi-manifests: verify_image_registry deploy/k8s $(foreach ver,$(KUBE_VERSIONS),$(call generate_helm_template,$(ver)))
🧹 Nitpick comments (2)
lb.yaml (1)
9-20: Consider optimizing shared dependencies.Both components share many common dependencies. Consider extracting common dependencies into a shared section to improve maintainability.
Example refactor:
los-csi: + common_deps: &common_deps + - file://Makefile + - file://Makefile.lb + - file://main.go + - file://pkg + - file://go.mod + - file://go.sum + - file://VERSION lb-csi-plugin: build: - make -f Makefile.lb docker-build docker-bundle install: - make -f Makefile.lb install checkin: - make -f Makefile.lb checkin deps: - - file://Makefile - - file://Makefile.lb - - file://main.go - - file://pkg + <<: *common_deps - file://deploy/Dockerfile - file://Dockerfile.builder - file://deploy/licenses - - file://go.mod - - file://go.sum - - file://VERSION - file://lb.yaml lb-csi-bundle: build: - make -f Makefile.lb docker-bundle deps: - - file://Makefile - - file://Makefile.lb - - file://main.go - - file://pkg + <<: *common_deps - file://deploy - - file://go.mod - - file://go.sum - - file://VERSIONAlso applies to: 24-32
Makefile.lb (1)
14-18: Enhance INSTALL_PATH validation.The current validation only checks if INSTALL_PATH is empty. Consider adding additional validation for path safety.
ifeq ($(INSTALL_PATH),"") $(error "lb-csi install path is not specified") endif +ifeq ($(shell test -d $(dir $(INSTALL_PATH)) || echo invalid),invalid) + $(error "lb-csi install path parent directory does not exist") +endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefile(14 hunks)Makefile.lb(1 hunks)lb.yaml(1 hunks)
🔇 Additional comments (4)
lb.yaml (1)
1-32: LGTM! Clear and well-structured configuration.
The configuration is well-organized with clear separation of components and their respective build, installation, and dependency specifications.
Makefile.lb (1)
1-54: LGTM! Well-structured Makefile with clear target definitions.
The Makefile follows good practices:
- Clear variable definitions with fallbacks
- Proper use of override directives
- Well-organized targets with dependencies
Makefile (2)
Line range hint 150-385: LGTM! Consistent integration of discovery client image.
The changes correctly integrate the discovery client image parameter across all Kubernetes versions.
Line range hint 150-385: Verify Kubernetes version compatibility range.
The manifest generation supports Kubernetes v1.17 through v1.30. Please verify if this range aligns with the CSI plugin's compatibility requirements.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information about the Kubernetes version compatibility:
Kubernetes version compatibility range is appropriate
The manifest generation for v1.17 through v1.30 aligns with the CSI plugin's compatibility requirements. The documentation confirms:
- Minimum supported version is v1.16 and later for CSI volume expansion
- Core deployment instructions cover Kubernetes v1.17 and above
- Plugin is actively tested and maintained across these versions, with proper sidecar container versions for each Kubernetes release
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check CSI spec compatibility with Kubernetes versions
# Check CSI spec version in go.mod
rg -A 5 "k8s.io/csi-translation-lib" go.mod
# Check minimum supported Kubernetes version in documentation
rg -i "kubernetes.*version|version.*kubernetes" README.md docs/*
Length of output: 5452
Makefile.lb
Outdated
| HELM_CHART_REPOSITORY := $(or $(HELM_CHART_REPOSITORY),http://helm-repo:8080) | ||
| HELM_CHART_REPOSITORY_USERNAME := $(or $(HELM_CHART_REPOSITORY_USERNAME),light) | ||
| HELM_CHART_REPOSITORY_PASSWORD := $(or $(HELM_CHART_REPOSITORY_PASSWORD),light) |
There was a problem hiding this comment.
Consider securing default credentials.
The default credentials for the Helm chart repository are hardcoded. This could pose a security risk if accidentally used in production.
Consider:
- Using environment variables without defaults
- Adding validation to ensure secure credentials are provided
-HELM_CHART_REPOSITORY_USERNAME := $(or $(HELM_CHART_REPOSITORY_USERNAME),light)
-HELM_CHART_REPOSITORY_PASSWORD := $(or $(HELM_CHART_REPOSITORY_PASSWORD),light)
+HELM_CHART_REPOSITORY_USERNAME := $(HELM_CHART_REPOSITORY_USERNAME)
+HELM_CHART_REPOSITORY_PASSWORD := $(HELM_CHART_REPOSITORY_PASSWORD)📝 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.
| HELM_CHART_REPOSITORY := $(or $(HELM_CHART_REPOSITORY),http://helm-repo:8080) | |
| HELM_CHART_REPOSITORY_USERNAME := $(or $(HELM_CHART_REPOSITORY_USERNAME),light) | |
| HELM_CHART_REPOSITORY_PASSWORD := $(or $(HELM_CHART_REPOSITORY_PASSWORD),light) | |
| HELM_CHART_REPOSITORY := $(or $(HELM_CHART_REPOSITORY),http://helm-repo:8080) | |
| HELM_CHART_REPOSITORY_USERNAME := $(HELM_CHART_REPOSITORY_USERNAME) | |
| HELM_CHART_REPOSITORY_PASSWORD := $(HELM_CHART_REPOSITORY_PASSWORD) |
|
@husam-lb From technical point of view about the changes you did, it looks good to me. However it seems to me that a review of someone that is familiar with the los-csi functionality and usage is required before merging the code into main tree. (@rahman-lb @yogev-lb ) |
| @@ -147,7 +147,8 @@ lb-csi-manifests: verify_image_registry deploy/k8s | |||
| --set kubeVersion=v1.17 \ | |||
| --set imageRegistry=$(DOCKER_REGISTRY) \ | |||
| --set sidecarImageRegistry=$(SIDECAR_DOCKER_REGISTRY) \ | |||
| --set image=$(DOCKER_TAG) > deploy/k8s/lb-csi-plugin-k8s-v1.17-dc.yaml | |||
| --set image=$(DOCKER_TAG) \ | |||
There was a problem hiding this comment.
Are you moving the tagging process done by the CI into this makefile ?
| @@ -147,7 +147,8 @@ lb-csi-manifests: verify_image_registry deploy/k8s | |||
| --set kubeVersion=v1.17 \ | |||
| --set imageRegistry=$(DOCKER_REGISTRY) \ | |||
| --set sidecarImageRegistry=$(SIDECAR_DOCKER_REGISTRY) \ | |||
| --set image=$(DOCKER_TAG) > deploy/k8s/lb-csi-plugin-k8s-v1.17-dc.yaml | |||
| --set image=$(DOCKER_TAG) \ | |||
| --set discoveryClientImage=$(DISCOVERY_CLIENT_DOCKER_TAG) > deploy/k8s/lb-csi-plugin-k8s-v1.17-dc.yaml | |||
There was a problem hiding this comment.
I think we're adding more maintenance complexity to this already complex makefile.
How about if you this ?
The actual change is minimum from one version to the other and follows a strict order.
Move the subpart of deploy/k8s/lb-csi-plugin-k8s-v1.28.yaml and deploy/k8s/lb-csi-plugin-k8s-v1.28-dc.yaml [ k8s-v1.17 - k8s-v1.30 ] into a separate input file as input of versionList.
Repeat loop for helm template as many times you have entries in versionList. This will reduce all these repeated lines 133 - 385 to 7/8 lines.
e.g
helm template deploy/helm/lb-csi/
--namespace=kube-system
--set allowExpandVolume=true
--set enableSnapshot=true
--set discoveryClientInContainer=true
--set kubeVersion=version[i]
--set imageRegistry=$(DOCKER_REGISTRY)
--set sidecarImageRegistry=$(SIDECAR_DOCKER_REGISTRY)
--set image=$(DOCKER_TAG)
--set discoveryClientImage=$(DISCOVERY_CLIENT_DOCKER_TAG) > deploy/k8s/lb-csi-plugin-version[i] -dc.yaml
There was a problem hiding this comment.
let me know your thoughts .........
There was a problem hiding this comment.
Yes I think it's complicated but let's do this as part of another PR.
I think we don't need to support all the versions, we should support latest versions of k8s only.
Let's do this along with updating the containers.
husam-lb
force-pushed
the
husam/use_los_csi
branch
from
028c9e3 to
98abd45
Compare
|
Add lb.yaml and Makefile.lb missing so this repo can be run with internal build tools.
Issue: LBM1-35810
Summary by CodeRabbit
New Features
lb-csi-plugin.lb.yamlforlb-csi-pluginandlb-csi-bundle, detailing build and installation commands.Bug Fixes
INSTALL_PATHis specified to avoid errors during the build process.