add iscsi initiator name to persistentvolume_info #1235

jimmyseto · 2020-09-18T00:59:32Z

What this PR does / why we need it:
adds iscsi initiator name and portals to persistentvolume_info

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 #

jimmyseto · 2020-09-18T01:52:56Z

/assign @brancz

tariq1890 · 2020-09-18T02:53:18Z

If we end up adding more labels, we are going to increase the cardinality of this persistentvolume_info metric way too much. I am okay with adding more avenues for monitoring persistent volumes, but we might need to consider splitting up into different metrics.

Thoughts @brancz @lilic ?

brancz · 2020-09-21T08:13:14Z

I wonder if this is still equally useful without the iscsi portals concatenated in the label? This is the part that worries me the most. Realistically anything that doesn't change over the lifetime of a persistentvolume I think is ok to be part of the metric.

jimmyseto · 2020-09-21T16:01:53Z

I wonder if this is still equally useful without the iscsi portals concatenated in the label? This is the part that worries me the most. Realistically anything that doesn't change over the lifetime of a persistentvolume I think is ok to be part of the metric.

i'm fine with removing portals, and only adding initiator name. the former was more for completeness-sake in case users found it useful. i'll update the PR today.

as an aside, with 2.0.0 alpha, i notice that the daemonset, deployment, and replicaset metrics/objects no longer seem to have labels/tags reported. is this expected?

with 2.0.0-alpha...

TYPE kube_daemonset_created gauge

kube_daemonset_created 1.600462747e+09

TYPE kube_deployment_created gauge

kube_deployment_created 1.600554025e+09

TYPE kube_replicaset_created gauge

kube_replicaset_created 1.600575955e+09

with 1.9.7...

TYPE kube_daemonset_created gauge

kube_daemonset_created{namespace="openshift-node",daemonset="sync"} 1.600462719e+09

TYPE kube_deployment_created gauge

kube_deployment_created{namespace="kube-system",deployment="kube-state-metrics"} 1.600521086e+09

TYPE kube_replicaset_created gauge

kube_replicaset_created{namespace="kube-system",replicaset="kube-state-metrics-66df65d47d"} 1.600561774e+09

thanks.

jimmyseto · 2020-09-22T12:36:19Z

@brancz @tariq1890 iscsi portals removed. only adding iscsi initiator name now.

lilic · 2020-09-22T14:17:50Z

as an aside, with 2.0.0 alpha, i notice that the daemonset, deployment, and replicaset metrics/objects no longer seem to have labels/tags reported. is this expected?

Do you mind opening a separate issue for this, with showing your deployment and few resource examples so I can try to reproduce, I didn't notice this while testing, thanks!

Also if you don't mind to squash your commits, thanks!

jimmyseto · 2020-09-22T14:49:52Z

as an aside, with 2.0.0 alpha, i notice that the daemonset, deployment, and replicaset metrics/objects no longer seem to have labels/tags reported. is this expected?

Do you mind opening a separate issue for this, with showing your deployment and few resource examples so I can try to reproduce, I didn't notice this while testing, thanks!

will do.

Also if you don't mind to squash your commits, thanks!

squashed!

jimmyseto · 2020-09-24T14:00:59Z

@brancz @tariq1890 iscsi portals removed. only adding iscsi initiator name now.

let me know if there's anything else. thanks.

w.r.t. cardinality, i do suspect there will be future needs to add more labels for the other pv types.

lilic

Just one question, putting on hold so Frederic or Tariq have a look.

/lgtm
/hold

Thanks!!

@brancz @tariq1890

lilic · 2020-09-25T13:32:12Z

internal/store/persistentvolume.go

@@ -153,6 +156,7 @@ var (
 								"iscsi_target_portal",
 								"iscsi_iqn",
 								"iscsi_lun",
+								"iscsi_initiator_name",


Is this just a weird way github is displaying a diff or is it a formatting issue?

Is this just a weird way github is displaying a diff or is it a formatting issue?

i think it might just be the display. if you view the file (vs. the diff), does it look as you would expect?

jimmyseto · 2020-09-25T17:15:57Z

Just one question, putting on hold so Frederic or Tariq have a look.

/lgtm
/hold

Thanks!!

@brancz @tariq1890

thanks, @lilic . do you think we could include this with the v2.0.0 release (which introduces the other iscsi labels)?

lilic · 2020-09-28T11:29:12Z

thanks, @lilic . do you think we could include this with the v2.0.0 release (which introduces the other iscsi labels)?

Currently we are trying to get folks to test that release, not sure this affects any testing but we only try to cherry-pick patches and bug fixes for now, I would say with the v2.0.1 release but up to @brancz and @tariq1890 if they want to override this I am fine with it being included in 2.0.0.

brancz · 2020-09-28T11:59:39Z

I would be ok with this as it's a really tiny change and very low risk in my opinion, but I'm also ok with holding off, as ultimately we need to also make progress on 2.0 ...

jimmyseto · 2020-10-01T17:19:43Z

I would be ok with this as it's a really tiny change and very low risk in my opinion, but I'm also ok with holding off, as ultimately we need to also make progress on 2.0 ...

thanks, @brancz .

what do you think, @lilic ? can we cherry-pick this small change into v2.0.0? if not, what's the timetable for v2.0.1?

lilic · 2020-10-02T08:52:16Z

what do you think, @lilic ? can we cherry-pick this small change into v2.0.0? if not, what's the timetable for v2.0.1?

note its not v2.0.0 released yet, we only have an alpha prerelease :) We don't have a timetable for v2.0.1, it depends on bugs found in v2.0.0-alpha.0 prerelease. It might take some time, as we want to do some benchmark testing as well. We can include this in the v2.0.0-alpha.1 prerelease!

