Updated Sig-windows Memory Limits tests to not assume all nodes are the same #107477

NikhilSharmaWe · 2022-01-11T19:39:00Z

What type of PR is this?

Fixing Bug

/kind bug

What this PR does / why we need it:

Updated Sig-windows Memory Limits tests to not assume all nodes are the same.

Which issue(s) this PR fixes:

Fixes #106608

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-01-11T19:39:08Z

@NikhilSharmaWe: 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.

NikhilSharmaWe · 2022-01-13T15:09:21Z

test/e2e/windows/memory_limits.go

+			},
+		}
+		f.PodClient().Create(&pod)
+	}


@jsturtevant Is this is right method for adding image name, node selector in Pod spec. If yes, Could you please tell about how to find the correct memory to be assigned to each pod.

Yes this is how you would create a pod. You will want to capture the result and to verify the pod started:

kubernetes/test/e2e/windows/dns.go

Lines 61 to 62 in 7dd03c8

testPod, err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Create(context.TODO(), testPod, metav1.CreateOptions{})

framework.ExpectNoError(err)

Add memory limit like this:

kubernetes/test/e2e/windows/cpu_limits.go

Lines 127 to 132 in 7dd03c8

Resources: v1.ResourceRequirements{

Limits: v1.ResourceList{

v1.ResourceMemory: memLimitQuantity,

v1.ResourceCPU: cpuLimitQuantity,

},

},

you can get that by querying the node for allocatable memory see an example in the getTotalAllocatableMemory function

@jsturtevant

I think we have to add only v1.ResourceMemory: status.Allocatable[v1.ResourceMemory] in Limits: v1.ResourceList.

Am I thinking correct here, do we also have to add value for v1.ResourceCPU ?

yes we only need memory in this case

@jsturtevant in case of pod we can add v1.ResourceMemory: status.Allocatable[v1.ResourceMemory] for each node.status, but what should be added as memory for failurePod.

it can be anything > than what is left over in the total Allocatable in the cluster for windows, since we have used it all up. something like 500MB or 1GB should be good.

NikhilSharmaWe · 2022-01-14T05:19:02Z

test/e2e/windows/memory_limits.go

+				},
+			},
+		}
+		pod, err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Create(context.TODO(), &pod, metav1.CreateOptions{})


@jsturtevant I think &pod should be correct argument for this case and &failurePod for the next case but it is showing an error. It is saying that pointer type should not be used, but in this example pointer to v1.Pod type is used in the argument.

What is the cause of the error here ?

@jsturtevant Could you please give your thoughts on the above comment ?

Suggested change

pod, err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Create(context.TODO(), &pod, metav1.CreateOptions{})

pod, err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Create(context.TODO(), pod, metav1.CreateOptions{})

This with the change https://github.com/kubernetes/kubernetes/pull/107477/files#r798791328 should work

NikhilSharmaWe · 2022-02-04T20:29:25Z

test/e2e/windows/memory_limits.go

-	ginkgo.By(fmt.Sprintf("Deploying %d pods with mem limit %v, then one additional pod", allocatablePods, memPerPod))
-
-	// these should all work
-	pods := newMemLimitTestPods(allocatablePods, imageutils.GetPauseImageName(), podType, strconv.FormatInt(memPerPod, 10))


@jsturtevant Yes true,

Could you please help me with this error

test/e2e/windows/memory_limits.go:132:3: ineffectual assignment to pod (ineffassign).

from https://github.com/gordonklaus/ineffassign

An assignment is ineffectual if the variable assigned is not thereafter used.

you are assigning pod but it's not used again. you can use _ instead as an example.

NikhilSharmaWe · 2022-02-08T16:14:29Z

@jsturtevant I think PR is ready to merge, please inform if it need further improvements.

jsturtevant · 2022-03-10T21:38:04Z

/cc

jsturtevant · 2022-03-10T21:38:33Z

/assign

jsturtevant · 2022-03-10T22:04:15Z

test/e2e/windows/memory_limits.go

