-
Notifications
You must be signed in to change notification settings - Fork 29
Robot Upgrade: volcano chart upgrade from 1.9.0 to 1.13.0 #3643
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @weizhoublue, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a substantial upgrade to the Volcano Helm chart, advancing it from version 1.9.0 to 1.13.0. The update focuses on expanding the system's capabilities in resource management and scheduling, introducing new custom resources, and refining existing ones. It also enhances the configurability and operational aspects of Volcano components, ensuring a more robust and feature-rich batch scheduling experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request upgrades the Volcano Helm chart from version 1.9.0 to 1.13.0. The changes are extensive, including updates to CRD definitions, new components like vc-agent, and numerous new configuration options in values.yaml. While most of the changes correctly reflect the new version's features, I've identified several critical issues in the Helm templates that would prevent a successful deployment. These include incorrect image path construction in multiple deployment files and a templating error in the webhook configurations. These issues need to be addressed to ensure the chart is deployable.
| - -v={{.Values.custom.admission_log_level}} | ||
| - 2>&1 | ||
| image: {{ .Values.admission.image.registry }}/{{ .Values.admission.image.repository }}:{{.Values.admission.image.tag }} | ||
| image: {{ .Values.basic.image_registry }}/{{ .Values.admission.image.registry }}/{{ .Values.admission.image.repository }}:{{.Values.admission.image.tag }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The image path is constructed incorrectly by concatenating two different registries ({{ .Values.basic.image_registry }} and {{ .Values.admission.image.registry }}). This will result in an invalid image path like docker.io/docker.m.daocloud.io/... and cause image pull failures.
To fix this, you should probably remove the {{ .Values.basic.image_registry }}/ part. Please also review the image path construction in scheduler.yaml and controllers.yaml as they have the same issue. Note that this will use docker.m.daocloud.io as the registry, which is inconsistent with admission-init.yaml that uses docker.io. A consistent approach for defining the image registry should be adopted across the chart.
image: {{ .Values.admission.image.registry }}/{{ .Values.admission.image.repository }}:{{.Values.admission.image.tag }}| - --logtostderr | ||
| - --enable-healthz=true | ||
| - --leader-elect={{ .Values.custom.leader_elect_enable }} | ||
| image: {{ .Values.basic.image_registry }}/{{ .Values.controller.image.registry }}/{{ .Values.controller.image.repository }}:{{.Values.controller.image.tag }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The image path is constructed incorrectly by concatenating two different registries ({{ .Values.basic.image_registry }} and {{ .Values.controller.image.registry }}). This will result in an invalid image path and cause image pull failures. A similar issue exists in other component templates like admission.yaml and scheduler.yaml.
image: {{ .Values.controller.image.registry }}/{{ .Values.controller.image.repository }}:{{.Values.controller.image.tag }}| containers: | ||
| - name: {{ .Release.Name }}-scheduler | ||
| image: {{ .Values.scheduler.image.registry }}/{{ .Values.scheduler.image.repository }}:{{.Values.scheduler.image.tag }} | ||
| image: {{ .Values.basic.image_registry }}/{{ .Values.scheduler.image.registry }}/{{ .Values.scheduler.image.repository }}:{{.Values.scheduler.image.tag }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The image path is constructed incorrectly by concatenating two different registries ({{ .Values.basic.image_registry }} and {{ .Values.scheduler.image.registry }}). This will result in an invalid image path and cause image pull failures. A similar issue exists in other component templates like admission.yaml and controllers.yaml.
image: {{ .Values.scheduler.image.registry }}/{{ .Values.scheduler.image.repository }}:{{.Values.scheduler.image.tag }}| {{- if .Values.custom.enabled_admissions | regexMatch "/cronjobs/validate" }} | ||
| {{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conditional block is incorrect. The if statement is immediately closed on the next line, causing the ValidatingWebhookConfiguration for cronjobs to be created unconditionally. This is likely not the desired behavior and will also cause a template rendering error due to a mismatched {{- end }} later in the file.
The resource definition should be wrapped inside the if block. Please remove line 472 and add a corresponding {{- end }} after the resource definition.
3d04d39 to
5257dc9
Compare
336f857 to
08dc9c7
Compare
d99adff to
802e818
Compare
e2e2458 to
af789a2
Compare
988adf3 to
9d229d2
Compare
56b2066 to
081c380
Compare
0af0e0f to
9225501
Compare
9225501 to
1c7665e
Compare
Signed-off-by: robot <[email protected]>
1c7665e to
7cb3e48
Compare
I am robot, upgrade: project volcano chart upgrade from 1.9.0 to 1.13.0