Fix SDN get and watch resource workflow

[pravisankar]

Trello card: https://trello.com/c/hB3SyOLw
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1275904

[pravisankar]

Tested on multi-node dev cluster

[danwinship]

So I guess the overall idea is that NewListWatchFromClient() does what watchAndGetResource() was trying to do, except without the bug?

The patches seem good, though I want to look through some parts of it again (and other people looking at them would still be great).

[dcbw]

Looks like a good cleanup to me, with the exception of the 5 second wait for service watching. That I wish we could make more reliable.

[danwinship]

with the removal of the startVersion argument this can just be return registry.getServices(namespace), but then also, you could just move the code from getServices() into this function and have GetServices() call GetServicesForNamespace(kapi.NamespaceAll)

[danwinship]

assuming it passes tests/extended/networking.sh, and fixes 1275904, then LGTM

[pravisankar]

Rebased and patched on top of master branch. Added some more changes to ditch ugly 5 second wait for service watching. Ready for review/merge.
@openshift/networking PTAL

[pravisankar]

Tested and passed extended networking tests (test/extended/networking.sh)

[dcbw]

Good cleanups...

Should we log something in the Watch* functions when the eventQueue fails, as long as the failure isn't "I'm done"? Otherwise transient errors could quietly kill the event queues and we'll be none-the-wiser from the logs. Also, I assume that's how we know when to terminate, when eventQueue.Pop() returns some "done" error?

Next outside the registry, how does subnets.go::watchSubnets() or watchNodes() know when to break out of the for() loop now that 'oc.stop' is gone?

One behavioral change (though I'm not sure if it matters?) is that now on startup, the "get all the things" calls will no longer block the main goroutine but instead are now done from a goroutine. That might cause race issues, since I think most of the stuff is currently done synchronously on startup. Not sure though?

[danwinship]

One behavioral change (though I'm not sure if it matters?) is that now on startup, the "get all the things" calls will no longer block the main goroutine but instead are now done from a goroutine. That might cause race issues, since I think most of the stuff is currently done synchronously on startup. Not sure though?

I think this will cause problems with endpoint filtering: the first call to OnEndpointsUpdate() might happen before WatchPods() has completely filled in registry.namespaceOfPodIP, causing warnings and incorrect filtering. It will eventually recover (since it gets asked to filter the entire list of endpoints every time any service changes), but it might cause problems at startup.

[danwinship]

Other than that though, LGTM

[pravisankar]

On Wed, Apr 6, 2016 at 1:56 PM, Dan Williams [email protected]
wrote:

Good cleanups...

Should we log something in the Watch* functions when the eventQueue fails,
as long as the failure isn't "I'm done"? Otherwise transient errors could
quietly kill the event queues and we'll be none-the-wiser from the logs.
Also, I assume that's how we know when to terminate, when eventQueue.Pop()
returns some "done" error?

eventQueue.Pop() is a blocking call and will wait if there are no events in
the queue but yes, it could fail due to various transient errors. Logging
the error will definitely help us during investigating an issue. We could
as well log the error and restart the event queue to overcome any transient
failures.

Next outside the registry, how does subnets.go::watchSubnets() or
watchNodes() know when to break out of the for() loop now that 'oc.stop' is
gone?

oc.Stop() was never called before and currently there is no good way to
call this method. These goroutines will be terminated/killed when the
openshift service is stopped (current behavior).

One behavioral change (though I'm not sure if it matters?) is that now on
startup, the "get all the things" calls will no longer block the main
goroutine but instead are now done from a goroutine. That might cause race
issues, since I think most of the stuff is currently done synchronously on
startup. Not sure though?

Yes, some of the synchronous stuff is done async now. We need to
synchronize if async goroutines depend on each other. populateVNIDMap() is
added to address dependency between watchNetNamespaces and watchServices.
As Winship pointed out in the next comment, I missed on populating
namespaceOfPodIP
that will be needed by proxy. I will fix this issue.

More challenging issue is synchronizing sdn master/node, I have seen 2 bugs
(https://bugzilla.redhat.com/show_bug.cgi?id=1323279 and
https://bugzilla.redhat.com/show_bug.cgi?id=1322130) where the node is yet
to receive NetNamespace event but the service or pod-setup checks for vnid
and fails. This can happen when the master is overloaded or too many
projects are created in a short period.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#241 (comment)

[pravisankar]

Updated, fixed endpoints filtering during startup.
Prefer to tackle other suggestions in a separate PR. Mainly to make watch routines resilient to transient errors and more logging. Winship did some logging work in #286
@openshift/networking PTAL

[danwinship]

LGTM now I think

[pravisankar]

Rebased and resolved merge conflicts.
@openshift/networking please review/merge