Small timout for worker notification

[fkorotkov]

It seems at the moment if a worker re-establishes notify stream (for example, if network flips or proxy breaks the connection) then we can see "no worker registered with this name" errors.

This change makes Notifier to wait for 30 seconds before failing, at the time of calling Notifier#Notify we know such worker exists.

PS not sure if we need to make the timeout configurable.

[edigaryev]

What do you think about simply respecting the ctx here?

We already have a configurable timeout which defaults to 30 seconds and translates to a context.Context:

orchard/internal/controller/api_vms_ip.go

Lines 29 to 30 in 88fba80

	waitContext, waitContextCancel := context.WithTimeout(ctx, time.Duration(wait)*time.Second)
	defer waitContextCancel()

You'll just need to pass it to Notify() and respect it while waiting for the worker to re-connect.

[fkorotkov]

Something like 23e36df?

[edigaryev]

Yes, there are a couple of other Notify() invocations that need the proper context supplied to them, though.

[fkorotkov]

But not all of them have config for wait. Will see how to pass it around

[fkorotkov]

I actually start liking this idea less and less particurally because of this use:

orchard/internal/controller/scheduler/scheduler.go

Lines 352 to 356 in 581de32

	if err := scheduler.notifier.Notify(context.Background(), affectedWorker, &rpc.WatchInstruction{
	Action: &rpc.WatchInstruction_SyncVmsAction{},
	}); err != nil {
	scheduler.logger.Errorf("Failed to reactively sync VMs on worker %s: %v", affectedWorker, err)
	}

With just waiting for context such use in the future might end up in a infinity loop. What do you think of passing a timeout duration explicitly to Notify and respect both context and the timeout.

[fkorotkov]

Reverted back. I think there should be some default on the worker notify channel reconnection.

[edigaryev]

With just waiting for context such use in the future might end up in a infinity loop.

Passing a time-bounded context should to the trick.

But overall it's a two sided coin: with the changes currently in this PR, we'll needlessly wait 30 seconds for each network-flipped worker in the scheduler, and there could be a lot of them.

[fkorotkov]

See aa71c03 but what about these two places 4d27b84