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 the invalid kube vip manifest #8831

Merged

Conversation

yankay
Copy link
Member

@yankay yankay commented May 16, 2022

What type of PR is this?

/kind bug

What this PR does / why we need it:

the kube-vip.yml generated is wrong, and cannot start by kubelet.

Which issue(s) this PR fixes:

Fixes #8829

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

企业微信截图_16526835027912

Fix an issue the kube-vip manifest with extra space.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 16, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @yankay. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 16, 2022
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 16, 2022
Copy link
Contributor

@cristicalin cristicalin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @yankay , thanks for the fix! It would make sense to add kube-vip to one of the CI jobs (like https://github.com/kubernetes-sigs/kubespray/blob/master/tests/files/packet_centos7-flannel-addons-ha.yml) to make sure we test this part of the code.

@cristicalin
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 16, 2022
@hsiaand
Copy link

hsiaand commented May 16, 2022

@yankay I saw you have submitted a PR. But I discovered another issue that I ran into last night built in same kube-vip.yml file.
could you make that fix in this PR as well or should I open a new issue?

The Jinja2 template for vip_interface container a quote that results in double-quoted string

{% if kube_vip_interface %}
    - name: vip_interface
      value: "{{ kube_vip_interface | string | to_json }}"
{% endif %}
# journalctl -l|grep kube-vip|tail -1
May 16 13:54:01 rad-c1g1mst2 kubelet[28119]: E0516 13:54:01.918807   28119 file.go:187] "Could not process manifest file" err="/etc/kubernetes/manifests/kube-vip.yml: couldn't parse as pod(yaml: line 17: did not find expected key), please check config file" path="/etc/kubernetes/manifests/kube-vip.yml"
# grep -B1 -n eth /etc/kubernetes/manifests/kube-vip.yml
17-    - name: vip_interface
18:      value: ""eth0""

@yankay
Copy link
Member Author

yankay commented May 17, 2022

@yankay I saw you have submitted a PR. But I discovered another issue that I ran into last night built in same kube-vip.yml file. could you make that fix in this PR as well or should I open a new issue?

The Jinja2 template for vip_interface container a quote that results in double-quoted string

{% if kube_vip_interface %}
    - name: vip_interface
      value: "{{ kube_vip_interface | string | to_json }}"
{% endif %}
# journalctl -l|grep kube-vip|tail -1
May 16 13:54:01 rad-c1g1mst2 kubelet[28119]: E0516 13:54:01.918807   28119 file.go:187] "Could not process manifest file" err="/etc/kubernetes/manifests/kube-vip.yml: couldn't parse as pod(yaml: line 17: did not find expected key), please check config file" path="/etc/kubernetes/manifests/kube-vip.yml"
# grep -B1 -n eth /etc/kubernetes/manifests/kube-vip.yml
17-    - name: vip_interface
18:      value: ""eth0""

Thanks,I would fix it in this PR at https://github.com/kubernetes-sigs/kubespray/pull/8831/files#diff-cc0a0aeba170366b6f4a94689aefbad605f05a42f39e67448ed093a795e0d6a1L19.

@yankay yankay force-pushed the fix-invalid-kube-vip-manifest branch 2 times, most recently from 5195cde to be0bfea Compare May 17, 2022 03:05
@yankay yankay force-pushed the fix-invalid-kube-vip-manifest branch from be0bfea to 4593c6b Compare May 17, 2022 03:06
@oomichi
Copy link
Contributor

oomichi commented May 17, 2022

/cc @sathieu
/approve

@k8s-ci-robot
Copy link
Contributor

@oomichi: GitHub didn't allow me to request PR reviews from the following users: sathieu.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @sathieu
/approve

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oomichi, yankay

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2022
@sathieu
Copy link
Contributor

sathieu commented May 17, 2022

thanks for those fixes, and sorry for the error. I probably pushed the wrong commit, because it's working in my setup.

