[kwok] Provide generic functions that enable it to simulate data for any indicator (Node) #555

nikola-jokic · 2023-04-28T11:39:39Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #199

Does this PR introduce a user-facing change?

[kwok] Provide generic functions that enable it to simulate data for any indicator

k8s-ci-robot · 2023-04-28T11:39:47Z

Hi @nikola-jokic. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

netlify · 2023-04-28T11:39:51Z

✅ Deploy Preview for k8s-kwok canceled.

Name	Link
🔨 Latest commit	`a7fa8aa`
🔍 Latest deploy log	https://app.netlify.com/sites/k8s-kwok/deploys/648931c7fcba070008bbe5ba

nikola-jokic · 2023-04-28T11:40:54Z

This is currently just a proposed way of exposing metrics from the controller, to make sure package structure is okay.
Looking forward to your suggestions @wzshiming 😄

wzshiming · 2023-05-01T14:55:19Z

The package structure is good to me,
I would like to minimize hard-coded metrics and use configuration whenever possible 😄

wzshiming · 2023-05-04T10:20:11Z

/ok-to-test

pkg/apis/internalversion/metric_type.go

pkg/kwok/server/metrics.go

test/kwokctl/metric.yaml

nikola-jokic · 2023-05-12T09:21:42Z

pkg/kwok/controllers/node_controller.go