jimmyseto · 2020-10-02T12:04:08Z

what do you think, @lilic ? can we cherry-pick this small change into v2.0.0? if not, what's the timetable for v2.0.1?

note its not v2.0.0 released yet, we only have an alpha prerelease :) We don't have a timetable for v2.0.1, it depends on bugs found in v2.0.0-alpha.0 prerelease. It might take some time, as we want to do some benchmark testing as well. We can include this in the v2.0.0-alpha.1 prerelease!

got it. sorry. i guess i got a little ahead of myself with the releases :-)

if we can include this in the v2.0.0-alpha.1 prerelease, that would be great! first, we probably need to get this into master though. who needs else needs to review/approve to merge this PR? once in master, will you cherry-pick it for v2.0.0-alpha.1 or would i need to create another PR for that?

lilic · 2020-10-05T08:00:51Z

We are merging this into release-2.0 first, same as with fixes, and merging the rest into master afterwards. I changed the base and returning the tests. Will lgtm afterwards. Thanks :)

lilic · 2020-10-05T09:06:57Z

@jimmyseto do you mind rebasing against the release-2.0 branch, we can merge after that! Thanks :)

jimmyseto · 2020-10-05T12:29:10Z

@jimmyseto do you mind rebasing against the release-2.0 branch, we can merge after that! Thanks :)

@lilic - done. note that the failed unit-test is a result of #1242. thanks!

lilic

/lgtm

Thanks!

k8s-ci-robot · 2020-10-05T14:13:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jimmyseto, lilic

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:

~~OWNERS~~ [lilic]

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

lilic · 2020-10-05T14:20:02Z

/hold cancel

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 18, 2020

k8s-ci-robot requested review from lilic and tariq1890 September 18, 2020 00:59

k8s-ci-robot assigned brancz Sep 18, 2020

jimmyseto changed the title ~~add iscsi initiator name and portals to persistentvolume_info~~ add iscsi initiator name to persistentvolume_info Sep 22, 2020

jimmyseto force-pushed the add_iscsi_initiator_name_and_portals_to_pv branch from 28560d4 to 7164cee Compare September 22, 2020 14:33

lilic reviewed Sep 25, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 25, 2020

k8s-ci-robot assigned lilic Sep 25, 2020

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 25, 2020

lilic changed the base branch from master to release-2.0 October 5, 2020 07:59

add iscsi initiator name to persistentvolume_info

2c8fe6e

jimmyseto force-pushed the add_iscsi_initiator_name_and_portals_to_pv branch from 7164cee to 2c8fe6e Compare October 5, 2020 12:20

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2020

lilic reviewed Oct 5, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 5, 2020

k8s-ci-robot merged commit 59701e6 into kubernetes:release-2.0 Oct 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add iscsi initiator name to persistentvolume_info #1235

add iscsi initiator name to persistentvolume_info #1235

jimmyseto commented Sep 18, 2020

jimmyseto commented Sep 18, 2020

tariq1890 commented Sep 18, 2020

brancz commented Sep 21, 2020

jimmyseto commented Sep 21, 2020

jimmyseto commented Sep 22, 2020

lilic commented Sep 22, 2020

jimmyseto commented Sep 22, 2020

jimmyseto commented Sep 24, 2020

lilic left a comment

lilic Sep 25, 2020

jimmyseto Sep 25, 2020

jimmyseto commented Sep 25, 2020

lilic commented Sep 28, 2020

brancz commented Sep 28, 2020

jimmyseto commented Oct 1, 2020

lilic commented Oct 2, 2020 •

edited

Loading

jimmyseto commented Oct 2, 2020

lilic commented Oct 5, 2020

lilic commented Oct 5, 2020

jimmyseto commented Oct 5, 2020

lilic left a comment

k8s-ci-robot commented Oct 5, 2020

lilic commented Oct 5, 2020

add iscsi initiator name to persistentvolume_info #1235

add iscsi initiator name to persistentvolume_info #1235

Conversation

jimmyseto commented Sep 18, 2020

jimmyseto commented Sep 18, 2020

tariq1890 commented Sep 18, 2020

brancz commented Sep 21, 2020

jimmyseto commented Sep 21, 2020

TYPE kube_daemonset_created gauge

TYPE kube_deployment_created gauge

TYPE kube_replicaset_created gauge

TYPE kube_daemonset_created gauge

TYPE kube_deployment_created gauge

TYPE kube_replicaset_created gauge

jimmyseto commented Sep 22, 2020

lilic commented Sep 22, 2020

jimmyseto commented Sep 22, 2020

jimmyseto commented Sep 24, 2020

lilic left a comment

Choose a reason for hiding this comment

lilic Sep 25, 2020

Choose a reason for hiding this comment

jimmyseto Sep 25, 2020

Choose a reason for hiding this comment

jimmyseto commented Sep 25, 2020

lilic commented Sep 28, 2020

brancz commented Sep 28, 2020

jimmyseto commented Oct 1, 2020

lilic commented Oct 2, 2020 • edited Loading

jimmyseto commented Oct 2, 2020

lilic commented Oct 5, 2020

lilic commented Oct 5, 2020

jimmyseto commented Oct 5, 2020

lilic left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 5, 2020

lilic commented Oct 5, 2020

lilic commented Oct 2, 2020 •

edited

Loading