OCPBUGS-50599: Enforce VIPs to be collocated at the same host #4844
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MaysaMacedo The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
MaysaMacedo
force-pushed
the
collocate-vips
branch
2 times, most recently
from
a3eeb7f to
346562a
Compare
|
/test e2e-openstack-dualstack |
MaysaMacedo
force-pushed
the
collocate-vips
branch
2 times, most recently
from
5144a60 to
810e374
Compare
|
/test e2e-openstack-dualstack |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
MaysaMacedo
changed the title
openshift-ci-robot
added
jira/severity-important
|
@MaysaMacedo: This pull request references Jira Issue OCPBUGS-50599, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@MaysaMacedo: This pull request references Jira Issue OCPBUGS-50599, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
| vrrp_sync_group VG_API { | ||
| group { | ||
| {{`{{ range $i, $config := .Configs }}`}} | ||
| {{`{{ .Cluster.Name }}`}}_API_{{`{{$i}}`}} |
There was a problem hiding this comment.
@mkowalski This PR worked, but I wonder if I should take into consideration the participateInAPIVRPP and
participateInIngressVRPP, like it's done at line 106, to define who will be part of the respective groups. Do you know?
There was a problem hiding this comment.
If you ignore the {{if $participateInAPIVRPP}} part what will happen is that you will have vrrp_sync_group also present in the nodes that do not have API VIP configuration (e.g. Ingress nodes that are not Masters)
I honestly don't know if keepalived will refuse to start or simply treat this stanza as "do nothing".
I would say that if everything worked with this change you did, then it works. Otherwise you would see keepalived pod failing to start in one of your nodes -- unless you deployed a cluster where all the nodes are master nodes.
If the latter is your case, you should re-run this PR on a cluster that has more nodes (basically you need a node which runs keepalived but does not run kubeapi-server)
There was a problem hiding this comment.
I just noticed this template is specific to masters, I need to update the workers as well.
There was a problem hiding this comment.
We need those checks I think as they were added to avoid this issue https://issues.redhat.com//browse/OCPBUGS-1565
There was a problem hiding this comment.
We need those checks I think as they were added to avoid this issue https://issues.redhat.com//browse/OCPBUGS-1565
They were added for the unicast peer list, but the vrrp_sync_group only defines which groups are bundled together. So what I was trying to say above -- it all depends on what is the behaviour of vrrp_sync_group if it contains a group member that does not exist. If it ignores it, it's all good. If it throws an error, then you imperatively need this additional check.
In either case, adding this check does not cost anything. It's not making the template super more complicated so feel free if you feel it's more clear
MaysaMacedo
force-pushed
the
collocate-vips
branch
3 times, most recently
from
0e176e1 to
db9d740
Compare
|
/test e2e-openstack-dualstack |
MaysaMacedo
force-pushed
the
collocate-vips
branch
from
db9d740 to
2347e2b
Compare
|
/test e2e-openstack-dualstack |
|
/test e2e-metal-ipi-ovn-dualstack |
templates/master/00-master/on-prem/files/keepalived-keepalived.yaml
Outdated
Show resolved
Hide resolved
templates/worker/00-worker/on-prem/files/keepalived-keepalived.yaml
Outdated
Show resolved
Hide resolved
When using dual-stack with OpenStack, both IPv4 and IPv6 share the same Neutron Port and this makes OVN thinks that both addresses are associated to the same Node, but that might not always be true as keepalived can put them in separate Nodes. To change that, let's make sure the API VIPs stays together through state changes, the same goes for Ingress VIPs.
MaysaMacedo
force-pushed
the
collocate-vips
branch
from
2347e2b to
19b28f8
Compare
|
I just updated the docs from the last review I got. But the e2e-openstack-dualstack job has passed in the last run. |
|
/test e2e-openstack-dualstack-v6primary |
|
@MaysaMacedo: The specified target(s) for The following commands are available to trigger optional jobs: Use In response to this:
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-sigs/prow repository. |
|
/test e2e-metal-ipi-ovn-dualstack |
|
@MaysaMacedo: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
|
/cc @mkowalski |
- What I did
When using dual-stack with OpenStack, both IPv4 and IPv6
share the same Neutron Port and this makes OVN thinks that
both addresses are associated to the same Node, but that might
not always be true as keepalived can put them in separate Nodes.
To change that, let's make sure the API VIPs stays together through
state changes, the same goes for Ingress VIPs.
- How to verify it
- Description for the changelog