Potential deadlock in knative.dev/pkg/metrics when using k8s.io/client-go/kubernetes

[anniefu]

When using knative.dev/metrics/pkg, the kubeclient has an unexpected dependency on the knative.dev/pkg/metrics pkg and its internal lock (metricsMux) that prevents the kubeclient from being used freely within the metrics pkg.

Here are the two uses of the metricsMux that don't play well.

knative.dev/pkg/metrics/exporter.go

• exporter.go protects its internal state, the current metrics config and current metrics exporter, with a Mutex (metricsMux) https://github.com/knative/pkg/blob/master/metrics/exporter.go#L28
• Whenever exporter.go updates the exporter, it needs to obtain the lock to read internal state
• Updating the exporter also requires using the kubeclient to read a K8s Secret specified in the metrics config https://github.com/knative/pkg/blob/master/metrics/stackdriver_exporter.go#L177
• Potentially other kubeclient things could be done in the future

k8s.io/client-go/

• The k8s.io/client-go/tools/metrics pkg provides a hook for users to record the latency of the kubernetes rest clients. https://github.com/kubernetes/client-go/blob/master/tools/metrics/metrics.go#L40-43
• knative.dev/metrics/pkg sets this hook
  https://github.com/knative/pkg/blob/master/metrics/metrics.go#L115-126
• On every kubeclient request, the hook calls metrics.Record() https://github.com/knative/pkg/blob/master/metrics/record.go#L41
• The call to metrics.Record() needs to obtains the knative.dev/pkg/metrics pkg's internal lock to read the metrics config

The effect of this is every call to kubeclient uses the metrics pkg to Record a metric, and needs to lock metricsMux. Every update to the metrics exporter needs to call the kubeclient, and needs to lock metricsMux.

This prevents fully locking the metrics exporter update section to prevent race conditions.

Ideally we find a way to make the kubeclient usable anywhere with the metrics package, or we make it very clear to developers that they cannot use it within a lock through a test that will fail or dev-oriented error.

Expected Behavior

You can use the client-go kubeclient while locked with the metricsMux.

Actual Behavior

Deadlock

Steps to Reproduce the Problem

Within the knative.dev/pkg/metrics pkg

1. Add a call to metricsMux.Lock()
2. Within the lock scope, attempt to use the client-go kubeclient
3. Deadlock

import (
  corev1 "k8s.io/api/core/v1"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
  "k8s.io/client-go/kubernetes"
  "k8s.io/client-go/rest"
}

func someFunc() {
  metricsMux.Lock()
  defer metricsMux.Unlock()
  config, _ := rest.InClusterConfig()
  kubeclient, _ := kubernetes.NewForConfig(config)

  kubeclient.CoreV1().Secrets("").Get("some-secret", metav1.GetOptions{})
  // Never reach here
}

## Additional Info

[github-actions]

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

[vagababov]

/assign @jjzeng-seattle @evankanderson

Is this still an issue with the latest refactoring?

[skonto]

@vagababov this is still an issue I verified by slightly modifying the code in the description and extending stats_reporter_test:

func TestDummy(t *testing.T){
	_, _ = NewStatsReporter("testreconciler")
	metrics.GetMetricsMux().Lock()
	defer metrics.GetMetricsMux().Unlock()
	config, _ := clientcmd.BuildConfigFromFlags("", "/home/user/.kube/config")
	kubeclient, _ := kubernetes.NewForConfig(config)
	_, _ = kubeclient.CoreV1().Secrets("").Get(context.TODO(), "some-secret", metav1.GetOptions{})
	// Never reach here
	fmt.Println("HELLO")
}

.GetMetricsMux() just provides access to the lock. This test never finishes.

[vagababov]

Well we should fix it :)

[skonto]

I will take a look.

[skonto]

I think I can fix this by adding a channel with several commands (GetConfig, UpdateConfig etc) without changing any semantics or adding any overhead. All calls are blocking on that global mutex and reason is we need to update all meters with the new exporter at once. I like the worker pattern here which I am using for this fix (as a side note all meters are instances of the worker type). Will create a PR.