scheduler/internal: Improving cache and heap test coverage #114273

TommyStarK · 2022-12-03T14:49:17Z

Signed-off-by: TommyStarK thomasmilox@gmail.com

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

If applied this commit will increase the unit tests code coverage of pkg/scheduler/internal/cache and pkg/scheduler/internal/heap.

Initial unit tests code coverage

After changes

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2022-12-03T14:49:20Z

Please note that we're already in Test Freeze for the release-1.26 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.26.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Sat Dec 3 09:45:01 UTC 2022.

k8s-ci-robot · 2022-12-03T14:49:25Z

@TommyStarK: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2022-12-03T14:49:25Z

Hi @TommyStarK. 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.

TommyStarK · 2022-12-03T14:51:26Z

Hello @chendave , @kerthcet

I know guys you are super busy and this PR is far from being a priority. As a very new aspiring contributor I am still trying to become familiar with the code and to start helping on small pieces. Hope you don't mind.

Any feedback will be greatly appreciated 😅.

Best regards,

kerthcet · 2022-12-05T03:57:58Z

Thanks @TommyStarK
/ok-to-test

chendave

Nice improvement on the test coverage!

roughly, this PR looks to me!

chendave · 2022-12-05T07:02:34Z

pkg/scheduler/internal/cache/cache_test.go

+		})
+	}
+
+	// should trigger an error log as there is no node info for this given name


could this be verified? or else what's the point to leave these in the testcases?

I don't think so. You are right there is no point to keep that.

Resolved in 1c12a991112

chendave · 2022-12-05T07:16:09Z

pkg/scheduler/internal/cache/cache_test.go

@@ -1854,6 +1860,106 @@ func TestSchedulerCache_updateNodeInfoSnapshotList(t *testing.T) {
 	}
 }

+func TestSchedulerCache_ErrorCases(t *testing.T) {


Alternatively, you can test this in the related function instead of pull the false cases out and test it here, e.g "forget unknown pod" can go to "TestForgetPod". It might be better to org these testcases, but I am fine with this as well.

It was my first intent but once my code added I felt like messing up with the existing scenarii so I change my mind and gathered all false cases into this function. I'd rather keep it this way but if you don't feel comfortable with that please tell me and I'll go back with the 1st approach :)

+1, this is hard to maintain in the future. I didn't see any precedents in kubernetes. Let's try to make the tests pure.

Sure thing. I will come back to move those tests to their related function.

@chendave, @kerthcet, I tried to keep tests as pure as possible and avoid messing too much with their clarity. This is what I ended with 807b29ad6e9120cbca49ae864b74a4f8ae003f36.

Below the new unit tests code coverage for the pkg/scheduler/internal/cache.

All pipelines have passed, remaining at your disposal if you have any remark :)

Best regards,

chendave · 2022-12-05T07:41:30Z

pkg/scheduler/internal/heap/heap_test.go

