From 56c539bff030af3f722d756c3010015446d8544f Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Wed, 11 May 2022 17:44:32 +0200 Subject: [PATCH 1/5] e2e: node: deviceplug: deepcopy the pod dev template Let's avoid unexpected side effects Signed-off-by: Francesco Romani --- test/e2e_node/device_plugin_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e_node/device_plugin_test.go b/test/e2e_node/device_plugin_test.go index fd700af1b00..9a4e3382f93 100644 --- a/test/e2e_node/device_plugin_test.go +++ b/test/e2e_node/device_plugin_test.go @@ -125,7 +125,7 @@ func testDevicePlugin(f *framework.Framework, pluginSockDir string) { dp.Spec.Containers[0].Env[i].Value = pluginSockDir } } - dptemplate = dp + dptemplate = dp.DeepCopy() devicePluginPod = f.PodClient().CreateSync(dp) ginkgo.By("Waiting for devices to become available on the local node") From 23147ff4b37df4f49b9ac60e5ac2ad70bb927aed Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Sun, 15 May 2022 09:44:30 +0200 Subject: [PATCH 2/5] e2e: node: devplugin: tolerate node readiness flip In the AfterEach check of the e2e node device plugin tests, the tests want really bad to clean up after themselves: - delete the sample device plugin - restart again the kubelet - ensure that after the restart, no stale sample devices (provided by the sample device plugin) are reported anymore. We observed that in the AfterEach block of these e2e tests we have quite reliably a flip/flop of the kubelet readiness state, possibly related to a race with/ a slow runtime/PLEG check. What happens is that the kubelet readiness state is true, but goes false for a quick interval and then goes true again and it's pretty stable after that (observed adding more logs to the check loop). The key factor here is the function `getLocalNode` aborts the test (as in `framework.ExpectNoError`) if the node state is not ready. So any occurrence of this scenario, even if it is transient, will cause a test failure. I believe this will make the e2e test unnecessarily fragile without making it more correct. For the purpose of the test we can tolerate this kind of glitches, with kubelet flip/flopping the ready state, granted that we meet eventually the final desired condition on which the node reports ready AND reports no sample devices present - which was the condition the code was trying to check. So, we add a variant of `getLocalNode`, which just fetches the node object the e2e_node framework created, alongside to a flag reporting the node readiness. The new helper does not make implicitly the test abort if the node is not ready, just bubbles up this information. Signed-off-by: Francesco Romani --- test/e2e_node/device_plugin_test.go | 18 ++++++++++++------ test/e2e_node/util.go | 13 +++++++++++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/test/e2e_node/device_plugin_test.go b/test/e2e_node/device_plugin_test.go index 9a4e3382f93..755589c1578 100644 --- a/test/e2e_node/device_plugin_test.go +++ b/test/e2e_node/device_plugin_test.go @@ -130,14 +130,16 @@ func testDevicePlugin(f *framework.Framework, pluginSockDir string) { ginkgo.By("Waiting for devices to become available on the local node") gomega.Eventually(func() bool { - return numberOfSampleResources(getLocalNode(f)) > 0 + node, ready := getLocalTestNode(f) + return ready && numberOfSampleResources(node) > 0 }, 5*time.Minute, framework.Poll).Should(gomega.BeTrue()) framework.Logf("Successfully created device plugin pod") ginkgo.By("Waiting for the resource exported by the sample device plugin to become available on the local node") gomega.Eventually(func() bool { - node := getLocalNode(f) - return numberOfDevicesCapacity(node, resourceName) == devsLen && + node, ready := getLocalTestNode(f) + return ready && + numberOfDevicesCapacity(node, resourceName) == devsLen && numberOfDevicesAllocatable(node, resourceName) == devsLen }, 30*time.Second, framework.Poll).Should(gomega.BeTrue()) }) @@ -162,8 +164,11 @@ func testDevicePlugin(f *framework.Framework, pluginSockDir string) { ginkgo.By("Waiting for devices to become unavailable on the local node") gomega.Eventually(func() bool { - return numberOfSampleResources(getLocalNode(f)) <= 0 + node, ready := getLocalTestNode(f) + return ready && numberOfSampleResources(node) <= 0 }, 5*time.Minute, framework.Poll).Should(gomega.BeTrue()) + + ginkgo.By("devices now unavailable on the local node") }) ginkgo.It("Can schedule a pod that requires a device", func() { @@ -284,8 +289,9 @@ func testDevicePlugin(f *framework.Framework, pluginSockDir string) { ginkgo.By("Waiting for resource to become available on the local node after re-registration") gomega.Eventually(func() bool { - node := getLocalNode(f) - return numberOfDevicesCapacity(node, resourceName) == devsLen && + node, ready := getLocalTestNode(f) + return ready && + numberOfDevicesCapacity(node, resourceName) == devsLen && numberOfDevicesAllocatable(node, resourceName) == devsLen }, 30*time.Second, framework.Poll).Should(gomega.BeTrue()) diff --git a/test/e2e_node/util.go b/test/e2e_node/util.go index 25185255c94..220fc3b15d6 100644 --- a/test/e2e_node/util.go +++ b/test/e2e_node/util.go @@ -254,6 +254,19 @@ func getLocalNode(f *framework.Framework) *v1.Node { return &nodeList.Items[0] } +// getLocalTestNode fetches the node object describing the local worker node set up by the e2e_node infra, alongside with its ready state. +// getLocalTestNode is a variant of `getLocalNode` which reports but does not set any requirement about the node readiness state, letting +// the caller decide. The check is intentionally done like `getLocalNode` does. +// Note `getLocalNode` aborts (as in ginkgo.Expect) the test implicitly if the worker node is not ready. +func getLocalTestNode(f *framework.Framework) (*v1.Node, bool) { + node, err := f.ClientSet.CoreV1().Nodes().Get(context.TODO(), framework.TestContext.NodeName, metav1.GetOptions{}) + framework.ExpectNoError(err) + ready := e2enode.IsNodeReady(node) + schedulable := e2enode.IsNodeSchedulable(node) + framework.Logf("node %q ready=%v schedulable=%v", node.Name, ready, schedulable) + return node, ready && schedulable +} + // logKubeletLatencyMetrics logs KubeletLatencyMetrics computed from the Prometheus // metrics exposed on the current node and identified by the metricNames. // The Kubelet subsystem prefix is automatically prepended to these metric names. From 98eb6db7c0202d1951ae5c14e218bd30e76ab36a Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Thu, 12 May 2022 19:51:37 +0200 Subject: [PATCH 3/5] e2e: node: fix plugins directory Previously, the e2e test was overriding the plugins socket directory to "/var/lib/kubelet/plugins_registry". This seems wrong, and with that setting the e2e test was already failing, because the registration process was timing out, in turn because the kubelet was trying to call back the device plugin in the wrong place (see below for details). I can't explain why it worked before - or it if worked at all - but it really seems that `pluginapi.DevicePluginPath` is the right setting here. +++ In a nutshell, the device plugin registration process works like this: 1. The kubelet runs and creates the device plugin socket registration endpoint: KubeletSocket = DevicePluginPath + "kubelet.sock" DevicePluginPath = "/var/lib/kubelet/device-plugins/" 2. Each device plugin will listen to an ENDPOINT the kubelet will connect backk to. IOW the kubelet will act like a client to each device plugin, to perform allocation requests (and more) Each device plugin will serve from a endpoint. The endpoint name is plugin-specific, but they all must be inside a well-known directory: pluginapi.DevicePluginPath 3. The kubelet creates the device plugin pod, like any other pod 4. During the startup, each device plugin wants to register itself in the kubelet. So it sends a request through the registration endpoint. Key details: grpc.Dial(kubelet registration socket) registration request reqt := &pluginapi.RegisterRequest{ Version: pluginapi.Version, Endpoint: endpointSocket, <- socket relative to pluginapi.DevicePluginPath ResourceName: resourceName, <- resource name to be exposed } 5. While handling the registration request, kubelet dial back the device plugin on socketDir + req.Endpoint. But socketDir is hardcoded in the device manager code to pluginapi.KubeletSocket Signed-off-by: Francesco Romani --- test/e2e_node/device_plugin_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/e2e_node/device_plugin_test.go b/test/e2e_node/device_plugin_test.go index 755589c1578..629b5000b02 100644 --- a/test/e2e_node/device_plugin_test.go +++ b/test/e2e_node/device_plugin_test.go @@ -25,6 +25,7 @@ import ( v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" + kubeletdevicepluginv1beta1 "k8s.io/kubelet/pkg/apis/deviceplugin/v1beta1" e2eskipper "k8s.io/kubernetes/test/e2e/framework/skipper" e2etestfiles "k8s.io/kubernetes/test/e2e/framework/testfiles" admissionapi "k8s.io/pod-security-admission/api" @@ -64,7 +65,7 @@ var ( var _ = SIGDescribe("Device Plugin [Feature:DevicePluginProbe][NodeFeature:DevicePluginProbe][Serial]", func() { f := framework.NewDefaultFramework("device-plugin-errors") f.NamespacePodSecurityEnforceLevel = admissionapi.LevelPrivileged - testDevicePlugin(f, "/var/lib/kubelet/plugins_registry") + testDevicePlugin(f, kubeletdevicepluginv1beta1.DevicePluginPath) }) // numberOfSampleResources returns the number of resources advertised by a node. From 48b5af49e026dd0a26850a683199dbebe65e80e3 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Mon, 16 May 2022 14:13:39 +0200 Subject: [PATCH 4/5] e2e: node: reorder imports trivial cleanup Signed-off-by: Francesco Romani --- test/e2e_node/device_plugin_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/e2e_node/device_plugin_test.go b/test/e2e_node/device_plugin_test.go index 629b5000b02..18e78b76c1a 100644 --- a/test/e2e_node/device_plugin_test.go +++ b/test/e2e_node/device_plugin_test.go @@ -19,8 +19,12 @@ package e2enode import ( "context" "path/filepath" + "regexp" "time" + "github.com/onsi/ginkgo" + "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" @@ -30,8 +34,6 @@ import ( e2etestfiles "k8s.io/kubernetes/test/e2e/framework/testfiles" admissionapi "k8s.io/pod-security-admission/api" - "regexp" - "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/uuid" @@ -40,9 +42,6 @@ import ( "k8s.io/kubernetes/test/e2e/framework" e2enode "k8s.io/kubernetes/test/e2e/framework/node" e2epod "k8s.io/kubernetes/test/e2e/framework/pod" - - "github.com/onsi/ginkgo" - "github.com/onsi/gomega" ) const ( From f3e157d1680c5cd4eb662318b6b1938823c2e047 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Thu, 5 May 2022 14:00:08 +0200 Subject: [PATCH 5/5] e2e: node: re-enable the device plugin tests Signed-off-by: Francesco Romani --- test/e2e_node/device_plugin_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/e2e_node/device_plugin_test.go b/test/e2e_node/device_plugin_test.go index 18e78b76c1a..baf84202572 100644 --- a/test/e2e_node/device_plugin_test.go +++ b/test/e2e_node/device_plugin_test.go @@ -30,7 +30,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" kubeletdevicepluginv1beta1 "k8s.io/kubelet/pkg/apis/deviceplugin/v1beta1" - e2eskipper "k8s.io/kubernetes/test/e2e/framework/skipper" e2etestfiles "k8s.io/kubernetes/test/e2e/framework/testfiles" admissionapi "k8s.io/pod-security-admission/api" @@ -97,8 +96,6 @@ func testDevicePlugin(f *framework.Framework, pluginSockDir string) { var devicePluginPod, dptemplate *v1.Pod ginkgo.BeforeEach(func() { - e2eskipper.Skipf("Device Plugin tests are currently broken and being investigated") - ginkgo.By("Wait for node to be ready") gomega.Eventually(func() bool { nodes, err := e2enode.TotalReady(f.ClientSet)