change the name of host-addresses annotation to host-cidrs #3915

JacobTanenbaum · 2023-09-20T19:29:51Z

commit: 8847618bbf2feeb42c30375a6de6f91afe011d91 changed the host-addresses annotation to include a subnet mask. This causes updates to experiance a network disruption as masters and nodes expect different data to be in the host-addresses annotation.

Changing the name of the host-addresses annotation to host-cidrs will eliminate the network disruption because nodes updating will not remove the host-addresses annotation and once upgraded masters will do the right thing with the host-cidrs annotation

- What this PR does and why is it needed

- Special notes for reviewers

- How to verify it

- Description for the changelog

martinkennelly

Some extra changes needed:

go-controller/pkg/node/node_ip_handler_linux.go:184:
go-controller/pkg/ovn/controller/services/services_controller.go:473
go-controller/pkg/ovn/controller/services/services_controller.go:486
go-controller/pkg/ovn/controller/services/node_tracker.go:254
go-controller/pkg/ovn/egressservices_test.go:1465
go-controller/pkg/ovn/ovn.go:359

Do you think we need to rename hostAddress below (go-controller/pkg/ovn/controller/services/services_controller.go) ? I am on the fench - up to you:

type nodeInfo struct {
	// the node's Name
	name string
	// The list of physical IPs reported by the gatewayconf annotation
	l3gatewayAddresses []net.IP
	// The list of physical IPs and subnet masks the node has, as reported by the host-cidrs annotation
	hostAddresses []net.IP

go-controller/pkg/ovn/controller/services/services_controller.go:473
go-controller/pkg/ovn/controller/services/services_controller.go:486
go-controller/pkg/ovn/controller/services/node_tracker.go:254
go-controller/pkg/ovn/egressservices_test.go:1465
go-controller/pkg/ovn/ovn.go:359
go-controller/pkg/util/node_annotations.go:719
go-controller/pkg/util/node_annotations.go:726
go-controller/pkg/util/node_annotations.go:735
go-controller/pkg/util/node_annotations.go:742
go-controller/pkg/util/node_annotations.go:775
go-controller/pkg/clustermanager/egressip_controller.go:1395

martinkennelly · 2023-09-21T09:30:24Z

go-controller/pkg/clustermanager/egressip_controller.go

 	// does not contain EgressIPs that are assigned to interfaces.
 	for _, node := range nodes {
-		nodeHostAddressesSet, err := util.ParseNodeHostAddressesDropNetMask(node)
+		nodeHostAddressesSet, err := util.ParseNodeHostCIDRSDropNetMask(node)


nit but shouldnt we have the s in CIDRS lowercase here and the rest of the vars? WDYT?

nodeHostAddressesSet, err := util.ParseNodeHostCIDRsDropNetMask(node)

I like that I did it

test/e2e/service.go

martinkennelly · 2023-09-21T10:08:16Z

go-controller/pkg/node/node_ip_handler_linux.go

@@ -307,7 +307,7 @@ func (c *addressManager) doesNodeHostAddressesMatch() bool {
 	}
 	// check to see if ips on the node differ from what we stored
 	// in host-address annotation


change this to host-cidrs

changed it to doNodeHostCIDRsMatch

JacobTanenbaum · 2023-09-21T14:57:51Z

/retest

ovn-robot · 2023-09-21T14:58:15Z

Oops, something went wrong:

Must have admin rights to Repository.

coveralls · 2023-09-21T16:17:01Z

coverage: 50.167% (-0.04%) from 50.203% when pulling 8decf7c on JacobTanenbaum:disruption into c463cea on ovn-org:master.

martinkennelly · 2023-09-22T11:58:28Z

Thanks Jacob - some extra changes:

pkg/ovn/controller/services/node_tracker.go:134
pkg/clustermanager/egressip_controller.go +687

martinkennelly · 2023-09-22T12:03:54Z

pkg/node/controllers/egressip/egressip_test.go:900
pkg/node/node_ip_handler_linux.go:243
pkg/node/node_ip_handler_linux.go:288
pkg/node/node_ip_handler_linux.go:292
pkg/node/node_ip_handler_linux.go:339
pkg/ovn/controller/services/node_tracker.go:62
pkg/ovn/controller/services/node_tracker.go:252
pkg/ovn/controller/services/node_tracker.go:261
pkg/ovn/gateway_init.go:563
pkg/ovn/master.go:303
pkg/ovn/default_network_controller.go:892
pkg/ovn/egressip_test.go:158
pkg/ovn/egressip_test.go:190
pkg/ovn/hybrid_test.go:336
pkg/ovn/hybrid_test.go:627
pkg/ovn/hybrid_test.go:842
pkg/ovn/hybrid_test.go:1125
pkg/ovn/master_test.go:941
pkg/ovn/ovn.go:355
pkg/ovn/ovn.go:376
pkg/ovn/ovn.go:377
pkg/clustermanager/egressip_controller.go:1394
pkg/clustermanager/egressip_event_handler.go:110
pkg/clustermanager/egressip_event_handler.go:125

martinkennelly · 2023-09-22T16:00:59Z

CP failures unrelated to this PR and looks like upstream issue - retesting:

Error: 275.9 internal/k8s/k8s.go:45:2: k8s.io/kubernetes@v1.26.0: read "https:/proxy.golang.org/@v/v1.26.0.zip": read tcp 172.17.0.3:39578->142.250.115.207:443: read: connection reset by peer

trozet

double check places you changed hostAddrs to hostCIDRs. Not all of them should be named CIDRs as some are truly just IP addresses without a mask.

go-controller/pkg/clustermanager/egressip_controller.go

go-controller/pkg/ovn/controller/services/node_tracker.go

go-controller/pkg/ovn/ovn.go

trozet · 2023-09-25T21:09:11Z

I took the liberty of addressing the comments and pushing a new revision since @JacobTanenbaum is on PTO and this is urgent

trozet · 2023-09-25T22:36:11Z

@qinqon fyi this flake keeps happening on several PRs:

[Fail] Kubevirt Virtual Machines when live migrated [It] with pre-copy should keep connectivity
/home/runner/work/ovn-kubernetes/ovn-kubernetes/test/e2e/kubevirt.go:619

https://github.com/ovn-org/ovn-kubernetes/actions/runs/6304894472/job/17118087308?pr=3915

commit: 8847618 changed the host-addresses annotation to include a subnet mask. This causes updates to experiance a network disruption as masters and nodes expect different data to be in the host-addresses annotation. Changing the name of the host-addresses annotation to host-cidrs will eliminate the network disruption because nodes updating will not remove the host-addresses annotation and once upgraded masters will do the right thing with the host-cidrs annotation Co-authored-by: Tim Rozet <trozet@redhat.com> Signed-off-by: Jacob Tanenbaum <jtanenba@redhat.com>

qinqon · 2023-09-26T05:41:51Z

@qinqon fyi this flake keeps happening on several PRs:

[Fail] Kubevirt Virtual Machines when live migrated [It] with pre-copy should keep connectivity /home/runner/work/ovn-kubernetes/ovn-kubernetes/test/e2e/kubevirt.go:619

[Fail] Kubevirt Virtual Machines when live migrated [It] with pre-copy should keep connectivity /home/runner/work/ovn-kubernetes/ovn-kubernetes/test/e2e/kubevirt.go:619

https://github.com/ovn-org/ovn-kubernetes/actions/runs/6304894472/job/17118087308?pr=3915

We are tracking this here, #3902, I am trying to reproduce it, but more or less I think is the kubevirt pod that handles webhook is at bad state.

tssurya

/lgtm let's bring this downstream and ensure 12mins becomes 0mins for stable upgrades :D
some questions inline just to clarify some parts.
thank you @JacobTanenbaum @trozet

tssurya · 2023-09-26T06:50:12Z

go-controller/pkg/clustermanager/egressip_controller.go

 	// does not contain EgressIPs that are assigned to interfaces.
 	for _, node := range nodes {
-		nodeHostAddressesSet, err := util.ParseNodeHostAddressesDropNetMask(node)
+		nodeHostAddrsSet, err := util.ParseNodeHostCIDRsDropNetMask(node)


so when we upgrade from older version, since the node rolls out first, this new annotation will already be set by the time master's roll out and thus there wont be any lag/delay in processing ovnManagedEIPs existing in the cluster right?

Node IP handler will update/sync this annot under 2 conditions:

it get trigger either by an addr on the node being create/altered - we sub to events

30s time resync period

so the max delay can be ~30s

it does do a sync in-line when generating a new addr manager so we are good i think:

func newAddressManagerInternal(nodeName string, k kube.Interface, config *managementPortConfig, watchFactory factory.NodeWatchFactory, useNetlink bool) *addressManager { mgr := &addressManager{ nodeName: nodeName, watchFactory: watchFactory, addresses: sets.NewString(), mgmtPortConfig: config, OnChanged: func() {}, useNetlink: useNetlink, } mgr.nodeAnnotator = kube.NewNodeAnnotator(k, nodeName) mgr.sync()func newAddressManagerInternal(nodeName string, k kube.Interface, config *managementPortConfig, watchFactory factory.NodeWatchFactory, useNetlink bool) *addressManager { mgr := &addressManager{ nodeName: nodeName, watchFactory: watchFactory, addresses: sets.NewString(), mgmtPortConfig: config, OnChanged: func() {}, useNetlink: useNetlink, } mgr.nodeAnnotator = kube.NewNodeAnnotator(k, nodeName) mgr.sync() <--- HERE

tssurya · 2023-09-26T06:57:47Z

go-controller/pkg/node/node_ip_handler_linux.go

@@ -220,7 +220,7 @@ func (c *addressManager) handleNodePrimaryAddrChange() {
 }

 // updateNodeAddressAnnotations updates all relevant annotations for the node including
-// k8s.ovn.org/host-addresses, k8s.ovn.org/node-primary-ifaddr, k8s.ovn.org/l3-gateway-config.


the host-addresses annotation will live on forever? or during upgrades it will naturally vanish?

It should live on forever until we clean it in the future when theres no longer an upgrade path from before IC.

tssurya · 2023-09-26T07:09:47Z

go-controller/pkg/ovn/gateway_init.go

@@ -560,16 +560,16 @@ func (oc *DefaultNetworkController) addExternalSwitch(prefix, interfaceID, nodeN
 	return nil
 }

-func (oc *DefaultNetworkController) addPolicyBasedRoutes(nodeName, mgmtPortIP string, hostIfAddr *net.IPNet, otherHostAddrs []string) error {
+func (oc *DefaultNetworkController) addPolicyBasedRoutes(nodeName, mgmtPortIP string, hostIfCIDR *net.IPNet, otherHostAddrs []string) error {


NOTE: This path of code is not tested at all which is why this bug was not caught when the match of the policy changed from hostIP to hostIP/16. No code should be able to modify core routing policies on ovn_cluster_router and get away with it. Tests should complain.

matchstr2 := fmt.Sprintf(`inport == "rtos-%s" && ip4.dst == nodePhysicalIP /* %s */`, nodeName, nodeName) matchstr3 := fmt.Sprintf("ip4.src == source && ip4.dst == nodePhysicalIP") matchstr6 := fmt.Sprintf("ip4.src == NO DELETE && ip4.dst == nodePhysicalIP /* %s-no */", nodeName)

All current unit tests are playing with dummy match expresssions trying to leverage the comment on the actual policy:

We need unit test coverage in OVN DB libovsdb test framework in gateway_test.go and gateway_init.go for the match to prevent this from happening in the future. I will not block this PR for that but I will open an issue for this.

hmm not true, we do have some tests here: addNodeLogicalFlows however for nonIC setups these should have clearly failed - we got lucky with upgrades for ic only because db's are wiped, so nonIC->nonIC should have been stuck forever but it didn't unsure why.

tssurya · 2023-09-26T07:12:10Z

go-controller/pkg/ovnwebhook/nodeadmission.go

@@ -25,7 +25,7 @@ type checkNodeAnnot func(v annotationChange, nodeName string) error

 // commonNodeAnnotationChecks holds annotations allowed for ovnkube-node:<nodeName> users in non-IC and IC environments
 var commonNodeAnnotationChecks = map[string]checkNodeAnnot{
-	util.OvnNodeHostAddresses:            nil,
+	util.OVNNodeHostCIDRs:                nil,


I don't know the order of upgrades for the new node-identity component. My question is: Is there a chance that this new component shows up alongside 4.13 ovnkube-node's? Or will the new component show up only after all ovnkube-node's are in 4.14. Basically do we need to handle the presence of an old ovnkube-node that could potentially update the old annotation and fail due to webhooks not knowing about that annotation?

openshift-merge-robot · 2023-09-27T15:46:24Z

Fix included in accepted release 4.15.0-0.nightly-2023-09-27-073353

JacobTanenbaum requested review from trozet, dcbw, girishmg and jcaamano as code owners September 20, 2023 19:29

martinkennelly self-assigned this Sep 20, 2023

JacobTanenbaum force-pushed the disruption branch from 778261e to 3405987 Compare September 20, 2023 20:27

martinkennelly reviewed Sep 21, 2023

View reviewed changes

JacobTanenbaum force-pushed the disruption branch 2 times, most recently from 70d8c71 to f663355 Compare September 21, 2023 13:55

JacobTanenbaum force-pushed the disruption branch 2 times, most recently from 0f09f68 to d3fbd46 Compare September 21, 2023 15:51

JacobTanenbaum force-pushed the disruption branch from d3fbd46 to c451a4b Compare September 22, 2023 14:01

martinkennelly closed this Sep 22, 2023

martinkennelly reopened this Sep 22, 2023

trozet requested changes Sep 22, 2023

View reviewed changes

go-controller/pkg/clustermanager/egressip_controller.go Outdated Show resolved Hide resolved

go-controller/pkg/ovn/controller/services/node_tracker.go Outdated Show resolved Hide resolved

go-controller/pkg/ovn/ovn.go Outdated Show resolved Hide resolved

trozet force-pushed the disruption branch from 69aad47 to 8decf7c Compare September 25, 2023 21:07

trozet force-pushed the disruption branch from 8decf7c to 53d1c13 Compare September 25, 2023 22:40

tssurya approved these changes Sep 26, 2023

View reviewed changes

martinkennelly approved these changes Sep 26, 2023

View reviewed changes

trozet merged commit 6487d84 into ovn-org:master Sep 26, 2023
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change the name of host-addresses annotation to host-cidrs #3915

change the name of host-addresses annotation to host-cidrs #3915

JacobTanenbaum commented Sep 20, 2023

martinkennelly left a comment •

edited

martinkennelly Sep 21, 2023

JacobTanenbaum Sep 21, 2023

martinkennelly Sep 21, 2023

JacobTanenbaum Sep 21, 2023

JacobTanenbaum commented Sep 21, 2023

ovn-robot commented Sep 21, 2023

coveralls commented Sep 21, 2023 •

edited

martinkennelly commented Sep 22, 2023

martinkennelly commented Sep 22, 2023 •

edited

martinkennelly commented Sep 22, 2023

trozet left a comment

trozet commented Sep 25, 2023

trozet commented Sep 25, 2023

qinqon commented Sep 26, 2023

tssurya left a comment

tssurya Sep 26, 2023

martinkennelly Sep 26, 2023 •

edited

martinkennelly Sep 26, 2023 •

edited

tssurya Sep 26, 2023

martinkennelly Sep 26, 2023

tssurya Sep 26, 2023

tssurya Sep 26, 2023

tssurya Sep 26, 2023

openshift-merge-robot commented Sep 27, 2023

change the name of host-addresses annotation to host-cidrs #3915

change the name of host-addresses annotation to host-cidrs #3915

Conversation

JacobTanenbaum commented Sep 20, 2023

martinkennelly left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobTanenbaum commented Sep 21, 2023

ovn-robot commented Sep 21, 2023

coveralls commented Sep 21, 2023 • edited

martinkennelly commented Sep 22, 2023

martinkennelly commented Sep 22, 2023 • edited

martinkennelly commented Sep 22, 2023

trozet left a comment

Choose a reason for hiding this comment

trozet commented Sep 25, 2023

trozet commented Sep 25, 2023

qinqon commented Sep 26, 2023

tssurya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinkennelly Sep 26, 2023 • edited

Choose a reason for hiding this comment

martinkennelly Sep 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented Sep 27, 2023

martinkennelly left a comment •

edited

coveralls commented Sep 21, 2023 •

edited

martinkennelly commented Sep 22, 2023 •

edited

martinkennelly Sep 26, 2023 •

edited

martinkennelly Sep 26, 2023 •

edited