@@ -97,12 +141,17 @@ func TestHeap_Add(t *testing.T) {
 // TestHeap_Delete tests Heap.Delete and ensures that heap invariant is
 // preserved after deleting items.
 func TestHeap_Delete(t *testing.T) {
-	h := New(testHeapObjectKeyFunc, compareInts)
+	h := NewWithRecorder(testHeapObjectKeyFunc, compareInts, new(testMetricRecorder)) // cover conditions on metric recorder


Do you want to add some statements to cover the metric inc/dec?

Resolved in 1c12a991112

chendave · 2022-12-07T09:54:45Z

this looks good to me now, pls remember to squash or org your commits properly.

/lgtm

/assign @Huang-Wei (per the suggestion from bot)

TommyStarK · 2022-12-12T14:07:48Z

@kerthcet I applied changes to address your latest comments. Hope it matches to your expectations :)
ps: Job pull-kubernetes-e2e-gce-100-performance failed tho. I reran it but it seems to be out of the scope of my changes

Remaining at your disposal.

Best,

alculquicondor · 2022-12-12T14:22:32Z

pkg/scheduler/internal/heap/heap.go

@@ -158,6 +164,14 @@ func (h *Heap) Update(obj interface{}) error {
 	return h.Add(obj)
 }

+// Cleanup removes all items.
+func (h *Heap) Cleanup() {


Why are you adding this function?

Sorry I misunderstood the comment of the last review. Removed.

It's called by UnschedulablePods. Just remove this function if it's not needed.

Got it. I removed it. sorry

TommyStarK · 2022-12-12T15:16:29Z

/retest-required

alculquicondor · 2022-12-12T16:15:57Z

pkg/scheduler/internal/cache/snapshot_test.go

+				if info == nil {
+					t.Error("node infos should not be nil")
+				}
+				if test.expectedNodesInfos[i] != info.String() {


/approve cancel
don't compare strings.

Compare the objects and use cmp.Diff to be able to tell the differences.

alculquicondor · 2022-12-12T16:17:52Z

/cancel-approve

alculquicondor · 2022-12-12T16:18:29Z

Once comments are addressed, I'll wait for another pass from @kerthcet

ahg-g · 2022-12-12T18:14:18Z

General question, why are we pushing back on testify? it seems more natural than cmp in some places?

TommyStarK · 2022-12-12T18:20:28Z

@alculquicondor I updated my PR to address your comments. Had issues using cmp.Diff on NodeInfo due to the underlying unexported fields. Hope it matches your expectations

alculquicondor · 2022-12-12T18:35:18Z

General question, why are we pushing back on testify?

Mostly for consistency and because errors.Is is a better practice than reflect.DeepEqual (the behavior that testify has) when checking errors.

ahg-g · 2022-12-12T18:36:36Z

General question, why are we pushing back on testify?

Mostly for consistency and because errors.Is is a better practice than reflect.DeepEqual (the behavior that testify has) when checking errors.

I am not sure if we want to be that strict, I liked how simple the code was with testify.

ahg-g · 2022-12-12T18:42:36Z

General question, why are we pushing back on testify?

Mostly for consistency and because errors.Is is a better practice than reflect.DeepEqual (the behavior that testify has) when checking errors.

I am not sure if we want to be that strict, I liked how simple the code was with testify.

As discussed offline with Aldo, assertions are not generally preferred, which I agree with. My concern with cmp is that it sometimes bloats the test code (comparing to the exact same error); but if we can use cmp with options to avoid that, then that is great.

TommyStarK · 2022-12-12T18:47:50Z

/retest-required

alculquicondor

/approve
Thanks
I will leave the lgtm to @kerthcet

alculquicondor · 2022-12-12T18:45:29Z

pkg/scheduler/internal/cache/snapshot_test.go

+			nodes: []*v1.Node{
+				{
+					ObjectMeta: metav1.ObjectMeta{Name: "node-0"},
+					Status:     v1.NodeStatus{},


this line is unnecessary, same below

alculquicondor · 2022-12-12T18:46:09Z

pkg/scheduler/internal/cache/snapshot_test.go

+						{
+							Pod: podWithPort,
+						},


nit:

Suggested change

{

Pod: podWithPort,

},

{ Pod: podWithPort },

alculquicondor · 2022-12-12T18:48:01Z

pkg/scheduler/internal/cache/snapshot_test.go

+				}
+			}
+
+			if !sets.String.Equal(test.expectedUsedPVCSet, snapshot.usedPVCSet) {


nit: cmp.Diff can make it easier to spot the differences.

k8s-ci-robot · 2022-12-12T18:50:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, alculquicondor, TommyStarK

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:

~~pkg/scheduler/OWNERS~~ [ahg-g,alculquicondor]

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

ahg-g · 2022-12-12T18:46:07Z

pkg/scheduler/internal/cache/snapshot_test.go

+				}
+			}
+
+			if !sets.String.Equal(test.expectedUsedPVCSet, snapshot.usedPVCSet) {


you can use cmp here as well

ahg-g · 2022-12-12T18:50:26Z

pkg/scheduler/internal/cache/snapshot_test.go

+
+			for i, node := range test.nodes {
+				info, err := snapshot.Get(node.Name)
+				if err != nil {


Every err != nil is now replaced with three lines, similar with info == nil below. Do we have a package to do those checks in a single line?

If this is the case please tell me and I will update the PR

Also not sure about this package. LGTM it's easy to read.

Signed-off-by: TommyStarK <thomasmilox@gmail.com>

TommyStarK · 2022-12-12T19:11:50Z

Thank you very much @alculquicondor @ahg-g for your time and review. I updated the code according your suggestions.
Best,

kerthcet · 2022-12-13T05:09:30Z

/lgtm

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Dec 3, 2022

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 3, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 3, 2022

k8s-ci-robot requested review from chendave and kerthcet December 3, 2022 14:50

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 3, 2022

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 Dec 5, 2022

chendave reviewed Dec 5, 2022

View reviewed changes

TommyStarK requested review from chendave and kerthcet and removed request for kerthcet and chendave December 6, 2022 11:52

k8s-ci-robot assigned chendave Dec 7, 2022

TommyStarK requested a review from kerthcet December 12, 2022 14:07

alculquicondor reviewed Dec 12, 2022

View reviewed changes

TommyStarK force-pushed the unit-tests/pkg-scheduler-internal branch from 59bc26f to 5db1983 Compare December 12, 2022 14:29

alculquicondor reviewed Dec 12, 2022

View reviewed changes

TommyStarK force-pushed the unit-tests/pkg-scheduler-internal branch from 5db1983 to a1a4b2b Compare December 12, 2022 18:18

alculquicondor reviewed Dec 12, 2022

View reviewed changes

ahg-g reviewed Dec 12, 2022

View reviewed changes

scheduler/internal: Improving cache and heap test coverage

94a29ef

Signed-off-by: TommyStarK <thomasmilox@gmail.com>

TommyStarK force-pushed the unit-tests/pkg-scheduler-internal branch from a1a4b2b to 94a29ef Compare December 12, 2022 19:09

k8s-ci-robot assigned kerthcet Dec 13, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 13, 2022

k8s-ci-robot merged commit 6228b91 into kubernetes:master Dec 13, 2022

k8s-ci-robot added this to the v1.26 milestone Dec 13, 2022

TommyStarK deleted the unit-tests/pkg-scheduler-internal branch December 13, 2022 10:04

liggitt modified the milestones: v1.26, v1.27 Dec 13, 2022

scheduler/internal: Improving cache and heap test coverage #114273

scheduler/internal: Improving cache and heap test coverage #114273

Conversation

TommyStarK commented Dec 3, 2022

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?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Dec 3, 2022

k8s-ci-robot commented Dec 3, 2022

k8s-ci-robot commented Dec 3, 2022

TommyStarK commented Dec 3, 2022

kerthcet commented Dec 5, 2022

chendave 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

TommyStarK Dec 5, 2022 • edited

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

chendave commented Dec 7, 2022

TommyStarK commented Dec 12, 2022

Choose a reason for hiding this comment

TommyStarK Dec 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TommyStarK commented Dec 12, 2022

Choose a reason for hiding this comment

alculquicondor commented Dec 12, 2022

alculquicondor commented Dec 12, 2022

ahg-g commented Dec 12, 2022

TommyStarK commented Dec 12, 2022

alculquicondor commented Dec 12, 2022

ahg-g commented Dec 12, 2022

ahg-g commented Dec 12, 2022 • edited

TommyStarK commented Dec 12, 2022

alculquicondor 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

k8s-ci-robot commented Dec 12, 2022

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

TommyStarK commented Dec 12, 2022

kerthcet commented Dec 13, 2022

TommyStarK Dec 5, 2022 •

edited

TommyStarK Dec 12, 2022 •

edited

ahg-g commented Dec 12, 2022 •

edited