CSI - bump the current CSI version from v1.16 to v1.17 and update release documents #59
Conversation
WalkthroughThe pull request introduces updates to version numbers across several components of the Lightbits CSI Plugin. The main version has been incremented from Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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: 3
🧹 Outside diff range and nitpick comments (2)
deploy/helm/lb-csi-workload-examples/Chart.yaml (1)
Line range hint
3-4: Update chart and app versionsThe chart's
versionandappVersionfields need to be updated to align with the CSI version bump from v1.16 to v1.17:-version: 0.14.0 -appVersion: 1.16.0 +version: 0.15.0 +appVersion: 1.17.0docs/src/plugin_deployment/plugin_deployment_using_chart_in_bundle.md (1)
24-24: Fix formatting: Replace hard tabs with spaces in helm list outputThe helm list output contains hard tabs which should be replaced with spaces for consistent formatting across different editors and platforms.
Apply this formatting fix:
-lb-csi kube-system 1 2024-12-04... deployed lb-csi-plugin-0.13.0 1.17.0 +lb-csi kube-system 1 2024-12-04... deployed lb-csi-plugin-0.13.0 1.17.0🧰 Tools
🪛 Markdownlint
24-24: Column: 7
Hard tabs(MD010, no-hard-tabs)
24-24: Column: 19
Hard tabs(MD010, no-hard-tabs)
24-24: Column: 44
Hard tabs(MD010, no-hard-tabs)
24-24: Column: 53
Hard tabs(MD010, no-hard-tabs)
24-24: Column: 74
Hard tabs(MD010, no-hard-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
VERSION(1 hunks)deploy/helm/lb-csi-workload-examples/Chart.yaml(1 hunks)deploy/helm/lb-csi/Chart.yaml(1 hunks)deploy/helm/lb-csi/values.yaml(1 hunks)docs/metadata.md(1 hunks)docs/src/CHANGELOG/CHANGELOG-1.17.0.md(1 hunks)docs/src/CHANGELOG/README.md(1 hunks)docs/src/SUMMARY.md(1 hunks)docs/src/introduction.md(1 hunks)docs/src/plugin_deployment/deployment.md(3 hunks)docs/src/plugin_deployment/plugin_deployment_using_chart_in_bundle.md(2 hunks)docs/src/plugin_deployment/plugin_deployment_using_lightbits_helm_repository.md(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- VERSION
- deploy/helm/lb-csi/Chart.yaml
- docs/src/CHANGELOG/README.md
- docs/src/SUMMARY.md
🧰 Additional context used
🪛 Markdownlint
docs/src/CHANGELOG/CHANGELOG-1.17.0.md
10-10: null
Bare URL used
(MD034, no-bare-urls)
23-23: null
Bare URL used
(MD034, no-bare-urls)
27-27: null
Bare URL used
(MD034, no-bare-urls)
docs/src/plugin_deployment/plugin_deployment_using_chart_in_bundle.md
24-24: Column: 7
Hard tabs
(MD010, no-hard-tabs)
24-24: Column: 19
Hard tabs
(MD010, no-hard-tabs)
24-24: Column: 44
Hard tabs
(MD010, no-hard-tabs)
24-24: Column: 53
Hard tabs
(MD010, no-hard-tabs)
24-24: Column: 74
Hard tabs
(MD010, no-hard-tabs)
🔇 Additional comments (9)
deploy/helm/lb-csi-workload-examples/Chart.yaml (1)
14-14: LGTM: Consistent dependency version updates
The version updates for all dependencies are consistent, bumping from 0.14.0 to 0.15.0 across all components while maintaining their respective conditions.
Also applies to: 17-17, 20-20, 23-23, 26-26, 29-29
deploy/helm/lb-csi/values.yaml (1)
11-11: LGTM - Discovery client image version update is in sync.
The discovery client image version has been updated to maintain consistency with the CSI plugin version.
Let's verify the discovery client configuration:
#!/bin/bash
# Description: Verify discovery client configuration and version references
# Check for any remaining old discovery client version references
echo "Checking for old discovery client version references:"
rg "lb-nvme-discovery-client:1.16.0"
# Verify discovery client settings
echo -e "\nChecking discovery client related configurations:"
rg "discoveryClient" --type yamldocs/src/plugin_deployment/plugin_deployment_using_chart_in_bundle.md (1)
77-77: LGTM: Version update is consistent
The image version update to 1.17.0 in the example command is consistent with the PR objectives and other documentation changes.
docs/src/plugin_deployment/plugin_deployment_using_lightbits_helm_repository.md (2)
25-28: LGTM! Version updates align with PR objectives.
The version updates correctly reflect the intended changes:
- CSI version bump from 1.16.0 to 1.17.0
- Chart version updates from 0.14.0 to 0.15.0
25-28:
Fix inconsistent snapshot-controller versions in documentation.
The helm search output shows snapshot-controller versions as 0.15.0, but later in the document, they are shown as 0.12.0. This inconsistency needs to be addressed.
Apply this diff to fix the inconsistency:
k8s/
-lightbits-helm-repo/snapshot-controller-3 0.12.0 3.0.3 Deploy snapshot-controller for K8s version < v1.20
-lightbits-helm-repo/snapshot-controller-4 0.12.0 4.2.1 Deploy snapshot-controller for K8s version >= v1.20
+lightbits-helm-repo/snapshot-controller-3 0.15.0 3.0.3 Deploy snapshot-controller for K8s version < v1.20
+lightbits-helm-repo/snapshot-controller-4 0.15.0 4.2.1 Deploy snapshot-controller for K8s version >= v1.20Likely invalid or redundant comment.
docs/metadata.md (1)
6-6: LGTM: Version update aligns with PR objectives
The CSI version update from v1.16.0 to v1.17.0 is consistent with the PR objectives and related changes across other files.
docs/src/introduction.md (1)
20-20: LGTM! Version update note is clear and accurate.
The upgrade recommendation clearly links Lightbits cluster version 3.12.1 with CSI plugin version 1.17.0, which aligns with the PR objectives.
docs/src/plugin_deployment/deployment.md (2)
239-240: LGTM: Helm charts version update is consistent
The Helm charts version update to 0.15.0 is properly documented and aligns with the version bump changes.
339-339: LGTM: CSI plugin image version update is accurate
The CSI plugin image version has been correctly updated to 1.17.0 in both StatefulSet and DaemonSet configurations, implementing the main objective of the PR.
Also applies to: 357-357
| @@ -1,14 +1,14 @@ | |||
| --- | |||
| enableExpandVolume: true | |||
| enableSnapshot: true | |||
| image: "lb-csi-plugin:1.16.0" | |||
| image: "lb-csi-plugin:1.17.0" | |||
There was a problem hiding this comment.
💡 Codebase verification
Version update verified, but one inconsistency found in workload examples
The CSI plugin version 1.17.0 is consistently used across the main Helm chart files:
values.yaml: Both CSI plugin and discovery client images use 1.17.0Chart.yaml: appVersion is set to 1.17.0
However, there's one remaining reference to version 1.16.0 in:
deploy/helm/lb-csi-workload-examples/Chart.yaml
This workload examples chart should also be updated to maintain version consistency across the repository.
🔗 Analysis chain
LGTM - CSI plugin image version update looks consistent.
The version bump from 1.16.0 to 1.17.0 aligns with the PR objectives.
Let's verify version consistency across deployment files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify CSI version consistency across deployment files
# Check VERSION file
echo "Checking VERSION file:"
cat VERSION
# Check Helm chart versions
echo -e "\nChecking Chart.yaml versions:"
cat deploy/helm/lb-csi/Chart.yaml
# Check for any remaining old version references
echo -e "\nChecking for any remaining old version references:"
rg "1.16.0" --type yaml
Length of output: 924
Script:
#!/bin/bash
# Let's try a more comprehensive version check
# Check for any 1.17.0 version references
echo "Checking 1.17.0 version references:"
rg "1.17.0" --type yaml
# Check for any remaining 1.16.0 references
echo -e "\nChecking for any 1.16.0 version references:"
rg "1.16.0" --type yaml
# Check the image references specifically
echo -e "\nChecking image references in values.yaml:"
cat deploy/helm/lb-csi/values.yaml | grep -E "image:|Image:"
# Check Chart.yaml for version info
echo -e "\nChecking Chart.yaml version info:"
cat deploy/helm/lb-csi/Chart.yaml | grep -E "version:|appVersion:"
Length of output: 1178
There was a problem hiding this comment.
@ron-lb real cool catch by codeRabbit, exactly type of issue easy to miss in reviews (we tend to review only what was changed and not what may also have needed to change).
@rahman-lb please update the above (i pushed a commit for missing changes, ideally should squash but not critical)
There was a problem hiding this comment.
@coderabbitai thanks for correct feedback. in same file seems like the helm chart version that was also updated from 1.14.0 to 1.15.0 was missed by code changes and this review
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!
There was a problem hiding this comment.
@ronen-lb, thank you for catching that. It appears I missed the Helm chart version update from 0.14.0 to 0.15.0 in the lb-csi-workload-examples chart. I'll make sure to check for such inconsistencies in future reviews.
✏️ Learnings added
Learnt from: ronen-lb
PR: LightBitsLabs/los-csi#59
File: deploy/helm/lb-csi/values.yaml:4-4
Timestamp: 2024-11-12T12:49:21.628Z
Learning: When reviewing version updates, verify that Helm chart versions are updated consistently across all `Chart.yaml` files, including `deploy/helm/lb-csi-workload-examples/Chart.yaml`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| subtitle: | | ||
|
|
||
| LightOS Version: v3.10.1 | ||
|
|
||
| Kubernetes Versions: v1.17 - v1.24 | ||
| Kubernetes Versions: v1.17 - v1.30 |
There was a problem hiding this comment.
💡 Codebase verification
Kubernetes version v1.30 is not validated or supported
Based on the search results, there is clear evidence that:
- The latest documented Kubernetes version support is v1.24 as shown in multiple files:
- Deployment manifests only go up to
lb-csi-plugin-k8s-v1.24.yaml - Feature matrix in
docs/src/introduction.mdonly lists up to "K8s v1.24"
- Deployment manifests only go up to
- The changelog entries (CHANGELOG-1.11.0.md) show incremental version support additions, with v1.24 being the last documented addition
- No documentation, test results, or deployment manifests exist for versions beyond v1.24
The metadata.md file should be updated to reflect the actual tested and supported Kubernetes versions: v1.17 - v1.24.
🔗 Analysis chain
Please verify the Kubernetes version range
The extension of supported Kubernetes versions up to v1.30 raises concerns:
- Kubernetes v1.30 hasn't been released yet (current latest stable is v1.28)
- Supporting future versions would require extensive testing and validation
- This change wasn't mentioned in the PR objectives
Consider keeping the upper bound to a currently available Kubernetes version that has been properly tested with this CSI version.
Let's verify the supported Kubernetes versions in other documentation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Kubernetes version references in documentation and configuration
rg -g '!{.git,vendor}' -i 'kubernetes.*v1\.[0-9]+' docs/
rg -g '!{.git,vendor}' -i 'k8s.*v1\.[0-9]+' docs/
Length of output: 8068
There was a problem hiding this comment.
@rahman-lb seems like support for version 30 was not merged to our master?
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!
There was a problem hiding this comment.
@rahman-lb why not add as commits to this PR?
There was a problem hiding this comment.
@ronen-lb I wanted to separate the PR because
- If something goes wrong in support version from v1.25 -v1.30 , we don't need to change anything on PR bumping version (except version v.130 mentioned)
- Any new CSI release uses previous version's bumping PR as references, and i wanted to keep the reference PR as much clean as possible.
|
@elada-lb is the PR check ready and we can run it on it? |
There was a problem hiding this comment.
@rahman-lb looks legit, please seem comment from codeRabbit/Myself.
please also mention in PR how was this tested
fix examples for helm chart that were missed in original commit. This commit can/should be squashed with initall commit. Signed-off-by: ronen <[email protected]>
|
@rahman-lb please add to ticket how this was verified, we don't currtnly have PR for this, so as muli mentioned, need manual sanity test for this |
| @@ -1,14 +1,14 @@ | |||
| --- | |||
| enableExpandVolume: true | |||
| enableSnapshot: true | |||
| image: "lb-csi-plugin:1.16.0" | |||
| image: "lb-csi-plugin:1.17.0" | |||
There was a problem hiding this comment.
@ron-lb real cool catch by codeRabbit, exactly type of issue easy to miss in reviews (we tend to review only what was changed and not what may also have needed to change).
@rahman-lb please update the above (i pushed a commit for missing changes, ideally should squash but not critical)
| @@ -1,14 +1,14 @@ | |||
| --- | |||
| enableExpandVolume: true | |||
| enableSnapshot: true | |||
| image: "lb-csi-plugin:1.16.0" | |||
| image: "lb-csi-plugin:1.17.0" | |||
There was a problem hiding this comment.
@coderabbitai thanks for correct feedback. in same file seems like the helm chart version that was also updated from 1.14.0 to 1.15.0 was missed by code changes and this review
Not ready yet, I'll need to know which system tests are required to run in the PR check and can prepare it ASAP |
|
@elada-lb you can start with test # 405_lb_csi_k8s_1_30_smoke |
|
FYI, I'll be testing the change today, via automated test and some edge cases manually, |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
README.md(1 hunks)
🔇 Additional comments (1)
README.md (1)
13-13: Verify version alignment with other components
The LightOS version 3.12.x compatibility aligns with the PR objectives mentioning Lightbits core v3.12.1. However, let's verify that all version numbers are consistently updated across the codebase.
README.md
Outdated
| @@ -10,7 +10,7 @@ | |||
|
|
|||
| The LightOS CSI plugin is a software module that implements management of persistent storage volumes exported by LightOS software for Container Orchestrator (CO) systems like Kubernetes and Mesos. In conjunction with the LightOS disaggregated storage solution, the CSI plugin provides a building block for the easy deployment of stateful containerized applications on CO clusters. | |||
|
|
|||
| The version of the plugin covered by this document implements version 1.2 of the [Container Storage Interface (CSI) Specification](https://github.com/container-storage-interface/spec/blob/v1.2.0/spec.md), and is compatible with LightOS version 2.3.x | |||
| The version of the plugin covered by this document implements version 1.3 of the [Container Storage Interface (CSI) Specification](https://github.com/container-storage-interface/spec/blob/v1.2.0/spec.md), and is compatible with LightOS version 3.12.x | |||
There was a problem hiding this comment.
Fix inconsistent CSI specification link
The line states that the plugin implements CSI Specification v1.3, but the link points to v1.2.0 of the spec. Please update the link to point to the correct version.
Apply this change:
-The version of the plugin covered by this document implements version 1.3 of the [Container Storage Interface (CSI) Specification](https://github.com/container-storage-interface/spec/blob/v1.2.0/spec.md), and is compatible with LightOS version 3.12.x
+The version of the plugin covered by this document implements version 1.3 of the [Container Storage Interface (CSI) Specification](https://github.com/container-storage-interface/spec/blob/v1.3.0/spec.md), and is compatible with LightOS version 3.12.x📝 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.
| The version of the plugin covered by this document implements version 1.3 of the [Container Storage Interface (CSI) Specification](https://github.com/container-storage-interface/spec/blob/v1.2.0/spec.md), and is compatible with LightOS version 3.12.x | |
| The version of the plugin covered by this document implements version 1.3 of the [Container Storage Interface (CSI) Specification](https://github.com/container-storage-interface/spec/blob/v1.3.0/spec.md), and is compatible with LightOS version 3.12.x |
|
Adding documentation for new CSI release v1.17.0 with Lightbits core v3.12.1
Testing Performed.
Issue : LBM1-34848
Summary by CodeRabbit
Release Notes for Version 1.17.0
New Features
Improvements
Changelog