fix: change kube_vip to kube-vip#3118
Conversation
Signed-off-by: redscholar <blacktiledhouse@gmail.com>
kubesphere-prow
Bot
added
release-note
kind/bug
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: redscholar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubesphere-prow
Bot
added
the
approved
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request standardizes the naming of the control plane endpoint type by replacing "kube_vip" with "kube-vip" across various Ansible tasks, configuration defaults, and documentation files. The reviewer raised critical feedback regarding backward compatibility, pointing out that strictly checking for "kube-vip" will break operations (such as uninstallation, initialization, prechecks, and DNS configuration) on existing clusters that were deployed using the older "kube_vip" value. It is recommended to update the conditional statements to support both "kube-vip" and "kube_vip" values.
|
|
||
| - name: Network | Remove ARP and IP address entries for kube-vip | ||
| when: eq .kubernetes.control_plane_endpoint.type "kube_vip" | ||
| when: eq .kubernetes.control_plane_endpoint.type "kube-vip" |
There was a problem hiding this comment.
Changing the type check strictly to kube-vip will break uninstallation for existing clusters that were deployed with kube_vip in their configuration. To ensure backward compatibility and prevent leaving stale ARP/IP entries on the nodes during uninstallation, we should support both kube-vip and kube_vip values.
when: or (eq .kubernetes.control_plane_endpoint.type "kube-vip") (eq .kubernetes.control_plane_endpoint.type "kube_vip")|
|
||
| - name: Delete arp by kube-vip | ||
| when: eq .kubernetes.control_plane_endpoint.type "kube_vip" | ||
| when: eq .kubernetes.control_plane_endpoint.type "kube-vip" |
There was a problem hiding this comment.
To maintain backward compatibility for existing clusters that were installed with kube_vip, we should allow both kube-vip and kube_vip during the uninstallation process. Otherwise, the ARP/IP cleanup tasks will be skipped for those clusters.
when: or (eq .kubernetes.control_plane_endpoint.type "kube-vip") (eq .kubernetes.control_plane_endpoint.type "kube_vip")| when: | ||
| - .kubernetes.kube_version | semverCompare ">=v1.29.0" | ||
| - eq .kubernetes.control_plane_endpoint.type "kube_vip" | ||
| - eq .kubernetes.control_plane_endpoint.type "kube-vip" |
There was a problem hiding this comment.
To ensure backward compatibility for existing clusters during initialization/upgrade phases, we should accept both kube-vip and kube_vip values.
- or (eq .kubernetes.control_plane_endpoint.type "kube-vip") (eq .kubernetes.control_plane_endpoint.type "kube_vip")| when: | ||
| - .kubernetes.kube_version | semverCompare ">=v1.29.0" | ||
| - eq .kubernetes.control_plane_endpoint.type "kube_vip" | ||
| - eq .kubernetes.control_plane_endpoint.type "kube-vip" |
There was a problem hiding this comment.
To ensure backward compatibility for existing clusters during initialization/upgrade phases, we should accept both kube-vip and kube_vip values.
- or (eq .kubernetes.control_plane_endpoint.type "kube-vip") (eq .kubernetes.control_plane_endpoint.type "kube_vip")| include_tasks: high-availability/kube_vip.yaml | ||
| when: | ||
| - .kubernetes.control_plane_endpoint.type | eq "kube_vip" | ||
| - .kubernetes.control_plane_endpoint.type | eq "kube-vip" |
There was a problem hiding this comment.
To ensure backward compatibility for existing clusters during deployment or scaling operations, we should support both kube-vip and kube_vip values.
- or (.kubernetes.control_plane_endpoint.type | eq "kube-vip") (.kubernetes.control_plane_endpoint.type | eq "kube_vip")| cat >> {{ .item }} <<EOF | ||
| # kubekey kubernetes control_plane_endpoint BEGIN | ||
| {{- if .kubernetes.control_plane_endpoint.type | eq "kube_vip" }} | ||
| {{- if .kubernetes.control_plane_endpoint.type | eq "kube-vip" }} |
There was a problem hiding this comment.
To ensure backward compatibility for existing clusters, the DNS configuration should handle both kube-vip and kube_vip values.
{{- if or (.kubernetes.control_plane_endpoint.type | eq "kube-vip") (.kubernetes.control_plane_endpoint.type | eq "kube_vip") }}| - name: Kubernetes | Validate kube-vip address | ||
| run_once: true | ||
| when: .kubernetes.control_plane_endpoint.type | eq "kube_vip" | ||
| when: .kubernetes.control_plane_endpoint.type | eq "kube-vip" |
There was a problem hiding this comment.
To ensure backward compatibility for existing clusters during precheck, we should validate the kube-vip address if the type is either kube-vip or kube_vip.
when: or (.kubernetes.control_plane_endpoint.type | eq "kube-vip") (.kubernetes.control_plane_endpoint.type | eq "kube_vip")| fail_msg: >- | ||
| "kubernetes.control_plane_endpoint.kube_vip.address" should be a un-used ip address. | ||
| when: .kubernetes.control_plane_endpoint.type | eq "kube_vip" | ||
| when: .kubernetes.control_plane_endpoint.type | eq "kube-vip" |
There was a problem hiding this comment.
To ensure backward compatibility for existing clusters during precheck, we should validate the kube-vip address if the type is either kube-vip or kube_vip.
when: or (.kubernetes.control_plane_endpoint.type | eq "kube-vip") (.kubernetes.control_plane_endpoint.type | eq "kube_vip")
1 participant
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for reviewers:
Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: