Apply calico bgp peer definition task to all nodes #8974
Conversation
k8s-ci-robot
added
kind/bug
|
Hi @orange-llajeanne. 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 Once the patch is verified, the new status will be reflected by the 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
added
needs-ok-to-test
| @@ -571,7 +571,7 @@ | |||
| - "{{ peers|selectattr('scope','undefined')|list|default([]) | union(peers|selectattr('scope','defined')|selectattr('scope','equalto', 'node')|list|default([])) }}" | |||
| when: | |||
| - peer_with_router|default(false) | |||
| - inventory_hostname == groups['kube_control_plane'][0] | |||
There was a problem hiding this comment.
The reason this was limited to kube_control_plane[0] was because there is where the calicoctl.sh script is configured, I don't think we intended to limit the coverage to a single node. I think your fix needs a delegate_to: kube_control_plane[0] to actually achieve its purpose to ensure ti calls calicoctl.sh on the right machine and with the right input for each cluster member.
There was a problem hiding this comment.
I updated this PR with a delegate_to to ensure that all calicoctl.sh calls are done on the first node. This is a different behavior than the one that was in place before #8833 , but I has the same effects so it's fine by me.
I'd like more details about the part of your answer that states that calicoctl.sh is not configured everywhere: in my understanding, this script is deployed (and functional) on all cluster nodes. This is why the task that this PR modifies was working up to the recent changes.
Is this not supposed to be the case anymore? Is calicoctl.sh planned to only be available on control plane nodes?
There was a problem hiding this comment.
You are correct that calicoctl.sh is deployed on all nodes, I was under a wrong impression that we only deployed it on control plane nodes. Since we render and apply manifests only on the first control plane node, it makes sense to have the same approach for calico commands even though they could be ran from any node. In the end I'm impartial to the implementation but open to other perspectives.
first control plane node
orange-llajeanne
force-pushed
the
fix_calico_node_router_peers
branch
from
4a0202e to
dfeea4d
Compare
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cristicalin, orange-llajeanne 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 |
k8s-ci-robot
added
the
approved
|
/lgtm |
k8s-ci-robot
added
the
lgtm
…ubernetes-sigs#8974) first control plane node
…ubernetes-sigs#8974) first control plane node
…ubernetes-sigs#8974) first control plane node
What type of PR is this?
/kind bug
What this PR does / why we need it:
This partly reverts PR #8833 , which limited the
Configure peering with router(s) at node scopetask to a single node.We actually need to keep applying this task to all nodes, since each k8s node should be able to define its own bgp peering configuration.
The fact that
inventory_hostnameis used in the BGPPeer definition seems to suggest that this task was never intended to be limited to a single arbitrary node.Which issue(s) this PR fixes:
Kubespray currently fails to configure calico on a multiple node cluster with router peering, only the peering configuration of the first control plane node is defined.
Does this PR introduce a user-facing change?: