datapath/orchestrator: Make sure calls to Reinitialize() don't deadlock

commit b22b49d ("datapath: Fix error handling in orchestrator
reinitialize") changed the logic inside reinitialize such that errChan
only receives a value if err != nil, so successful calls to Reinitialize
will block forever.

Simplify the logic around errChan a bit and make sure that it always
receives a value whether or not reinitialize() succeeds or fails.

Fixes: b22b49d ("datapath: Fix error handling in orchestrator reinitialize")
Signed-off-by: Jordan Rife <jrife@google.com>

Author: jrife

pkg/datapath/orchestrator/orchestrator.go

@@ -214,7 +214,7 @@ func (o *orchestrator) reconciler(ctx context.Context, health cell.Health) error
 	}
 
 	var (
-		request   reinitializeRequest
+		request   = reinitializeRequest{ctx: ctx}
 		retryChan <-chan time.Time
 	)
 	for {
@@ -247,7 +247,8 @@ func (o *orchestrator) reconciler(ctx context.Context, health cell.Health) error
 			// Reinitializing is expensive, only do so if the configuration has changed.
 			prevConfig := o.latestLocalNodeConfig.Load()
 			if prevConfig == nil || !prevConfig.DeepEqual(&localNodeConfig) {
-				if err := o.reinitialize(ctx, request, &localNodeConfig); err != nil {
+				err = o.reinitialize(request.ctx, &localNodeConfig)
+				if err != nil {
 					o.params.Log.Warn("Failed to initialize datapath, retrying later",
 						logfields.Error, err,
 						logfields.RetryDelay, reinitRetryDuration,
@@ -258,15 +259,14 @@ func (o *orchestrator) reconciler(ctx context.Context, health cell.Health) error
 					retryChan = nil
 					health.OK("OK")
 				}
-			} else {
-				// We don't need to reinitialize, but we still need to unblock the requestor if there is one.
-				if request.errChan != nil {
-					close(request.errChan)
-				}
 			}
 		}
 
-		request = reinitializeRequest{}
+		if request.errChan != nil {
+			request.errChan <- err
+			close(request.errChan)
+		}
+		request = reinitializeRequest{ctx: ctx}
 
 		select {
 		case <-ctx.Done():
@@ -315,6 +315,9 @@ func (o *orchestrator) DatapathInitialized() <-chan struct{} {
 	return o.dpInitialized
 }
 
+// Reinitialize makes one attempt to reinitialize the datapath. If that attempt
+// is usuccessful, it returns the error that occurred but the orchestrator will
+// continue to attempt datapath (re)initiailization until it is successful.
 func (o *orchestrator) Reinitialize(ctx context.Context) error {
 	errChan := make(chan error)
 	o.trigger <- reinitializeRequest{
@@ -324,11 +327,7 @@ func (o *orchestrator) Reinitialize(ctx context.Context) error {
 	return <-errChan
 }
 
-func (o *orchestrator) reinitialize(ctx context.Context, req reinitializeRequest, localNodeConfig *config.Config) error {
-	if req.ctx != nil {
-		ctx = req.ctx
-	}
-
+func (o *orchestrator) reinitialize(ctx context.Context, localNodeConfig *config.Config) error {
 	err := o.params.Loader.Reinitialize(
 		ctx,
 		localNodeConfig,
@@ -341,13 +340,6 @@ func (o *orchestrator) reinitialize(ctx context.Context, req reinitializeRequest
 		err = o.params.ConnectorConfig.Reinitialize()
 	}
 	if err != nil {
-		if req.errChan != nil {
-			select {
-			case req.errChan <- err:
-			default:
-			}
-			close(req.errChan)
-		}
 		return err
 	}