@@ -277,6 +284,7 @@ func (c *NodeController) watchResources(ctx context.Context, opt metav1.ListOpti
 					node := event.Object.(*corev1.Node)
 					if c.need(node) {
 						c.putNodeInfo(node)
+						c.applyMetrics(ctx, node)


Should we delete the registry when the node is deleted, or should we set values to 0?

nikola-jokic · 2023-05-12T09:25:19Z

startedContainersTotal is kind of tricky. The fastest approach is to do it on the Pod Controller. Basically, this function would evaluate to true if node.metadata.name is pod's NodeName, and then increase counter on true.
Is there a better way I'm not seeing?

Update:
My first suggestion would not really work in case we have startedContainersTotal used as a part of the expression... We can have a counter per node to hold a state and then apply it for a node. Querying the API would be really expensive

pkg/kwok/metrics/cel/evaluate.go

pkg/kwok/server/metrics.go

pkg/kwok/controllers/metrics.go

nikola-jokic · 2023-05-17T15:34:02Z

/cc @wzshiming
This is what I have so far, and it was working fine locally (manually tested) 😄
Need to add tests and hopefully find places to refactor, but I would appreciate your review! ☺️

wzshiming · 2023-06-09T17:30:41Z

pkg/kwok/metrics/histogram.go

+}
+
+// Set is used to increment value at le bucket and add val to the sum.
+func (h *Histogram) Set(le string, val float64) (err error) {


I tested it and it didn't behave as I expected, so I refactored it and wzshiming@e4f49e3 it meets expectations 😄.

Apply my patch git apply <(curl https://github.com/wzshiming/kwok/commit/e4f49e3acf3ae0915cb3898e8f9cbf326888aa93.patch)

Amazing, sorry I could not make it work right away, I did not exactly know how it should behave 😄
I tried applying patch but it produced an error
Do you mind pushing on the branch 😄

BTY, I added auto docs for API, so this PR needs to rebase then exec ./hack/update-api-docs.sh

Rebased and executed
I'll apply the patch by hand. git shows error (patch does not apply)

nikola-jokic · 2023-06-11T11:57:35Z

/retest

wzshiming · 2023-06-12T10:09:17Z

pkg/apis/v1alpha1/metric_types.go

+// MetricSpec holds spec for metrics.
+type MetricSpec struct {
+	// Path is a restful service path.
+	Path string `json:"path"`


Suggestion, if kwokctl is using this configuration, we should add this Path to the Prometheus collection target, so we can open the web of Prometheus and see the chart of the configured metrics.

WDYT?

That sounds great as long as we keep it static
If you see it become dynamic (having route like /nodes/{nodeName}) in the future, is it okay to opt-out of the prometheus collection targets?

Don't worry, Prometheus supports rules for dynamically discovered targets.

https://prometheus.io/docs/prometheus/latest/configuration/configuration/#http_sd_config

test/kwokctl/metric.yaml

wzshiming · 2023-06-12T10:17:17Z

pkg/kwok/metrics/cel/evaluate.go

+}
+
+// EvaluateMetric evaluates a cel program and returns a metric value
+func (e *Evaluator) EvaluateMetric(node *corev1.Node) (float64, error) {


Let's keep it in the same style as EvaluateString.

wzshiming · 2023-06-12T10:22:00Z

hack/tools/tools.go

 	// gen-crd-api-reference-docs
 	_ "github.com/ahmetb/gen-crd-api-reference-docs"
+	// code-generator


Revert this file

Oh sure, sorry about that

test/kwokctl/metric.yaml

wzshiming · 2023-06-12T15:09:22Z

test/kwokctl/metric.yaml

+      help: "[ALPHA] Cumulative number of containers started"
+      kind: counter
+      value: 'startedContainersTotal( node.metadata.name )'
+    - name: kubelet_pleg_relist_duration_seconds


Look nice 😄

pkg/kwok/controllers/pod_controller.go

hack/tools/tools.go

wzshiming · 2023-06-12T15:41:20Z

BTY, commits are too much, squash it 😄

pkg/kwok/controllers/controller.go

Co-authored-by: Shiming Zhang <wzshiming@foxmail.com>

wzshiming

/approve
/lgtm

Thank you for your contribution!

k8s-ci-robot · 2023-06-14T06:01:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nikola-jokic, wzshiming

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~~ [wzshiming]

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

nikola-jokic · 2023-06-14T06:12:00Z

Thank you for doing many reviews and improvements!

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Apr 28, 2023

k8s-ci-robot requested review from Huang-Wei and yibozhuang April 28, 2023 11:39

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 28, 2023

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 28, 2023

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 4, 2023

nikola-jokic force-pushed the nikola-jokic/metrics branch from 7330ed0 to 99456b1 Compare May 12, 2023 09:14

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2023

nikola-jokic commented May 12, 2023

View reviewed changes

pkg/apis/internalversion/metric_type.go Outdated Show resolved Hide resolved

nikola-jokic commented May 12, 2023

View reviewed changes

pkg/kwok/server/metrics.go Outdated Show resolved Hide resolved

nikola-jokic commented May 12, 2023

View reviewed changes

test/kwokctl/metric.yaml Show resolved Hide resolved

nikola-jokic commented May 12, 2023

View reviewed changes

pkg/kwok/metrics/cel/evaluate.go Outdated Show resolved Hide resolved

nikola-jokic changed the title ~~Nikola jokic/metrics~~ [kwok] Provide generic functions that enable it to simulate data for any indicator (Node) May 12, 2023

nikola-jokic commented May 17, 2023

View reviewed changes

pkg/kwok/server/metrics.go Outdated Show resolved Hide resolved

nikola-jokic commented May 17, 2023

View reviewed changes

pkg/kwok/controllers/metrics.go Outdated Show resolved Hide resolved

k8s-ci-robot requested a review from wzshiming May 17, 2023 15:34

wzshiming reviewed Jun 9, 2023

View reviewed changes

nikola-jokic force-pushed the nikola-jokic/metrics branch from 34e0a90 to e120819 Compare June 9, 2023 17:44

wzshiming reviewed Jun 12, 2023

View reviewed changes

test/kwokctl/metric.yaml Outdated Show resolved Hide resolved

wzshiming reviewed Jun 12, 2023

View reviewed changes

nikola-jokic commented Jun 12, 2023

View reviewed changes

test/kwokctl/metric.yaml Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 12, 2023

nikola-jokic force-pushed the nikola-jokic/metrics branch from 5ef07f7 to 077008e Compare June 12, 2023 13:17

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 12, 2023

wzshiming reviewed Jun 12, 2023

View reviewed changes

nikola-jokic force-pushed the nikola-jokic/metrics branch from 08ccda5 to bb96667 Compare June 13, 2023 15:10

wzshiming reviewed Jun 14, 2023

View reviewed changes

pkg/kwok/controllers/controller.go Outdated Show resolved Hide resolved

wzshiming force-pushed the nikola-jokic/metrics branch 2 times, most recently from 0c3021c to ac3cefd Compare June 14, 2023 03:17

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2023

Add metric

a7fa8aa

Co-authored-by: Shiming Zhang <wzshiming@foxmail.com>

wzshiming force-pushed the nikola-jokic/metrics branch from ac3cefd to a7fa8aa Compare June 14, 2023 03:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2023

wzshiming approved these changes Jun 14, 2023

View reviewed changes

k8s-ci-robot assigned wzshiming Jun 14, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2023

k8s-ci-robot merged commit e619889 into kubernetes-sigs:main Jun 14, 2023

nikola-jokic deleted the nikola-jokic/metrics branch June 14, 2023 07:00

wzshiming mentioned this pull request Jun 14, 2023

[kwok] Provide Kubelet metrics #199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kwok] Provide generic functions that enable it to simulate data for any indicator (Node) #555

[kwok] Provide generic functions that enable it to simulate data for any indicator (Node) #555

nikola-jokic commented Apr 28, 2023 •

edited by wzshiming

Loading

k8s-ci-robot commented Apr 28, 2023

netlify bot commented Apr 28, 2023 •

edited

Loading

nikola-jokic commented Apr 28, 2023

wzshiming commented May 1, 2023 •

edited

Loading

wzshiming commented May 4, 2023

nikola-jokic May 12, 2023

nikola-jokic commented May 12, 2023 •

edited

Loading

nikola-jokic commented May 17, 2023

wzshiming Jun 9, 2023 •

edited

Loading

nikola-jokic Jun 9, 2023

wzshiming Jun 9, 2023

nikola-jokic Jun 9, 2023

nikola-jokic commented Jun 11, 2023

wzshiming Jun 12, 2023

nikola-jokic Jun 12, 2023

wzshiming Jun 12, 2023 •

edited

Loading

wzshiming Jun 12, 2023 •

edited

Loading

wzshiming Jun 12, 2023

nikola-jokic Jun 12, 2023

wzshiming Jun 12, 2023

wzshiming commented Jun 12, 2023

wzshiming left a comment

k8s-ci-robot commented Jun 14, 2023

nikola-jokic commented Jun 14, 2023

[kwok] Provide generic functions that enable it to simulate data for any indicator (Node) #555

[kwok] Provide generic functions that enable it to simulate data for any indicator (Node) #555

Conversation

nikola-jokic commented Apr 28, 2023 • edited by wzshiming Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Apr 28, 2023

netlify bot commented Apr 28, 2023 • edited Loading

✅ Deploy Preview for k8s-kwok canceled.

nikola-jokic commented Apr 28, 2023

wzshiming commented May 1, 2023 • edited Loading

wzshiming commented May 4, 2023

Choose a reason for hiding this comment

nikola-jokic commented May 12, 2023 • edited Loading

nikola-jokic commented May 17, 2023

wzshiming Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikola-jokic commented Jun 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzshiming Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

wzshiming Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzshiming commented Jun 12, 2023

wzshiming left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 14, 2023

nikola-jokic commented Jun 14, 2023

nikola-jokic commented Apr 28, 2023 •

edited by wzshiming

Loading

netlify bot commented Apr 28, 2023 •

edited

Loading

wzshiming commented May 1, 2023 •

edited

Loading

nikola-jokic commented May 12, 2023 •

edited

Loading

wzshiming Jun 9, 2023 •

edited

Loading

wzshiming Jun 12, 2023 •

edited

Loading

wzshiming Jun 12, 2023 •

edited

Loading