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.

Sign up for GitHub

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

OCPBUGS-50599: Enforce VIPs to be collocated at the same host #4844

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+95 −0
Open

OCPBUGS-50599: Enforce VIPs to be collocated at the same host #4844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions templates/master/00-master/on-prem/files/keepalived-keepalived.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,63 @@ contents:
fall 2
}

# When using dual-stack with OpenStack, both IPv4 and IPv6 share the same Neutron Port,
# causing OVN to assume both addresses belong to the same node, which may not always be the case.
# To address this, we ensure that the API VIPs remain grouped through state changes,
# the same goes for Ingress VIPs.
{{- if and (eq .Infra.Status.PlatformStatus.Type "OpenStack") (or (eq .IPFamilies "DualStack") (eq .IPFamilies "DualStackIPv6Primary")) }}
vrrp_sync_group VG_API {
group {
{{`{{ range $i, $config := .Configs }}`}}
{{`{{$nonVirtualIP := .NonVirtualIP}}`}}

{{`{{$participateInAPIVRRP := not .EnableUnicast}}`}}
{{`{{- if .EnableUnicast}}
{{- range .LBConfig.Backends}}
{{- if eq $nonVirtualIP .Address}}
{{$participateInAPIVRRP = true}}
{{- end}}
{{- end}}
{{- end}}`}}

{{`{{if $participateInAPIVRRP}}`}}
{{`{{ .Cluster.Name }}`}}_API_{{`{{$i}}`}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just noticed this template is specific to masters, I need to update the workers as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need those checks I think as they were added to avoid this issue https://issues.redhat.com//browse/OCPBUGS-1565

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{{`{{ end }}`}}
{{`{{ end }}`}}
}
track_script {
chk_ocp_lb
chk_ocp_both
chk_mcs
}
}

vrrp_sync_group VG_INGRESS {
group {
{{`{{ range $i, $config := .Configs }}`}}
{{`{{$nonVirtualIP := .NonVirtualIP}}`}}
{{`{{$participateInIngressVRRP := not .EnableUnicast}}`}}
{{`{{- if .EnableUnicast}}
{{- range .IngressConfig.Peers}}
{{- if eq $nonVirtualIP .}}
{{$participateInIngressVRRP = true}}
{{- end}}
{{- end}}
{{- end}}`}}

{{`{{if $participateInIngressVRRP}}`}}
{{`{{ .Cluster.Name }}`}}_INGRESS_{{`{{$i}}`}}
{{`{{ end }}`}}
{{`{{ end }}`}}
}
track_script {
chk_ingress
chk_ingress_ready
chk_default_ingress
}
}
{{- end}}

{{`{{ range $i, $config := .Configs }}`}}
{{`{{$nonVirtualIP := .NonVirtualIP}}`}}

Expand Down Expand Up @@ -105,11 +162,13 @@ contents:
virtual_ipaddress {
{{`{{ .Cluster.APIVIP }}`}}/{{`{{ .Cluster.VIPNetmask }}`}} label vip
}
{{- if not (and (eq .Infra.Status.PlatformStatus.Type "OpenStack") (or (eq .IPFamilies "DualStack") (eq .IPFamilies "DualStackIPv6Primary"))) }}
track_script {
chk_ocp_lb
chk_ocp_both
chk_mcs
}
{{- end}}
}
{{`{{end}}`}}

Expand Down Expand Up @@ -146,11 +205,13 @@ contents:
virtual_ipaddress {
{{`{{ .Cluster.IngressVIP }}`}}/{{`{{ .Cluster.VIPNetmask }}`}} label vip
}
{{- if not (and (eq .Infra.Status.PlatformStatus.Type "OpenStack") (or (eq .IPFamilies "DualStack") (eq .IPFamilies "DualStackIPv6Primary"))) }}
track_script {
chk_ingress
chk_ingress_ready
chk_default_ingress
}
{{- end}}
}
{{`{{ end }}`}}
{{`{{ end }}`}}
34 changes: 34 additions & 0 deletions templates/worker/00-worker/on-prem/files/keepalived-keepalived.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,38 @@ contents:
fall 2
}

# When using dual-stack with OpenStack, both IPv4 and IPv6 share the same Neutron Port,
# causing OVN to assume both addresses belong to the same node, which may not always be the case.
# To address this, we ensure that the API VIPs remain grouped through state changes,
# the same goes for Ingress VIPs.
{{- if and (eq .Infra.Status.PlatformStatus.Type "OpenStack") (or (eq .IPFamilies "DualStack") (eq .IPFamilies "DualStackIPv6Primary")) }}
vrrp_sync_group VG_INGRESS {
group {
{{`{{ range $i, $config := .Configs }}`}}
{{`{{$nonVirtualIP := .NonVirtualIP}}`}}

{{`{{$participateInIngressVRRP := not .EnableUnicast}}`}}
{{`{{- if .EnableUnicast}}
{{- range .IngressConfig.Peers}}
{{- if eq $nonVirtualIP .}}
{{$participateInIngressVRRP = true}}
{{- end}}
{{- end}}
{{- end}}`}}

{{`{{if $participateInIngressVRRP}}`}}
{{`{{ .Cluster.Name }}`}}_INGRESS_{{`{{$i}}`}}
{{`{{ end }}`}}
{{`{{ end }}`}}
}
track_script {
chk_ingress
chk_ingress_ready
chk_default_ingress
}
}
{{- end}}

{{`{{ range $i, $config := .Configs }}`}}
{{`{{$nonVirtualIP := .NonVirtualIP}}`}}

Expand Down Expand Up @@ -70,11 +102,13 @@ contents:
virtual_ipaddress {
{{`{{ .Cluster.IngressVIP }}`}}/{{`{{ .Cluster.VIPNetmask }}`}} label vip
}
{{- if not (and (eq .Infra.Status.PlatformStatus.Type "OpenStack") (or (eq .IPFamilies "DualStack") (eq .IPFamilies "DualStackIPv6Primary"))) }}
track_script {
chk_ingress
chk_ingress_ready
chk_default_ingress
}
{{- end}}
}
{{`{{ end }}`}}
{{`{{ end }}`}}