+		framework.ExpectNoError(err)
+	}
+	failurePod := &v1.Pod{
+		Spec: v1.PodSpec{


Both this and the other pod spec are missing the pod name and container name (see the original pod spec definition for reference)

I would suggest this one be called something like "mem-failure-pod" and the other pods be the "mem-test-"

jsturtevant · 2022-03-11T00:02:27Z

test/e2e/windows/memory_limits.go

-			},
+	for _, node := range nodeList.Items {
+		status := node.Status
+		pod := &v1.Pod{
 			Spec: v1.PodSpec{


this pod spec should also include a node: node.Name field to make it deploy to that node

…he same

jsturtevant · 2022-03-11T17:00:34Z

Thanks for sticking with this one!

The tests passed on a cluster with different size VMs:

k get nodes -l "kubernetes.io/os=windows" -o json | jq '.items[].status.allocatable.memory'

"8285748Ki"
"8285748Ki"
"16674356Ki"
"16674356Ki"

Mar 11 08:52:31.809: INFO: Found FailedScheduling event with message 0/5 nodes are available: 1 node(s) had taint {node-role.kubernetes.io/master: }, that the pod didn't tolerate, 4 Insufficient memory. preemption: 0/5 nodes are available: 1 No victims found on node capz-conf-4pkcs for preemptor pod mem-failure-pod, 1 No victims found on node capz-conf-6q4qt for preemptor pod mem-failure-pod, 1 No victims found on node capz-conf-cmqhw for preemptor pod mem-failure-pod, 1 No victims found on node capz-conf-l5nsx for preemptor pod mem-failure-pod, 1 Preemption is not helpful for scheduling.
[AfterEach] [sig-windows] [Feature:Windows] Memory Limits [Serial] [Slow]
  test/e2e/framework/framework.go:186
Mar 11 08:52:31.809: INFO: Waiting up to 3m0s for all (but 0) nodes to be ready
STEP: Destroying namespace "memory-limit-test-windows-8971" for this suite.
•{"msg":"PASSED [sig-windows] [Feature:Windows] Memory Limits [Serial] [Slow] attempt to deploy past allocatable memory limits should fail deployments of pods once there isn't enough memory","total":1,"completed":1,"skipped":4182,"failed":0}

jsturtevant · 2022-03-11T17:01:20Z

/lgtm
/approve

k8s-ci-robot · 2022-03-11T17:01:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsturtevant, NikhilSharmaWe

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/windows/OWNERS~~ [jsturtevant]

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

jsturtevant · 2022-03-11T17:40:24Z

/test pull-kubernetes-node-e2e-containerd

jsturtevant · 2022-03-11T18:25:53Z

unrelated flakes
/retest

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jan 11, 2022

k8s-ci-robot requested review from dineshgovindasamy and jayunit100 January 11, 2022 19:39

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/windows Categorizes an issue or PR as relevant to SIG Windows. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 11, 2022

NikhilSharmaWe mentioned this pull request Jan 11, 2022

Updated Sig-windows Memory Limits tests to not assume all nodes are t… #106985

Closed

NikhilSharmaWe force-pushed the memlim branch from 9c27fd7 to 7927057 Compare January 13, 2022 15:03

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 13, 2022

NikhilSharmaWe commented Jan 13, 2022

View reviewed changes

NikhilSharmaWe force-pushed the memlim branch 8 times, most recently from 2050e35 to e60d904 Compare January 14, 2022 04:58

NikhilSharmaWe commented Jan 14, 2022

View reviewed changes

NikhilSharmaWe requested a review from jsturtevant January 14, 2022 17:50

marosset added this to In Progress (v1.24) in SIG-Windows Jan 27, 2022

jsturtevant reviewed Feb 4, 2022

View reviewed changes

NikhilSharmaWe force-pushed the memlim branch from 32e2051 to 003864a Compare February 5, 2022 04:05

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 5, 2022

NikhilSharmaWe force-pushed the memlim branch from 003864a to c34fe0d Compare February 5, 2022 04:32

NikhilSharmaWe requested a review from jsturtevant February 7, 2022 09:49

marosset moved this from In Progress (v1.24) to In Review (v1.24) in SIG-Windows Feb 24, 2022

k8s-ci-robot assigned jsturtevant Mar 10, 2022

jsturtevant suggested changes Mar 10, 2022

View reviewed changes

jsturtevant reviewed Mar 11, 2022

View reviewed changes

NikhilSharmaWe changed the title ~~Updated Sig-windows Memory Limits tests to not assume all nodes are t…~~ Updated Sig-windows Memory Limits tests to not assume all nodes are the same Mar 11, 2022

NikhilSharmaWe force-pushed the memlim branch 2 times, most recently from 1f92873 to 54584fe Compare March 11, 2022 04:41

NikhilSharmaWe requested a review from jsturtevant March 11, 2022 05:03

NikhilSharmaWe force-pushed the memlim branch from 54584fe to f7f0dba Compare March 11, 2022 05:13

Updated Sig-windows Memory Limits tests to not assume all nodes are t…

cf48674

…he same

NikhilSharmaWe force-pushed the memlim branch from f7f0dba to cf48674 Compare March 11, 2022 05:18

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2022

k8s-ci-robot merged commit 86ad8fc into kubernetes:master Mar 11, 2022

SIG-Windows automation moved this from In Review (v1.24) to Done (v1.24) Mar 11, 2022

k8s-ci-robot added this to the v1.24 milestone Mar 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated Sig-windows Memory Limits tests to not assume all nodes are the same #107477

Updated Sig-windows Memory Limits tests to not assume all nodes are the same #107477

NikhilSharmaWe commented Jan 11, 2022 •

edited

k8s-ci-robot commented Jan 11, 2022

NikhilSharmaWe Jan 13, 2022

jsturtevant Jan 13, 2022

jsturtevant Jan 13, 2022

NikhilSharmaWe Jan 13, 2022 •

edited

jsturtevant Jan 13, 2022

NikhilSharmaWe Jan 13, 2022

jsturtevant Jan 13, 2022 •

edited

NikhilSharmaWe Jan 14, 2022 •

edited

NikhilSharmaWe Jan 20, 2022

jsturtevant Feb 3, 2022

jsturtevant Feb 3, 2022

This comment was marked as resolved.

NikhilSharmaWe Feb 4, 2022

jsturtevant Feb 4, 2022

NikhilSharmaWe commented Feb 8, 2022 •

edited

jsturtevant commented Mar 10, 2022

jsturtevant commented Mar 10, 2022

jsturtevant Mar 10, 2022

jsturtevant Mar 11, 2022

jsturtevant commented Mar 11, 2022

jsturtevant commented Mar 11, 2022

k8s-ci-robot commented Mar 11, 2022

jsturtevant commented Mar 11, 2022

jsturtevant commented Mar 11, 2022

	testPod, err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Create(context.TODO(), testPod, metav1.CreateOptions{})
	framework.ExpectNoError(err)

	Resources: v1.ResourceRequirements{
	Limits: v1.ResourceList{
	v1.ResourceMemory: memLimitQuantity,
	v1.ResourceCPU: cpuLimitQuantity,
	},
	},

	pod, err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Create(context.TODO(), &pod, metav1.CreateOptions{})
	pod, err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Create(context.TODO(), pod, metav1.CreateOptions{})

Updated Sig-windows Memory Limits tests to not assume all nodes are the same #107477

Updated Sig-windows Memory Limits tests to not assume all nodes are the same #107477

Conversation

NikhilSharmaWe commented Jan 11, 2022 • 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?

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

k8s-ci-robot commented Jan 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikhilSharmaWe Jan 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsturtevant Jan 13, 2022 • edited

Choose a reason for hiding this comment

NikhilSharmaWe Jan 14, 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

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikhilSharmaWe commented Feb 8, 2022 • edited

jsturtevant commented Mar 10, 2022

jsturtevant commented Mar 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsturtevant commented Mar 11, 2022

jsturtevant commented Mar 11, 2022

k8s-ci-robot commented Mar 11, 2022

jsturtevant commented Mar 11, 2022

jsturtevant commented Mar 11, 2022

NikhilSharmaWe commented Jan 11, 2022 •

edited

NikhilSharmaWe Jan 13, 2022 •

edited

jsturtevant Jan 13, 2022 •

edited

NikhilSharmaWe Jan 14, 2022 •

edited

NikhilSharmaWe commented Feb 8, 2022 •

edited