-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add metrics server support for provision controller #796
Conversation
/area lib |
cc @childsb What do you think? |
cc @wongma7 |
/assign @jsafrane |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, there is just one question I have about the port
@@ -156,6 +170,12 @@ const ( | |||
DefaultRetryPeriod = 2 * time.Second | |||
// DefaultTermLimit is used when option function TermLimit is omitted | |||
DefaultTermLimit = 30 * time.Second | |||
// DefaultMetricsPort is used when option function MetricsPort is omitted | |||
DefaultMetricsPort = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a port other than random we should default to? Like some convention for all provisioners. Will the port need to be exposed via the pod yaml? Because then a convention would make it easy to distribute yamls with the port already filled in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though port 0
means picking a random port in network programming, I used it to disabling metrics server here, like kubelet --cadvisor-port
flag. Sorry, comments here is a bit confusing.
Will the port need to be exposed via the pod yaml? Because then a convention would make it easy to distribute yamls with the port already filled in
It depends on provisioner implementation. Provisioner can have a non-zero metrics port and pass it to NewProvisionController()
to enable metrics server by default.
My intent is to make this feature optional for backward compatibility, each provisioner can customize based on needs, e.g. disable/enable by default, choose default port. We can have some convention for default port (e.g. 8000), but we may need to add another option to toggle feature.
What do you prefer? Do you want to enable metrics server by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm okay I agree with you actually let's disable it by default
/lgtm |
@cofyc and @wongma7
|
@sandaymin123 no, those stats are calculated by kubernetes for in-tree volumes that support the GetMetrics call. We will have to figure out a different solution for out-of-tree. |
What this PR does / why we need it:
This pr add metrics server support for provision controller.
Like kubelet's
storage_operation_xxx
metrics, it's better we can have some metrics to monitor latency of provision/delete operations and failures in provisioner.Current metrics:
Metrics server is disabled by default. Each provisioner can enable based on needs.
rbd/cephfs example: https://github.com/kubernetes-incubator/external-storage/pull/797/files
Examples: