ovn-ic: node update missing static routes #3724
Conversation
flavio-fernandes
requested review from
trozet,
dcbw,
girishmg and
jcaamano
as code owners
flavio-fernandes
force-pushed
the
ovnic-static-routes
branch
from
2d04856
to
2939719
Compare
|
Verified fix by starting cluster in kind using Also by ensuring that ipv6 static routes on all nodes have the appropriate routes: |
flavio-fernandes
force-pushed
the
ovnic-static-routes
branch
2 times, most recently
from
84ad24f
to
8df1540
Compare
|
oh i had fixed this just a couple of weeks ago and its starting to flake again: I will investigate why: link to failure for safe-keeping: https://github.com/ovn-org/ovn-kubernetes/actions/runs/5408009751/jobs/9826941679?pr=3724 |
There was a problem hiding this comment.
thanks for fixing this.. could you add a test if its not too complicated? we don't want to regress on this again.. to me it seems like we might not have dualstack UTs that cover the routes being created in the update from 0 to 1 subnets... I think @dcbw had written some UTs on similar-ish bug before so you could use that as inspiration.
|
LGTM |
flavio-fernandes
force-pushed
the
ovnic-static-routes
branch
from
8df1540
to
74d8af7
Compare
@tssurya : I have added tests in |
|
+1 for the new test cases and fixing the typo in the test cases |
| return false | ||
| } | ||
| return true | ||
| }, 10).Should(gomega.BeTrue()) |
There was a problem hiding this comment.
wow it takes 10seconds? i'd expect millisecond order but this is fine i guess..
There was a problem hiding this comment.
Ha! It is a "copy and paste" value from line 1575 above.
There was a problem hiding this comment.
i am not a fan of such long timeouts but its fine, won't hold the PR
| } | ||
| clusterRouter, err := libovsdbops.GetLogicalRouter(libovsdbOvnNBClient, &r) | ||
| if err != nil { | ||
| return fmt.Errorf("could not find cluster router %s in the nb db for default network : err - %w", r.Name, err) |
There was a problem hiding this comment.
that's weird.. why not use
gomega.Expect(err).NotTo(gomega.HaveOccurred()) here?
There was a problem hiding this comment.
indeed! I copied that logic from the non-test code and failed to adapt this error checking. good catch!
| } | ||
| sr, err := libovsdbops.FindLogicalRouterStaticRoutesWithPredicate(libovsdbOvnNBClient, newPredicate) | ||
| gomega.Expect(err).NotTo(gomega.HaveOccurred()) | ||
| gomega.Expect(sr).Should(gomega.HaveLen(1)) |
There was a problem hiding this comment.
hmm why only 1? why don't we find the v4 route as well since its a dualstack test?
There was a problem hiding this comment.
that is bc the loop walks each one of the static route from the logical router and looks it up by uuid. It should be getting 1 at each iteration.
When node from a remote zone is updated, we only perform the actual update when necessary. This commit improved the logic for doing the remote update in cases where the subnets of the remote node change. That is particularly needed when node changes from ipv4 to dual stack (ipv4 + ipv6) Reported-at: https://issues.redhat.com/browse/SDN-3993 Signed-off-by: Flavio Fernandes <flaviof@redhat.com>
flavio-fernandes
force-pushed
the
ovnic-static-routes
branch
from
74d8af7
to
980abd3
Compare
| return false | ||
| } | ||
| return true | ||
| }, 10).Should(gomega.BeTrue()) |
There was a problem hiding this comment.
Ha! It is a "copy and paste" value from line 1575 above.
| } | ||
| clusterRouter, err := libovsdbops.GetLogicalRouter(libovsdbOvnNBClient, &r) | ||
| if err != nil { | ||
| return fmt.Errorf("could not find cluster router %s in the nb db for default network : err - %w", r.Name, err) |
There was a problem hiding this comment.
indeed! I copied that logic from the non-test code and failed to adapt this error checking. good catch!
| } | ||
| sr, err := libovsdbops.FindLogicalRouterStaticRoutesWithPredicate(libovsdbOvnNBClient, newPredicate) | ||
| gomega.Expect(err).NotTo(gomega.HaveOccurred()) | ||
| gomega.Expect(sr).Should(gomega.HaveLen(1)) |
There was a problem hiding this comment.
that is bc the loop walks each one of the static route from the logical router and looks it up by uuid. It should be getting 1 at each iteration.
|
2 known UT failures in https://github.com/ovn-org/ovn-kubernetes/actions/runs/5433226854/jobs/9880779775?pr=3724 : will retest... |
|
¯_(ツ)_/¯ |
|
this is getting a bit annoying now.. :/ its happening on all PRs unfortunately: |
|
again unrelated failures: See #3739 |
| return false | ||
| } | ||
| return true | ||
| }, 10).Should(gomega.BeTrue()) |
There was a problem hiding this comment.
i am not a fan of such long timeouts but its fine, won't hold the PR
|
LGTM |
| syncZoneIC = syncZoneIC || h.oc.isLocalZoneNode(oldNode) | ||
| // Check if the node moved from local zone to remote zone and if so syncZoneIC should be set to true. | ||
| // Also check if node subnet changed, so static routes are properly set | ||
| syncZoneIC = syncZoneIC || h.oc.isLocalZoneNode(oldNode) || nodeSubnetChanged(oldNode, newNode) |
There was a problem hiding this comment.
Update: this is great, but not enough! It does not take into consideration cases where the node annotation set via zone_cluster_controller see node.Annotations[ovnTransitSwitchPortAddr] changed.
See: #3770
When node from a remote zone is updated, we only perform the actual update when necessary. This commit improved the logic for doing the remote update in cases where the subnets of the remote node change. That is particularly needed when node changes from ipv4 to dual stack (ipv4 + ipv6)
Reported-at: https://issues.redhat.com/browse/SDN-3993