Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Kubernetes version string handling by stripping metadata #623

Merged

Conversation

Nimbus318
Copy link
Contributor

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR fixes an issue where Kubernetes version strings with additional metadata (e.g., +k3s1) caused problems in dynamically generated image tags. The new strippedKubeVersion function removes the part after the + in the Kubernetes version string, ensuring consistent and predictable image tagging.

Which issue(s) this PR fixes:
Fixes #621

Special notes for your reviewer:
@archlitchi
@wawa0210

Does this PR introduce a user-facing change?:
Yes, this PR restores compatibility for specifying certain parameters during helm install/upgrade.

After version 2.4.1, users were unable to specify parameters like scheduler.kubeScheduler.tag (scheduler image version), devicePlugin.deviceSplitCount, devicePlugin.deviceMemoryScaling, and devicePlugin.deviceCoreScaling directly during Helm operations.

With this PR merged:

  • Users can now specify the scheduler image version using scheduler.kubeScheduler.tag during helm install or helm upgrade. If the parameter is not provided, the image version will continue to be auto-generated based on the Kubernetes version.
  • Parameters like deviceSplitCount, deviceMemoryScaling, and deviceCoreScaling are also restored for direct specification during Helm operations.

Copy link

codecov bot commented Nov 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Flag Coverage Δ
unittests 26.80% <ø> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

@wawa0210
Copy link
Member

wawa0210 commented Nov 18, 2024

Very good bug fix, have we done some testing? For example, can it work properly in the K3s environment?

also,you should rebase your commits into one commit

…er `+`

This fix ensures that Kubernetes version strings with additional metadata (e.g., `+k3s1`) are properly handled. The function `strippedKubeVersion` is introduced to remove the part after the `+`, resolving issues with version compatibility in dynamically generated image tags.

Signed-off-by: Nimbus <[email protected]>
@Nimbus318
Copy link
Contributor Author

I have rebased the commits into one as requested.

Regarding testing, I have not tested this in a real K3s environment. However, I conducted mock testing for this function locally by injecting a mock Kubernetes version value in _helpers.tpl for validation. Below are the steps I followed:

Mock the Kubernetes version by temporarily injecting a value for testing:

{{/*
    Mock Kubernetes version for testing
*/}}
{{- define "mockedCapabilitiesKubeVersion" -}}
v1.31.1+k3s1
{{- end }}

Modify the existing logic to replace Capabilities.KubeVersion.Version with the mock value:

{{- define "strippedKubeVersion" -}}
{{- $parts := split "+" (include "mockedCapabilitiesKubeVersion" .) -}}
{{- print $parts._0 -}}
{{- end }}

Validated that the output correctly strips the metadata after +.

@wawa0210 wawa0210 merged commit 5396897 into Project-HAMi:master Nov 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InvalidImageName when deploying HAMi after latest update
2 participants