Unregister prom gauges when recycling cluster watcher

[alpeb]

Fixes #11839

When in restartClusterWatcher we fail to connect to the target cluster for whatever reason, the function gets called again 10s later, and tries to register the same prometheus metrics without unregistering them first, which generates warnings.

The problem lies in NewRemoteClusterServiceWatcher, which instantiates the remote kube-api client and registers the metrics, returning a nil object if the client can't connect. cleanupWorkers at the beginning of restartClusterWatcher won't unregister those metrics because of that nil object.

This fix reorders NewRemoteClusterServiceWatcher so that an object is returned even when there's an error, so cleanup on that object can be performed.

[mateiidavid]

Looks good. So, to fix, this basically ensures that any error encountered after building the remote client will still retain a reference to a cluster watcher. e.g.

linkerd2/multicluster/cmd/service-mirror/main.go

Lines 155 to 163 in 999eff3

	err = restartClusterWatcher(ctx, link, *namespace, creds, controllerK8sAPI, *requeueLimit, *repairPeriod, metrics, *enableHeadlessSvc)
	if err != nil {
	// failed to restart cluster watcher; give a bit of slack
	// and restart the link watch to give it another try
	log.Error(err)
	time.Sleep(linkWatchRestartAfter)
	linkWatch.Stop()
	}
	case watch.Deleted:

if we reach this point, and we instantiated a client (and as a result registered the metrics) we'll clean-up the reference in the next iteration of the link watch and ensure we don't register the same metrics again.

Good way to fix it imo.

[olix0r]

It's generally not considered sound to handle a value when err != nil; so we probably ought to omit the value.

Suggested change

	return &rcsw, fmt.Errorf("cannot connect to api for target cluster %s: %w", clusterName, err)
	return nil, fmt.Errorf("cannot connect to api for target cluster %s: %w", clusterName, err)

However, your description seems to indicate that this is load-bearing:

This fix reorders NewRemoteClusterServiceWatcher so that an object is returned even when there's an error, so cleanup on that object can be performed.

But the return value is not used at the call-site when an error is returned:

linkerd2/multicluster/cmd/service-mirror/main.go

Lines 306 to 319 in cf2999d

	clusterWatcher, err = servicemirror.NewRemoteClusterServiceWatcher(
	ctx,
	namespace,
	controllerK8sAPI,
	cfg,
	&link,
	requeueLimit,
	repairPeriod,
	ch,
	enableHeadlessSvc,
	)
	if err != nil {
	return fmt.Errorf("unable to create cluster watcher: %w", err)
	}

So, how does this change fix the problem exactly? How do we avoid introducing another problem like this. Can you add a comment so we don't easily run into this problem again?

[olix0r]

OH! this is setting a global value. I don't think this is a sound pattern. Instead, the caller should use:

 	 cw, err = servicemirror.NewRemoteClusterServiceWatcher( 
	 	ctx, 
	 	namespace, 
	 	controllerK8sAPI, 
	 	cfg, 
	 	&link, 
	 	requeueLimit, 
	 	repairPeriod, 
	 	ch, 
	 	enableHeadlessSvc, 
	 ) 
	 if err != nil { 
	 	return fmt.Errorf("unable to create cluster watcher: %w", err) 
	 }
     clusterWatcher = cw

This removes the need for the change to NewRemoteClusterServiceWatcher.

[alpeb]

If we don't modify NewRemoteClusterServiceWatcher to return rcsw on an error, the caller won't be able to perform the gauges cleanup. Actually I've just thought of something else; we should be able to perform the cleanup directly inside NewRemoteClusterServiceWatcher before returning the error. I've just pushed that, LMKWYT.

[olix0r]

I think this makes sense. Let's add a comment above this line explaining that the remoteAPI registers gauges and they must be explicitly unregistered on error. https://github.com/linkerd/linkerd2/pull/11875/files#diff-58391f2b0ac5849326792fbaf12a8e4aa8b06886acbe9fda308357d131ed38dcR172