New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add metrics for volume scheduling operations #59529

Merged
merged 1 commit into from Oct 23, 2018

Conversation

@wackxu
Contributor

wackxu commented Feb 8, 2018

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes # #56162

Special notes for your reviewer:
/assign @msau42
Release note:

Add metrics for volume scheduling operations
@msau42

This comment has been minimized.

Member

msau42 commented Feb 8, 2018

/ok-to-test

Buckets: prometheus.ExponentialBuckets(1000, 2, 15),
},
)
VolumeBindingPredicateLatency = prometheus.NewHistogram(

This comment has been minimized.

@msau42

msau42 Feb 8, 2018

Member

@bsalamat, what do you think about having a metric on a specific predicate? This predicate does have the potential to be slow because it has to iterative through all PVs.

This comment has been minimized.

@bsalamat

bsalamat Feb 9, 2018

Contributor

Having metrics for a specific predicate is not consistent with the rest of our code base, but it's fine for now while we are finding a better solution for dynamic resource binding.

@@ -87,6 +87,30 @@ var (
Name: "total_preemption_attempts",
Help: "Total preemption attempts in the cluster till now",
})
AssumeBindVolumesLatency = prometheus.NewHistogram(

This comment has been minimized.

@msau42

msau42 Feb 8, 2018

Member

I know the function name is assume and bind, but I think it makes more sense for the metric to be named just "AssumeVolumesLatency" because the function doesn't actually do the bind operation, it just queues it.

This comment has been minimized.

@wackxu

wackxu Feb 9, 2018

Contributor

Done

@@ -350,6 +352,8 @@ func (sched *Scheduler) bindVolumesWorker() {
Status: v1.ConditionFalse,
Reason: reason,
})
metrics.BindPodVolumeLatency.Observe(metrics.SinceInMicroseconds(start))

This comment has been minimized.

@msau42

msau42 Feb 8, 2018

Member

It may also be good to add a metric to count VolumeBindingFailed. I don't expect it to happen often.

This comment has been minimized.

@wackxu

wackxu Feb 9, 2018

Contributor

@msau42 Done

@wackxu

This comment has been minimized.

Contributor

wackxu commented Feb 9, 2018

I do not know why verify import boss failure, It is so strange.

errors in package "k8s.io/kubernetes/pkg/kubectl/cmd/resource":
import k8s.io/kubernetes/pkg/scheduler/metrics did not match any allowed prefix

errors in package "k8s.io/kubernetes/pkg/kubectl/cmd/set":
import k8s.io/kubernetes/pkg/scheduler/metrics did not match any allowed prefix

errors in package "k8s.io/kubernetes/pkg/kubectl/cmd/testing":
import k8s.io/kubernetes/pkg/scheduler/metrics did not match any allowed prefix

errors in package "k8s.io/kubernetes/pkg/kubectl/cmd":
import k8s.io/kubernetes/pkg/scheduler/metrics did not match any allowed prefix

errors in package "k8s.io/kubernetes/pkg/kubectl/cmd/config":
import k8s.io/kubernetes/pkg/scheduler/metrics did not match any allowed prefix
@msau42

This comment has been minimized.

Member

msau42 commented Feb 9, 2018

@wackxu can you also add metrics to pkg/controller/volume/persistentvolume/scheduler_binder_cache.go? I want to track how many times a binding was added to the cache, and how many times it was removed from the cache. This could help detect potential memory leaks.

@msau42

This comment has been minimized.

Member

msau42 commented Feb 9, 2018

regarding your import failures, I think because the change is adding an import to the predicates pkg, it will trigger some allowed import check. You can modify the import restrictions file, like this

@wackxu

This comment has been minimized.

Contributor

wackxu commented Feb 11, 2018

@msau42 Done, PTAL

@@ -59,6 +60,8 @@ func (c *podBindingCache) DeleteBindings(pod *v1.Pod) {
podName := getPodName(pod)
delete(c.bindings, podName)
metrics.VolumeBindingDeleteFromSchedulerBinderCache.Inc()

This comment has been minimized.

@msau42

msau42 Feb 12, 2018

Member

I think it would be better to only record when it's actually removed (it could have already been removed previously)

@@ -72,6 +75,8 @@ func (c *podBindingCache) UpdateBindings(pod *v1.Pod, node string, bindings []*b
c.bindings[podName] = nodeBinding
}
nodeBinding[node] = bindings
metrics.VolumeBindingAddToSchedulerBinderCache.Inc()

This comment has been minimized.

@msau42

msau42 Feb 12, 2018

Member

And here, only record when it's actually added.

@wackxu

This comment has been minimized.

Contributor

wackxu commented Feb 26, 2018

@msau42 Done, PTAL

@@ -71,6 +75,10 @@ func (c *podBindingCache) UpdateBindings(pod *v1.Pod, node string, bindings []*b
nodeBinding = nodeBindings{}
c.bindings[podName] = nodeBinding
}
if _, ok := nodeBinding[node]; !ok {
metrics.VolumeBindingAddToSchedulerBinderCache.Inc()

This comment has been minimized.

@msau42

msau42 Feb 26, 2018

Member

I think this can just be updated in the block above, since the metrics is based on the pod name, not node.

This comment has been minimized.

@wackxu

wackxu Feb 27, 2018

Contributor

Done

@@ -1588,6 +1591,7 @@ func (c *VolumeBindingChecker) predicate(pod *v1.Pod, meta algorithm.PredicateMe
return false, failReasons, nil
}
metrics.VolumeBindingPredicateLatency.Observe(metrics.SinceInMicroseconds(start))

This comment has been minimized.

@msau42

msau42 Feb 27, 2018

Member

I think we should also record latency when predicate returns failReasons because it still has to do lots of calculations in FindPodVolumes.

VolumeBindingFailed = prometheus.NewCounter(
prometheus.CounterOpts{
Subsystem: schedulerSubsystem,
Name: "total_volume_binding_failed",

This comment has been minimized.

@msau42

msau42 Feb 27, 2018

Member

I think the convention is to have count or latency at the end of the name, so something like "volume_binding_error_count"

@@ -298,6 +299,7 @@ func (sched *Scheduler) assumeAndBindVolumes(assumed *v1.Pod, host string) error
}
return err
}
metrics.AssumeVolumesLatency.Observe(metrics.SinceInMicroseconds(start))

This comment has been minimized.

@msau42

msau42 Feb 27, 2018

Member

We should observe latency even when error

@@ -56,6 +73,8 @@ type PVCLister interface {
func Register(pvLister PVLister, pvcLister PVCLister) {
registerMetrics.Do(func() {
prometheus.MustRegister(newPVAndPVCCountCollector(pvLister, pvcLister))
prometheus.MustRegister(VolumeBindingAddToSchedulerBinderCache)

This comment has been minimized.

@msau42

msau42 Feb 27, 2018

Member

I think the registration here is not going to work. This registration is called in PV controller process, but the volume binding cache is a library used by scheduler process. So I think the library needs export a register method that the scheduler will call.

This comment has been minimized.

@msau42

msau42 Apr 3, 2018

Member

These registration calls should be removed from this file, because this is called for PV controller process, not scheduler process.

@msau42

This comment has been minimized.

Member

msau42 commented Mar 24, 2018

@wackxu thanks for working on this. I just wanted to check if you will still have time to continue on this?

@wackxu

This comment has been minimized.

Contributor

wackxu commented Mar 24, 2018

@msau42 Sorry for the delay, will update later

@msau42

This comment has been minimized.

Member

msau42 commented Mar 24, 2018

No hurry, we have plenty of time before 1.11 code freeze

@wackxu

This comment has been minimized.

Contributor

wackxu commented Mar 28, 2018

/retest

@wackxu

This comment has been minimized.

Contributor

wackxu commented Mar 28, 2018

/test pull-kubernetes-integration

@wackxu

This comment has been minimized.

Contributor

wackxu commented Mar 28, 2018

@msau42 Done, PTAL

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Mar 29, 2018

@wackxu

This comment has been minimized.

Contributor

wackxu commented Mar 29, 2018

/test pull-kubernetes-integration

1 similar comment
@wackxu

This comment has been minimized.

Contributor

wackxu commented Mar 29, 2018

/test pull-kubernetes-integration

@wackxu

This comment has been minimized.

Contributor

wackxu commented Jul 17, 2018

/test pull-kubernetes-integration

podName := getPodName(assumedPod)
glog.V(4).Infof("BindPodVolumes for pod %q", podName)
start := time.Now()
defer func() {
VolumeSchedulingStageLatency.WithLabelValues("bind").Observe(SinceInSeconds(start))

This comment has been minimized.

@bsalamat

bsalamat Jul 17, 2018

Contributor

Observe is called when the BindPodVolumes returns an error as well. Is this intended or Observe should be called only if err is nil?

This comment has been minimized.

@wackxu

wackxu Jul 19, 2018

Contributor

intended

}
// SinceInSeconds gets the time since the specified start in seconds.
func SinceInSeconds(start time.Time) float64 {

This comment has been minimized.

@msau42

msau42 Jul 19, 2018

Member

do we really need a separate function for this? the logic seems pretty simple

@msau42

This is looking really good thanks! Just one minor nit

// RegisterForScheduler is used for scheduler, because the volume binding cache is a library
// used by scheduler process.
func RegisterForScheduler() {

This comment has been minimized.

@msau42

msau42 Jul 20, 2018

Member

Can you call this "RegisterVolumeSchedulingMetrics" to be more clear?

@msau42

This comment has been minimized.

Member

msau42 commented Jul 23, 2018

/lgtm
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 23, 2018

@msau42

This comment has been minimized.

Member

msau42 commented Jul 27, 2018

/assign @bsalamat

@@ -21,6 +21,7 @@ import (
"time"
"github.com/prometheus/client_golang/prometheus"
"k8s.io/kubernetes/pkg/controller/volume/persistentvolume"

This comment has been minimized.

@bsalamat

bsalamat Jul 31, 2018

Contributor

Any chance we could separate the metrics in a different package and import only the metrics package instead of the whole persistent volume controller?

This comment has been minimized.

@msau42

msau42 Aug 14, 2018

Member

@wackxu would you be able to look into this?

This comment has been minimized.

@wackxu

wackxu Oct 12, 2018

Contributor

We have already import k8s.io/kubernetes/pkg/controller/volume/persistentvolume in the scheduler and the metric is used only for scheduler like scheduler_binder files. I think we can extract a separate dir for those files in a follow pr

This comment has been minimized.

@bsalamat

bsalamat Oct 12, 2018

Contributor

That sounds reasonable.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jul 31, 2018

/test all

Tests are more than 96 hours old. Re-running tests.

@msau42

This comment has been minimized.

Member

msau42 commented Oct 3, 2018

@wackxu a lot of refactoring has gone in 1.12, so this change needs to be rebased. Do you still have time to work on this?

@wackxu

This comment has been minimized.

Contributor

wackxu commented Oct 12, 2018

@msau42 Sorry for the delay, will update today

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 12, 2018

@bsalamat

/approve

VolumeBindingFailed = prometheus.NewCounter(
prometheus.CounterOpts{
Subsystem: VolumeSchedulerSubsystem,
Name: "binding_error_total",

This comment has been minimized.

@msau42

msau42 Oct 15, 2018

Member

Can we also have error count for assume and predicates too? (similar to stage latency)

This comment has been minimized.

@wackxu

wackxu Oct 23, 2018

Contributor

Sorry for the delay, updated

@wackxu wackxu changed the title from Add metrics to volume scheduling operations to Add metrics for volume scheduling operations Oct 23, 2018

@msau42

This comment has been minimized.

Member

msau42 commented Oct 23, 2018

/lgtm
/approve

Thanks for continuing to work on this!

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 23, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 23, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, msau42, wackxu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@msau42

This comment has been minimized.

Member

msau42 commented Oct 23, 2018

/retest

1 similar comment
@msau42

This comment has been minimized.

Member

msau42 commented Oct 23, 2018

/retest

@k8s-ci-robot k8s-ci-robot merged commit 101d26c into kubernetes:master Oct 23, 2018

18 checks passed

cla/linuxfoundation wackxu authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@wackxu wackxu deleted the wackxu:addmetricvol branch Oct 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment