Add `pod_swap_usage_bytes` as an expected metric in resource metric e2e test #119421

iholder101 · 2023-07-19T08:03:28Z

What type of PR is this?

/kind cleanup
/kind failing-test

What this PR does / why we need it:

Recently, swap stats were added to /stats/summary and /metrics/resource.
These changes caused the ResourceMetricsAPI e2e test to fail at periodic jobs [1], that do not expect the pod_swap_usage_bytes metric.

This PR adds expectation for pod_swap_usage_bytes metric to the test, which will now cause it to pass.

[1] https://testgrid.k8s.io/sig-node-cri-o#ci-crio-cgroupv1-node-e2e-features

Which issue(s) this PR fixes:

Fixes #119400

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2023-07-19T08:03:37Z

Hi @iholder101. Thanks for your PR.

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

iholder101 · 2023-07-19T08:04:33Z

/sig node

PiotrProkop · 2023-07-19T08:11:26Z

/ok-to-test

pacoxu · 2023-07-19T08:15:02Z

/assign

iholder101 · 2023-07-19T08:16:13Z

/test pull-kubernetes-cos-cgroupv1-containerd-node-e2e-features
/test pull-kubernetes-cos-cgroupv2-containerd-node-e2e-features

iholder101 · 2023-07-19T09:29:01Z

/test pull-kubernetes-cos-cgroupv1-containerd-node-e2e-features
/test pull-kubernetes-cos-cgroupv2-containerd-node-e2e-features

iholder101 · 2023-07-19T11:14:51Z

@pacoxu @ffromani

Tests pass and PR is ready to review. PTAL.

Changes summary:
The pod_swap_usage_bytes metric is not always reported, as it depends if swap is enabled on the node. Therefore, I've added the IgnoreMissing option to MatchKeys, this way even if pod_swap_usage_bytes is missing the test would pass.

However, I wanted to keep ensuring that all of the other metrics are always reported. Therefore, I've explicitly ensured they were reported by using And(..., haveKeys(...)).

ffromani · 2023-07-19T11:33:19Z

test/e2e_node/resource_metrics_test.go

@@ -165,3 +165,18 @@ func boundedSample(lower, upper interface{}) types.GomegaMatcher {
 		"Histogram": gstruct.Ignore(),
 	}))
 }
+
+func haveKeys(keys ...string) types.GomegaMatcher {
+	gomega.Expect(keys).ToNot(gomega.BeEmpty())


I'd use ExpectWithOffset - not sure if there's a better method mark this function as helper

Thanks, done 👍

ffromani · 2023-07-19T11:35:38Z

test/e2e_node/resource_metrics_test.go

@@ -73,7 +73,7 @@ var _ = SIGDescribe("ResourceMetricsAPI [NodeFeature:ResourceMetrics]", func() {
 			memoryCapacity := node.Status.Capacity["memory"]
 			memoryLimit := memoryCapacity.Value()

-			matchResourceMetrics := gstruct.MatchAllKeys(gstruct.Keys{
+			matchResourceMetrics := gomega.And(gstruct.MatchKeys(gstruct.IgnoreMissing, gstruct.Keys{


how hard is to detect if the swap is neabled in the node programmatically, and just use different MatchAllKeys in the two flows?

I guess we can fix the failure and iterate later

I don't know of an easy way to do so.

Even if NodeSwap feature-gate is enabled, it doesn't mean that the node actually has swap provisioned on it. We could always try to find the node that the pod runs on and try executing stuff on it to find this out, but I think it's a big enough complication.

I guess we can fix the failure and iterate later

Sounds good to me 👍 Thanks!

thinking aloud, maybe we can use swapon? Anyway, the more I think about it the more it seems wiser to fix the burning issue and improve later, the current approach seems good enough for the time being.

If you resubmit for whatever reason, please add a copy of this content #119421 (comment) as code comment right above the gomega.And

Signed-off-by: Itamar Holder <iholder@redhat.com>

Use haveKeys() matcher from previous commit to ensure required keys exist. Signed-off-by: Itamar Holder <iholder@redhat.com>

iholder101 · 2023-07-19T11:47:39Z

/test pull-kubernetes-cos-cgroupv1-containerd-node-e2e-features
/test pull-kubernetes-cos-cgroupv2-containerd-node-e2e-features

ffromani

/lgtm

k8s-ci-robot · 2023-07-19T11:49:28Z

LGTM label has been added.

Git tree hash: cfdeabac715824bb51bbbfc184c67161945e9947

iholder101 · 2023-07-19T12:16:44Z

/test pull-kubernetes-e2e-kind

ffromani · 2023-07-19T13:11:50Z

/cc @SergeyKanzhelev @mrunalp

bart0sh · 2023-07-19T17:06:23Z

/assign @SergeyKanzhelev @mrunalp

SergeyKanzhelev

/lgtm

k8s-ci-robot · 2023-07-19T17:25:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iholder101, SergeyKanzhelev

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:

~~test/e2e_node/OWNERS~~ [SergeyKanzhelev]

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

mrunalp · 2023-07-19T21:41:52Z

/milestone v1.28

k8s-triage-robot · 2023-07-20T01:38:20Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

pacoxu · 2023-07-20T03:27:45Z

https://prow.k8s.io/job-history/gs/kubernetes-jenkins/logs/ci-crio-cgroupv1-node-e2e-features
is green now.

test failures(7 today) link

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 19, 2023

iholder101 force-pushed the swap/adapt-resource-metric-test branch from 27e07f7 to 683e36a Compare July 19, 2023 08:04

k8s-ci-robot requested review from bobbypage and odinuge July 19, 2023 08:04

k8s-ci-robot added the area/test label Jul 19, 2023

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 19, 2023

k8s-ci-robot 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 Jul 19, 2023

k8s-ci-robot assigned pacoxu Jul 19, 2023

iholder101 force-pushed the swap/adapt-resource-metric-test branch from 683e36a to bbd9786 Compare July 19, 2023 09:27

iholder101 force-pushed the swap/adapt-resource-metric-test branch from bbd9786 to a4f8892 Compare July 19, 2023 09:55

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 19, 2023

ffromani reviewed Jul 19, 2023

View reviewed changes

iholder101 added 2 commits July 19, 2023 14:44

Add a haveKeys() helper function to match multiple keys

81abfca

Signed-off-by: Itamar Holder <iholder@redhat.com>

Add pod_swap_usage_bytes as an expected metric in e2e test

ee82654

Use haveKeys() matcher from previous commit to ensure required keys exist. Signed-off-by: Itamar Holder <iholder@redhat.com>

iholder101 force-pushed the swap/adapt-resource-metric-test branch from 0d74030 to ee82654 Compare July 19, 2023 11:44

ffromani reviewed Jul 19, 2023

View reviewed changes

k8s-ci-robot assigned ffromani Jul 19, 2023

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

k8s-ci-robot requested review from mrunalp and SergeyKanzhelev July 19, 2023 13:11

SergeyKanzhelev added this to Triage in SIG Node CI/Test Board Jul 19, 2023

SergeyKanzhelev added this to Triage in SIG Node PR Triage Jul 19, 2023

bart0sh moved this from Triage to Needs Approver in SIG Node PR Triage Jul 19, 2023

k8s-ci-robot assigned mrunalp and SergeyKanzhelev Jul 19, 2023

SergeyKanzhelev approved these changes Jul 19, 2023

View reviewed changes

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

k8s-ci-robot added this to the v1.28 milestone Jul 19, 2023

k8s-ci-robot merged commit fa88c0b into kubernetes:master Jul 20, 2023
14 of 15 checks passed

SIG Node CI/Test Board automation moved this from Triage to Done Jul 20, 2023

SIG Node PR Triage automation moved this from Needs Approver to Done Jul 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `pod_swap_usage_bytes` as an expected metric in resource metric e2e test #119421

Add `pod_swap_usage_bytes` as an expected metric in resource metric e2e test #119421

iholder101 commented Jul 19, 2023 •

edited

k8s-ci-robot commented Jul 19, 2023

iholder101 commented Jul 19, 2023

PiotrProkop commented Jul 19, 2023

pacoxu commented Jul 19, 2023

iholder101 commented Jul 19, 2023

iholder101 commented Jul 19, 2023

iholder101 commented Jul 19, 2023 •

edited

ffromani Jul 19, 2023

iholder101 Jul 19, 2023

ffromani Jul 19, 2023

ffromani Jul 19, 2023

iholder101 Jul 19, 2023 •

edited

iholder101 Jul 19, 2023

ffromani Jul 19, 2023 •

edited

iholder101 commented Jul 19, 2023

ffromani left a comment

k8s-ci-robot commented Jul 19, 2023

iholder101 commented Jul 19, 2023

ffromani commented Jul 19, 2023

bart0sh commented Jul 19, 2023

SergeyKanzhelev left a comment

k8s-ci-robot commented Jul 19, 2023

mrunalp commented Jul 19, 2023

k8s-triage-robot commented Jul 20, 2023

pacoxu commented Jul 20, 2023

Add pod_swap_usage_bytes as an expected metric in resource metric e2e test #119421

Add pod_swap_usage_bytes as an expected metric in resource metric e2e test #119421

Conversation

iholder101 commented Jul 19, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Jul 19, 2023

iholder101 commented Jul 19, 2023

PiotrProkop commented Jul 19, 2023

pacoxu commented Jul 19, 2023

iholder101 commented Jul 19, 2023

iholder101 commented Jul 19, 2023

iholder101 commented Jul 19, 2023 • edited

ffromani Jul 19, 2023

Choose a reason for hiding this comment

iholder101 Jul 19, 2023

Choose a reason for hiding this comment

ffromani Jul 19, 2023

Choose a reason for hiding this comment

ffromani Jul 19, 2023

Choose a reason for hiding this comment

iholder101 Jul 19, 2023 • edited

Choose a reason for hiding this comment

iholder101 Jul 19, 2023

Choose a reason for hiding this comment

ffromani Jul 19, 2023 • edited

Choose a reason for hiding this comment

iholder101 commented Jul 19, 2023

ffromani left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 19, 2023

iholder101 commented Jul 19, 2023

ffromani commented Jul 19, 2023

bart0sh commented Jul 19, 2023

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 19, 2023

mrunalp commented Jul 19, 2023

k8s-triage-robot commented Jul 20, 2023

pacoxu commented Jul 20, 2023

Add `pod_swap_usage_bytes` as an expected metric in resource metric e2e test #119421

Add `pod_swap_usage_bytes` as an expected metric in resource metric e2e test #119421

iholder101 commented Jul 19, 2023 •

edited

iholder101 commented Jul 19, 2023 •

edited

iholder101 Jul 19, 2023 •

edited

ffromani Jul 19, 2023 •

edited