Introduce pagination and filtering to improve ClusterProfiler memory usage #6511

knopt · 2021-10-04T14:55:49Z

What this PR does / why we need it:

This MR improves (significantly reduces) memory usage of virt-api when executing ClusterProfiler dump request. Large scale cluster without this feature might experience virt-api OOMs when using ClusterProfiler.

Which issue(s) this PR fixes:
Fixes #6478

Special notes for your reviewer:

General idea behind this change is to use pagination to reduce number of pods processed at one time. Additionally, label selector is introduced for users convenience and further memory reduction.

Label selector can be used to for instance filter pods based on their component type. Additionally, field selector can be added, the question remains if it's necessary atm.

Release note:

Fixed virt-api significant memory usage when using Cluster Profiler with large KubeVirt deployments. (#6478, @knopt)

kubevirt-bot · 2021-10-04T14:56:11Z

Hi @knopt. Thanks for your PR.

I'm waiting for a kubevirt 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.

knopt · 2021-10-04T15:43:18Z

/assign @davidvossel
Could you please have a look?

VirrageS · 2021-10-07T14:46:53Z

tools/cluster-profiler/cluster-profiler.go

+	req := &v1.ClusterProfilerRequest{
+		PageSize:      int64(pageSize),
+		LabelSelector: labelSelector,
+	}
+	counter := 0


Suggested change

req := &v1.ClusterProfilerRequest{

PageSize: int64(pageSize),

LabelSelector: labelSelector,

}

counter := 0

var (

req = &v1.ClusterProfilerRequest{

PageSize: int64(pageSize),

LabelSelector: labelSelector,

}

counter = 0

)

VirrageS · 2021-10-07T14:47:17Z

tools/cluster-profiler/cluster-profiler.go

+	if _, err := os.Stat(dir); err == nil {
+		oldResultsDstDir := fmt.Sprintf("%s-old-%s", dir, rand.String(4))
+		log.Printf("Moving already existing %q => %q\n", dir, oldResultsDstDir)
+		os.Rename(dir, oldResultsDstDir)


Suggested change

os.Rename(dir, oldResultsDstDir)

if err := os.Rename(dir, oldResultsDstDir); err != nil {

return err

}

VirrageS · 2021-10-07T14:51:28Z

pkg/virt-api/rest/profiler.go

+
+	for _, pod := range podList.Items {
+		if podIsReadyComponent(&pod) {
+			pods = append(pods, pod.DeepCopy())


Why do we need DeepCopy here? I think we could just simply use non-allocatable filtering and drop the DeepCopy, so before the for loop:

pods = podList.Items[:0]

VirrageS · 2021-10-07T15:01:05Z

pkg/virt-api/rest/profiler.go

 	clientutil "kubevirt.io/client-go/util"
 )

+const (
+	maxClusterProfilerResultsPageSize     = 20


20 sounds really low for the max, is there a reason for it?

See #6478. Each additional entry adds ~9Mb of MEM usage to virt-api, so restricting max to 20 gives us at maximum ~200Mb usage.

This was just my judgement of how much additional memory is reasonable for this feature. Probably it would be great to have it configurable in a config, but I think it should go in a different MR if anyone needs it.
What's more, dumping cluster profiles is not performance related, so I would prefer safety (not killing virt-api with OOMs) over speed of downloading debugging data.

kubevirt-bot · 2021-10-07T15:03:26Z

@VirrageS: changing LGTM is restricted to collaborators

In response to this:

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.

davidvossel

this is great, I just made a few comments.

The pagination logic in the cluster-profiler tool is way better than i expected. Functionally, you've found a way to reduce the data cached in virt-api at one time while still making it trivial for a user to retrieve all that data using the client tooling.

davidvossel · 2021-10-07T21:16:54Z

staging/src/kubevirt.io/client-go/api/v1/types.go

+
+// +k8s:openapi-gen=true
+type ClusterProfilerRequest struct {
+	LabelSelector string `json:"labelSelectors"`


there's an api warning because the variable name is LabelSelector and the json marker has an s on it.

also, does this need omiteempty as well?

Added omitempty

davidvossel · 2021-10-07T21:17:50Z

api/api-rule-violations-known.list

@@ -221,6 +221,7 @@ API rule violation: names_match,kubevirt.io/client-go/api/v1,CDRomTarget,ReadOnl
 API rule violation: names_match,kubevirt.io/client-go/api/v1,CPU,DedicatedCPUPlacement
 API rule violation: names_match,kubevirt.io/client-go/api/v1,CloudInitConfigDriveSource,UserDataSecretRef
 API rule violation: names_match,kubevirt.io/client-go/api/v1,CloudInitNoCloudSource,UserDataSecretRef
+API rule violation: names_match,kubevirt.io/client-go/api/v1,ClusterProfilerRequest,LabelSelector


either change the variable to LabelSelectors, or change the json marker to labelSelector. The s character mis match in types.go is what causes this. We ideally don't want to add more known violations unless we have to.

Thanks, I've changed labelSelector json tag

davidvossel · 2021-10-07T21:23:07Z

tools/cluster-profiler/cluster-profiler.go

+	flag.IntVar(&pageSize, "page-size", defaultDumpPageSize, "Page size used for fetching profile results. Works only with dump command")
+
+	// NOTE: To profile specific kubevirt component (for example virt-api) use `kubevirt.io=virt-operator` label selector.
+	flag.StringVar(&labelSelector, "l", "", "Label selector for limiting pods to fetch the profiler results from. kubectl LIST label selector format expected")


does label only work for the dump command as well?

It works only for the dump command. I've added the info to the flag description + validation

davidvossel · 2021-10-07T21:27:58Z

tools/cluster-profiler/cluster-profiler.go

+func fetchAndSaveClusterProfilerResults(c kubecli.KubevirtClient, pageSize int, labelSelector, outputDir string) error {
+	if err := prepareDir(outputDir); err != nil {
+		return err
 	}

+	req := &v1.ClusterProfilerRequest{
+		PageSize:      int64(pageSize),
+		LabelSelector: labelSelector,
+	}
+	counter := 0
+
+	for {
+		fmt.Printf("\rFetching in progress. Downloaded so far: %d ", counter)
+		result, err := c.ClusterProfiler().Dump(req)
+		if err != nil {
+			return err
+		}
+
+		if len(result.ComponentResults) == 0 {
+			break
+		}
+
+		err = writeResultsToDisk(outputDir, result)
+		if err != nil {
+			return err
+		}
+
+		counter += len(result.ComponentResults)
+		if result.Continue == "" {
+			break
+		}
+		req.Continue = result.Continue
+	}
+
+	log.Printf("\rSUCCESS: Dumped PProf %d results for KubeVirt control plane to [%s]\n", counter, outputDir)
+	return nil


ah interesting. So the user never sees the token for the pages, we just always download all the data all at once in a loop using this tool. I like that.

davidvossel

/lgtm
/approve

kubevirt-bot · 2021-10-15T21:34:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel

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

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

fgimenez · 2021-10-18T07:32:29Z

Hi @knopt thanks a lot for your contribution! We require commits to be signed, you can read more about it here https://github.com/kubevirt/kubevirt/blob/main/CONTRIBUTING.md#contributor-compliance-with-developer-certificate-of-origin-dco Let us know if you need any help :)

…usage Signed-off-by: Tomasz Knopik <tomasz.knopik@gmail.com>

knopt · 2021-10-26T08:42:56Z

I've rebased the change + adjusted commit messages

davidvossel

/lgtm

davidvossel · 2021-10-28T14:36:45Z

/lgtm

fgimenez · 2021-10-28T14:40:06Z

/ok-to-test

kubevirt-commenter-bot · 2021-10-28T20:26:58Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-commenter-bot · 2021-10-29T08:28:02Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

kubevirt-bot requested review from rmohr and rnetser October 4, 2021 14:56

kubevirt-bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 4, 2021

kubevirt-bot assigned davidvossel Oct 4, 2021

knopt force-pushed the knopt-cluster-profiler-scale branch from b6185f3 to 6774d08 Compare October 4, 2021 15:56

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Oct 4, 2021

knopt force-pushed the knopt-cluster-profiler-scale branch 3 times, most recently from 581df90 to 75665fd Compare October 6, 2021 13:01

davidvossel self-requested a review October 7, 2021 14:41

VirrageS suggested changes Oct 7, 2021

View reviewed changes

davidvossel reviewed Oct 7, 2021

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2021

knopt force-pushed the knopt-cluster-profiler-scale branch from 75665fd to 6e42b39 Compare October 14, 2021 13:08

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 14, 2021

davidvossel approved these changes Oct 15, 2021

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2021

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2021

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2021

knopt force-pushed the knopt-cluster-profiler-scale branch from 6e42b39 to 055886e Compare October 26, 2021 08:22

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed lgtm Indicates that a PR is ready to be merged. dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Oct 26, 2021

knopt force-pushed the knopt-cluster-profiler-scale branch from 055886e to d750e4b Compare October 26, 2021 08:42

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2021

Introduce pagination and filtering to improve ClusterProfiler memory …

d750e4b

…usage Signed-off-by: Tomasz Knopik <tomasz.knopik@gmail.com>

davidvossel reviewed Oct 28, 2021

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2021

kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 28, 2021

kubevirt-bot merged commit e3dc958 into kubevirt:main Oct 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce pagination and filtering to improve ClusterProfiler memory usage #6511

Introduce pagination and filtering to improve ClusterProfiler memory usage #6511

knopt commented Oct 4, 2021

kubevirt-bot commented Oct 4, 2021

knopt commented Oct 4, 2021

VirrageS Oct 7, 2021

knopt Oct 14, 2021

VirrageS Oct 7, 2021

knopt Oct 14, 2021

VirrageS Oct 7, 2021

knopt Oct 14, 2021

VirrageS Oct 7, 2021

knopt Oct 14, 2021

kubevirt-bot commented Oct 7, 2021

davidvossel left a comment

davidvossel Oct 7, 2021

knopt Oct 14, 2021

davidvossel Oct 7, 2021

knopt Oct 14, 2021

davidvossel Oct 7, 2021

knopt Oct 14, 2021

davidvossel Oct 7, 2021

davidvossel left a comment

kubevirt-bot commented Oct 15, 2021

fgimenez commented Oct 18, 2021

knopt commented Oct 26, 2021

davidvossel left a comment •

edited

davidvossel commented Oct 28, 2021

fgimenez commented Oct 28, 2021

kubevirt-commenter-bot commented Oct 28, 2021

kubevirt-commenter-bot commented Oct 29, 2021

Introduce pagination and filtering to improve ClusterProfiler memory usage #6511

Introduce pagination and filtering to improve ClusterProfiler memory usage #6511

Conversation

knopt commented Oct 4, 2021

kubevirt-bot commented Oct 4, 2021

knopt commented Oct 4, 2021

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

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

kubevirt-bot commented Oct 7, 2021

davidvossel left a comment

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

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

davidvossel left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Oct 15, 2021

fgimenez commented Oct 18, 2021

knopt commented Oct 26, 2021

davidvossel left a comment • edited

Choose a reason for hiding this comment

davidvossel commented Oct 28, 2021

fgimenez commented Oct 28, 2021

kubevirt-commenter-bot commented Oct 28, 2021

kubevirt-commenter-bot commented Oct 29, 2021

davidvossel left a comment •

edited