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.

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

watch-list: define a job type for the perf-dashboard #2276

Merged

Conversation

p0lyn0mial
Copy link
Contributor

What type of PR is this?

a separate job type will allow to display metrics
collected during watchlist per tests like
https://perf-dash.k8s.io/#/?jobname=gce-100Nodes-master&metriccategoryname=E2E&metricname=LoadResources&PodName=kube-apiserver-e2e-big-master%2Fkube-apiserver&Resource=memory

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 25, 2023
@k8s-ci-robot k8s-ci-robot requested review from mborsz and wojtek-t May 25, 2023 10:09
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 25, 2023
@p0lyn0mial
Copy link
Contributor Author

/assign @Argh4k @mborsz

watchListDescriptions = TestDescriptions{
"E2E": {
"LoadResources": []TestDescription{{
Name: "load",
Copy link
Member

Choose a reason for hiding this comment

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

This should be watch-list I think? Based on file in artifacts ResourceUsageSummary_watch-list_2023-05-24T08:35:20Z.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, okay, thx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would also add something that would allow me to show latencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps

"LoadResponsiveness": []TestDescription{{
				Name:             "watch-list",
				OutputFilePrefix: "APIResponsiveness",
				Parser:           parsePerfData,
			}},

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or

"LoadResponsiveness_PrometheusSimple": []TestDescription{{
				Name:             "watch-list",
				OutputFilePrefix: "APIResponsivenessPrometheus_simple",
				Parser:           parsePerfData,
			}},

?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to you can run perfdash locally to play with and find all the metrics that you need. To do that modify perfdash/Makefile by replacing githubConfigDir with https://api.github.com/repos/kubernetes/test-infra/contents/config/jobs/kubernetes/sig-scalability and replace builds with some reasonable number like 2 so you don't have to wait minutes for perdash to scrape all the data. After that just run make run from perfdash directory and you should have perfdash hosted at localhost:8080

@@ -623,13 +623,24 @@ var (
},
}

watchListDescriptions = TestDescriptions{
Copy link
Member

Choose a reason for hiding this comment

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

Why do we even need to introduce this? Why can't we use the existing performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

honesty i don't understand why it doesn't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started a slack thread if you want to chime in - https://kubernetes.slack.com/archives/C09QZTRH7/p1684911329191689

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't work due to the fact that perfdash has predefined file names here: https://github.com/kubernetes/perf-tests/pull/2276/files#diff-17340b391bfccaca6d6cf3409b032676038adfa8dead9fb15f65557a99bd098eR82
and watch-list/config.yaml has test name watch-list and not load

Copy link
Member

Choose a reason for hiding this comment

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

This is... weird.

Can we then instead parametrized the map and reuse it? (i.e. make load/watch-list a parameter of function returning this map)?

Copy link
Contributor Author

@p0lyn0mial p0lyn0mial May 25, 2023

Choose a reason for hiding this comment

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

If we want to keep the new type then I don't see value in making the map reusable. IMO it would make the code less readable.

I would appreciate if the whole mechanism could be more generic. Ideally if add a new job would not require defining new types.

Copy link
Member

Choose a reason for hiding this comment

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

I would appreciate if the whole mechanism could be more generic. Ideally if add a new job would not require defining new types.

I would like that too, but I don't want to overinvest in perfdash...

@p0lyn0mial p0lyn0mial force-pushed the upstream-watch-list-job-type branch from 3b4ec43 to 7fbbde3 Compare May 25, 2023 11:02
@Argh4k
Copy link
Contributor

Argh4k commented May 26, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2023
@wojtek-t
Copy link
Member

OK, let's merge it to unblock stuff. And we can improve in a follow up.

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Argh4k, p0lyn0mial, wojtek-t

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 May 26, 2023
@p0lyn0mial
Copy link
Contributor Author

thanks, i have also opened kubernetes/test-infra#29604

@k8s-ci-robot k8s-ci-robot merged commit 5958818 into kubernetes:master May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants