Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add prometheus metrics to CSI external-provisioner using new csi-lib-utils library #388

Merged
merged 2 commits into from
Dec 24, 2019
Merged

Add prometheus metrics to CSI external-provisioner using new csi-lib-utils library #388

merged 2 commits into from
Dec 24, 2019

Conversation

saad-ali
Copy link
Member

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds metrics to the external-provisioner CSI gRPC calls:

  • csi_sidecar_operations_seconds [ALPHA] Container Storage Interface operation duration with gRPC error code status total

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add prometheus metrics to CSI external-provisioner

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 24, 2019
@saad-ali
Copy link
Member Author

/assign @msau42
/assign @verult

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 24, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saad-ali

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 24, 2019
@saad-ali saad-ali force-pushed the addMetricsUsingLib2 branch from c9af6a7 to 65ebb2a Compare December 24, 2019 01:14
@saad-ali saad-ali mentioned this pull request Dec 24, 2019
Closed
msau42
msau42 reviewed Dec 24, 2019
View reviewed changes
cmd/csi-provisioner/csi-provisioner.go
@@ -68,6 +69,9 @@ var (
leaderElectionNamespace = flag.String("leader-election-namespace", "", "Namespace where the leader election resource lives. Defaults to the pod namespace if not set.")
strictTopology = flag.Bool("strict-topology", false, "Passes only selected node topology to CreateVolume Request, unlike default behavior of passing aggregated cluster topologies that match with topology keys of the selected node.")

metricsAddress = flag.String("metrics-address", "", "The TCP network address address where the prometheus metrics endpoint will listen (example: `:8080`). The default is empty string, which means metrics endpoint is disabled.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also update README.md with these new options?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

go.mod Outdated
k8s.io/klog v1.0.0
k8s.io/kubernetes v1.14.0
sigs.k8s.io/sig-storage-lib-external-provisioner v4.0.1+incompatible
)

replace k8s.io/api => k8s.io/api v0.0.0-20190918195907-bd6ac527cfd2
replace k8s.io/api => k8s.io/api v0.17.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were these forced overrides or automatic? I'm wondering if we can actually remove most of these replace statements now that most of the repos have switched to semantic versioning.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automatic via go-get-kubernetes.sh
I'll do some clean up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we can actually remove most of these replace statements now that most of the repos have switched to semantic versioning.

We can't, because the semantic versioning for Kubernetes components is too weak: go get -u ./... for updating to the latest "minor and patch upgrades" of other dependencies would also update all Kubernetes components if we removed the replace statements. This is bad because there's no guarantee that 0.18.0 is really API compatible with 0.17.0. I think it's better to update Kubernetes via the script and use go get -u ./... for the rest.

@saad-ali saad-ali force-pushed the addMetricsUsingLib2 branch from 65ebb2a to 701a7f3 Compare December 24, 2019 01:53
@saad-ali
Copy link
Member Author

Feedback addressed. PTAL

msau42
msau42 reviewed Dec 24, 2019
View reviewed changes
README.md Outdated
@@ -61,6 +61,10 @@ Note that the external-provisioner does not scale with more replicas. Only one e

* `--worker-threads <num>`: Number of simultaneously running `ControllerCreateVolume` and `ControllerDeleteVolume` operations. Default value is `100`.

* `metrics-address`: The TCP network address address where the prometheus metrics endpoint will run (example: `:8080` which corresponds to port 8080 on local host). The default is empty string, which means metrics endpoint is disabled.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add "--" similar to the other options

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@saad-ali saad-ali force-pushed the addMetricsUsingLib2 branch from 701a7f3 to 6975c89 Compare December 24, 2019 02:09
@saad-ali
Copy link
Member Author

Feedback addressed PTAL

@saad-ali
Update dependencies
e73400e 
Change Kubernetes dependency to v1.17.0
Update external-snapshotter to head (for 1.17.0 compatability)
Change csi-lib-utils to v0.7.0
@saad-ali saad-ali force-pushed the addMetricsUsingLib2 branch from 6975c89 to e73400e Compare December 24, 2019 02:21
@saad-ali
Copy link
Member Author

Rebased to official 0.7.0 csi-lib-utils release.

@msau42
Copy link
Collaborator

msau42 commented Dec 24, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 24, 2019
@k8s-ci-robot k8s-ci-robot merged commit 86c4605 into kubernetes-csi:master Dec 24, 2019
@saad-ali saad-ali mentioned this pull request Feb 27, 2020
Closed
@saad-ali saad-ali mentioned this pull request Jun 19, 2020
Open
@verult
Copy link
Contributor

verult commented Jul 10, 2020

@mattcary @saikat-royc

kbsonlong pushed a commit to kbsonlong/external-provisioner that referenced this pull request Dec 29, 2023
@k8s-ci-robot
Merge pull request kubernetes-csi#388 from kubernetes-csi/dependabot/…
342aa53 
…go_modules/google.golang.org/grpc-1.51.0

Bump google.golang.org/grpc from 1.50.1 to 1.51.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly left review comments

@msau42 msau42 msau42 left review comments

@davidz627 davidz627 Awaiting requested review from davidz627

@lpabon lpabon Awaiting requested review from lpabon

Assignees

@verult verult

@msau42 msau42

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@saad-ali @k8s-ci-robot @msau42 @verult @pohly