@cristicalin
Copy link
Contributor

Thanks for doing this @yankay

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2022
@k8s-ci-robot k8s-ci-robot merged commit 3d8f3bc into kubernetes-sigs:master May 18, 2022
@sathieu
Copy link
Contributor

sathieu commented May 18, 2022

[...] I probably pushed the wrong commit, because it's working in my setup.

Indeed, here was my local diff:

diff --git a/roles/kubernetes/node/templates/manifests/kube-vip.manifest.j2 b/roles/kubernetes/node/templates/manifests/kube-vip.manifest.j2
index 16246156..f23559dc 100644
--- a/roles/kubernetes/node/templates/manifests/kube-vip.manifest.j2
+++ b/roles/kubernetes/node/templates/manifests/kube-vip.manifest.j2
@@ -14,31 +14,31 @@ spec:
       value: {{ kube_vip_arp_enabled | string | to_json }}
     - name: port
       value: "6443"
-    {% if kube_vip_interface %}
+    {% if kube_vip_interface -%}
     - name: vip_interface
       value: "{{ kube_vip_interface | string | to_json }}"
-    {% endif %}
-    {% if kube_vip_services_interface %}
+    {% endif -%}
+    {% if kube_vip_services_interface -%}
     - name: vip_servicesinterface
       value: {{ kube_vip_services_interface | string | to_json }}
-    {% endif %}
-    {% if kube_vip_cidr %}
+    {% endif -%}
+    {% if kube_vip_cidr -%}
     - name: vip_cidr
       value: {{ kube_vip_cidr | string | to_json }}
-    {% endif %}
-    {% if kube_vip_controlplane_enabled %}
+    {% endif -%}
+    {% if kube_vip_controlplane_enabled -%}
     - name: cp_enable
       value: "true"
     - name: cp_namespace
       value: kube-system
     - name: vip_ddns
       value: {{ kube_vip_ddns_enabled | string | to_json }}
-    {% endif %}
-    {% if kube_vip_services_enabled %}
+    {% endif -%}
+    {% if kube_vip_services_enabled -%}
     - name: svc_enable
       value: "true"
-    {% endif %}
-    {% if kube_vip_leader_election_enabled %}
+    {% endif -%}
+    {% if kube_vip_leader_election_enabled -%}
     - name: vip_leaderelection
       value: "true"
     - name: vip_leaseduration
@@ -47,8 +47,8 @@ spec:
       value: "3"
     - name: vip_retryperiod
       value: "1"
-    {% endif %}
-    {% if kube_vip_bgp_enabled %}
+    {% endif -%}
+    {% if kube_vip_bgp_enabled -%}
     - name: bgp_enable
       value: "true"
     - name: bgp_routerid
@@ -61,11 +61,11 @@ spec:
       value: {{ kube_vip_bgp_peerpass | to_json }}
     - name: bgp_peeras
       value: {{ kube_vip_bgp_peeras | to_json }}
-    {% if kube_vip_bgppeers %}
+    {% if kube_vip_bgppeers -%}
     - name: bgp_peers
       value: {{ kube_vip_bgp_peeras | join(',')  | to_json }}
-    {% endif %}
-    {% endif %}
+    {% endif -%}
+    {% endif -%}
     - name: address
       value: {{ kube_vip_address | to_json }}
     image: {{ kube_vip_image_repo }}:{{ kube_vip_image_tag }}

(I had no kube_vip_interface, so didn't catch the double-double quotes)

Again, sorry ...

@oomichi oomichi added the kind/bug Categorizes issue or PR as related to a bug. label May 28, 2022
@oomichi oomichi mentioned this pull request May 28, 2022
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jun 30, 2023
* add Feature synchronized time checking

* fix-invalid-kube-vip-manifest
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Oct 23, 2023
* add Feature synchronized time checking

* fix-invalid-kube-vip-manifest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

invalid kube-vip.yml generated
6 participants