From a6e8f7530a01694a6907c89e8c66efdb79d35d8e Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Wed, 3 Nov 2021 12:53:59 +0100 Subject: [PATCH 1/4] e2e: node: podresources: add internal helpers the intent is to make the code more readable, no intended changes in behaviour. Now it should be a bit more explicit why the code is checking some values. Signed-off-by: Francesco Romani --- test/e2e_node/podresources_test.go | 36 ++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/test/e2e_node/podresources_test.go b/test/e2e_node/podresources_test.go index ab2e7a6704b..3cf97bd9736 100644 --- a/test/e2e_node/podresources_test.go +++ b/test/e2e_node/podresources_test.go @@ -54,6 +54,28 @@ type podDesc struct { cpuRequest int // cpuRequest is in millicores } +func (desc podDesc) CpuRequestQty() resource.Quantity { + qty := resource.NewMilliQuantity(int64(desc.cpuRequest), resource.DecimalSI) + return *qty +} + +func (desc podDesc) CpuRequestExclusive() int { + if (desc.cpuRequest % 1000) != 0 { + // exclusive cpus are request only if the quantity is integral; + // hence, explicitely rule out non-integral requests + return 0 + } + return desc.cpuRequest / 1000 +} + +func (desc podDesc) RequiresCPU() bool { + return desc.cpuRequest > 0 +} + +func (desc podDesc) RequiresDevices() bool { + return desc.resourceName != "" && desc.resourceAmount > 0 +} + func makePodResourcesTestPod(desc podDesc) *v1.Pod { cnt := v1.Container{ Name: desc.cntName, @@ -64,15 +86,15 @@ func makePodResourcesTestPod(desc podDesc) *v1.Pod { }, Command: []string{"sh", "-c", "sleep 1d"}, } - if desc.cpuRequest > 0 { - cpuRequestQty := resource.NewMilliQuantity(int64(desc.cpuRequest), resource.DecimalSI) - cnt.Resources.Requests[v1.ResourceCPU] = *cpuRequestQty - cnt.Resources.Limits[v1.ResourceCPU] = *cpuRequestQty + if desc.RequiresCPU() { + cpuRequestQty := desc.CpuRequestQty() + cnt.Resources.Requests[v1.ResourceCPU] = cpuRequestQty + cnt.Resources.Limits[v1.ResourceCPU] = cpuRequestQty // we don't really care, we only need to be in guaranteed QoS cnt.Resources.Requests[v1.ResourceMemory] = resource.MustParse("100Mi") cnt.Resources.Limits[v1.ResourceMemory] = resource.MustParse("100Mi") } - if desc.resourceName != "" && desc.resourceAmount > 0 { + if desc.RequiresDevices() { cnt.Resources.Requests[v1.ResourceName(desc.resourceName)] = resource.MustParse(fmt.Sprintf("%d", desc.resourceAmount)) cnt.Resources.Limits[v1.ResourceName(desc.resourceName)] = resource.MustParse(fmt.Sprintf("%d", desc.resourceAmount)) } @@ -185,7 +207,7 @@ func matchPodDescWithResources(expected []podDesc, found podResMap) error { if !ok { return fmt.Errorf("no container resources for pod %q container %q", podReq.podName, podReq.cntName) } - if podReq.cpuRequest > 0 { + if podReq.RequiresCPU() { if isIntegral(podReq.cpuRequest) && len(cntInfo.CpuIds) != int(podReq.cpuRequest) { return fmt.Errorf("pod %q container %q expected %d cpus got %v", podReq.podName, podReq.cntName, podReq.cpuRequest, cntInfo.CpuIds) } @@ -193,7 +215,7 @@ func matchPodDescWithResources(expected []podDesc, found podResMap) error { return fmt.Errorf("pod %q container %q requested %d expected to be allocated CPUs from shared pool %v", podReq.podName, podReq.cntName, podReq.cpuRequest, cntInfo.CpuIds) } } - if podReq.resourceName != "" && podReq.resourceAmount > 0 { + if podReq.RequiresDevices() { dev := findContainerDeviceByName(cntInfo.GetDevices(), podReq.resourceName) if dev == nil { return fmt.Errorf("pod %q container %q expected data for resource %q not found", podReq.podName, podReq.cntName, podReq.resourceName) From 4b46c3a0d29db53e3f5292bd1821b02fb7f88d40 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Wed, 3 Nov 2021 13:03:36 +0100 Subject: [PATCH 2/4] e2e: node: podresources: fix exclusive cpus check Since commit 42dd01aa3f3 the cpuRequest is in millicores, hence we need to properly check translating to exclusive cpus when verifying the resource allocation. Signed-off-by: Francesco Romani --- test/e2e_node/podresources_test.go | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/test/e2e_node/podresources_test.go b/test/e2e_node/podresources_test.go index 3cf97bd9736..7586c765936 100644 --- a/test/e2e_node/podresources_test.go +++ b/test/e2e_node/podresources_test.go @@ -62,7 +62,7 @@ func (desc podDesc) CpuRequestQty() resource.Quantity { func (desc podDesc) CpuRequestExclusive() int { if (desc.cpuRequest % 1000) != 0 { // exclusive cpus are request only if the quantity is integral; - // hence, explicitely rule out non-integral requests + // hence, explicitly rule out non-integral requests return 0 } return desc.cpuRequest / 1000 @@ -208,11 +208,11 @@ func matchPodDescWithResources(expected []podDesc, found podResMap) error { return fmt.Errorf("no container resources for pod %q container %q", podReq.podName, podReq.cntName) } if podReq.RequiresCPU() { - if isIntegral(podReq.cpuRequest) && len(cntInfo.CpuIds) != int(podReq.cpuRequest) { - return fmt.Errorf("pod %q container %q expected %d cpus got %v", podReq.podName, podReq.cntName, podReq.cpuRequest, cntInfo.CpuIds) - } - if !isIntegral(podReq.cpuRequest) && len(cntInfo.CpuIds) != 0 { - return fmt.Errorf("pod %q container %q requested %d expected to be allocated CPUs from shared pool %v", podReq.podName, podReq.cntName, podReq.cpuRequest, cntInfo.CpuIds) + if exclusiveCpus := podReq.CpuRequestExclusive(); exclusiveCpus != len(cntInfo.CpuIds) { + if exclusiveCpus == 0 { + return fmt.Errorf("pod %q container %q requested %d expected to be allocated CPUs from shared pool %v", podReq.podName, podReq.cntName, podReq.cpuRequest, cntInfo.CpuIds) + } + return fmt.Errorf("pod %q container %q expected %d cpus got %v", podReq.podName, podReq.cntName, exclusiveCpus, cntInfo.CpuIds) } } if podReq.RequiresDevices() { @@ -893,7 +893,3 @@ func getKubeVirtDevicePluginPod() *v1.Pod { return p } - -func isIntegral(cpuRequest int) bool { - return (cpuRequest % 1000) == 0 -} From 14105c09fbee9063b8367cc97e085f5693b4a921 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Tue, 9 Nov 2021 11:43:55 +0100 Subject: [PATCH 3/4] e2e: node: wait for kvm plugin removal we need to make sure the system state is completely cleaned up again, to avoid to mess up with the shared node state, before we transition from one test to another. Signed-off-by: Francesco Romani --- test/e2e_node/podresources_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/test/e2e_node/podresources_test.go b/test/e2e_node/podresources_test.go index 7586c765936..d75acb18f7a 100644 --- a/test/e2e_node/podresources_test.go +++ b/test/e2e_node/podresources_test.go @@ -849,6 +849,7 @@ func teardownKubeVirtDevicePluginOrFail(f *framework.Framework, pod *v1.Pod) { err := f.ClientSet.CoreV1().Pods(pod.Namespace).Delete(context.TODO(), pod.Name, deleteOptions) framework.ExpectNoError(err) + waitForAllContainerRemoval(pod.Name, pod.Namespace) } func findKubeVirtResource(node *v1.Node) int64 { From bf9bab5bc6521967812b6322b06f84b5e12f1497 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Tue, 9 Nov 2021 16:14:35 +0100 Subject: [PATCH 4/4] e2e: podresources: wait for local node ready again Let's wait for the local node (aka the kubelet) to be ready before to query podresources again, to avoid false negatives. Co-authored-by: Artyom Lukianov Signed-off-by: Francesco Romani --- test/e2e_node/podresources_test.go | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/test/e2e_node/podresources_test.go b/test/e2e_node/podresources_test.go index d75acb18f7a..9a7188905ef 100644 --- a/test/e2e_node/podresources_test.go +++ b/test/e2e_node/podresources_test.go @@ -796,9 +796,24 @@ var _ = SIGDescribe("POD Resources [Serial] [Feature:PodResources][NodeFeature:P expectPodResources(1, cli, []podDesc{desc}) + restartTime := time.Now() ginkgo.By("Restarting Kubelet") restartKubelet(true) - framework.WaitForAllNodesSchedulable(f.ClientSet, framework.TestContext.NodeSchedulableTimeout) + + // we need to wait for the node to be reported ready before we can safely query + // the podresources endpoint again. Otherwise we will have false negatives. + ginkgo.By("Wait for node to be ready") + gomega.Eventually(func() bool { + node, err := f.ClientSet.CoreV1().Nodes().Get(context.TODO(), framework.TestContext.NodeName, metav1.GetOptions{}) + framework.ExpectNoError(err) + for _, cond := range node.Status.Conditions { + if cond.Type == v1.NodeReady && cond.Status == v1.ConditionTrue && cond.LastHeartbeatTime.After(restartTime) { + return true + } + } + return false + }, 5*time.Minute, framework.Poll).Should(gomega.BeTrue()) + expectPodResources(1, cli, []podDesc{desc}) tpd.deletePodsForTest(f) })