From 7bec28438704c56e83fd5816d8b812b6758e35dd Mon Sep 17 00:00:00 2001 From: Eddie Torres Date: Wed, 17 Jan 2024 01:08:48 +0000 Subject: [PATCH] Remove preStop LCH context timeout Signed-off-by: Eddie Torres --- cmd/hooks/prestop.go | 35 ++++++++++++------------------- cmd/hooks/prestop_test.go | 43 +-------------------------------------- cmd/options.go | 7 +------ 3 files changed, 15 insertions(+), 70 deletions(-) diff --git a/cmd/hooks/prestop.go b/cmd/hooks/prestop.go index 754512527a..5e6a22f865 100644 --- a/cmd/hooks/prestop.go +++ b/cmd/hooks/prestop.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "os" - "time" v1 "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" @@ -28,33 +27,30 @@ No unnecessary delay is added to the termination workflow, as the PreStop hook l If the PreStop hook hangs during its execution, the driver node pod will be forcefully terminated after 32 seconds (30s terminationGracePeriodSeconds + 2 second grace period extension from Kubelet). */ -func PreStop(clientset kubernetes.Interface, timeout time.Duration) error { - klog.InfoS("PreStop: executing PreStop lifecycle hook", "timeout", timeout) - - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() +func PreStop(clientset kubernetes.Interface) error { + klog.InfoS("PreStop: executing PreStop lifecycle hook") nodeName := os.Getenv("CSI_NODE_NAME") if nodeName == "" { return fmt.Errorf("PreStop: CSI_NODE_NAME missing") } - node, err := fetchNode(ctx, clientset, nodeName) + node, err := fetchNode(clientset, nodeName) if err != nil { return err } if isNodeBeingDrained(node) { klog.InfoS("PreStop: node is being drained, checking for remaining VolumeAttachments", "node", nodeName) - return waitForVolumeAttachments(ctx, clientset, nodeName) + return waitForVolumeAttachments(clientset, nodeName) } klog.InfoS("PreStop: node is not being drained, skipping VolumeAttachments check", "node", nodeName) return nil } -func fetchNode(ctx context.Context, clientset kubernetes.Interface, nodeName string) (*v1.Node, error) { - node, err := clientset.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) +func fetchNode(clientset kubernetes.Interface, nodeName string) (*v1.Node, error) { + node, err := clientset.CoreV1().Nodes().Get(context.Background(), nodeName, metav1.GetOptions{}) if err != nil { return nil, fmt.Errorf("fetchNode: failed to retrieve node information: %w", err) } @@ -70,7 +66,7 @@ func isNodeBeingDrained(node *v1.Node) bool { return false } -func waitForVolumeAttachments(ctx context.Context, clientset kubernetes.Interface, nodeName string) error { +func waitForVolumeAttachments(clientset kubernetes.Interface, nodeName string) error { allAttachmentsDeleted := make(chan struct{}) factory := informers.NewSharedInformerFactory(clientset, 0) @@ -81,7 +77,7 @@ func waitForVolumeAttachments(ctx context.Context, clientset kubernetes.Interfac klog.V(5).InfoS("DeleteFunc: VolumeAttachment deleted", "node", nodeName) va := obj.(*storagev1.VolumeAttachment) if va.Spec.NodeName == nodeName { - if err := checkVolumeAttachments(ctx, clientset, nodeName, allAttachmentsDeleted); err != nil { + if err := checkVolumeAttachments(context.Background(), clientset, nodeName, allAttachmentsDeleted); err != nil { klog.ErrorS(err, "DeleteFunc: error checking VolumeAttachments") } } @@ -90,7 +86,7 @@ func waitForVolumeAttachments(ctx context.Context, clientset kubernetes.Interfac klog.V(5).InfoS("UpdateFunc: VolumeAttachment updated", "node", nodeName) va := newObj.(*storagev1.VolumeAttachment) if va.Spec.NodeName == nodeName { - if err := checkVolumeAttachments(ctx, clientset, nodeName, allAttachmentsDeleted); err != nil { + if err := checkVolumeAttachments(context.Background(), clientset, nodeName, allAttachmentsDeleted); err != nil { klog.ErrorS(err, "UpdateFunc: error checking VolumeAttachments") } } @@ -102,18 +98,13 @@ func waitForVolumeAttachments(ctx context.Context, clientset kubernetes.Interfac go informer.Run(allAttachmentsDeleted) - if err := checkVolumeAttachments(ctx, clientset, nodeName, allAttachmentsDeleted); err != nil { + if err := checkVolumeAttachments(context.Background(), clientset, nodeName, allAttachmentsDeleted); err != nil { klog.ErrorS(err, "waitForVolumeAttachments: error checking VolumeAttachments") } - select { - case <-allAttachmentsDeleted: - klog.InfoS("waitForVolumeAttachments: finished waiting for VolumeAttachments to be deleted. preStopHook completed") - return nil - - case <-ctx.Done(): - return fmt.Errorf("waitForVolumeAttachments: timed out waiting for preStopHook to complete: %w", ctx.Err()) - } + <-allAttachmentsDeleted + klog.InfoS("waitForVolumeAttachments: finished waiting for VolumeAttachments to be deleted. preStopHook completed") + return nil } func checkVolumeAttachments(ctx context.Context, clientset kubernetes.Interface, nodeName string, allAttachmentsDeleted chan struct{}) error { diff --git a/cmd/hooks/prestop_test.go b/cmd/hooks/prestop_test.go index 4364661635..f9cb6ae704 100644 --- a/cmd/hooks/prestop_test.go +++ b/cmd/hooks/prestop_test.go @@ -3,7 +3,6 @@ package hooks import ( "fmt" "testing" - "time" "github.com/golang/mock/gomock" "github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver" @@ -153,46 +152,6 @@ func TestPreStopHook(t *testing.T) { return nil }, }, - { - name: "TestPreStopHook: node is being drained, volume attachments remain -- timeout exceeded", - nodeName: "test-node", - expErr: fmt.Errorf("waitForVolumeAttachments: timed out waiting for preStopHook to complete: context deadline exceeded"), - mockFunc: func(nodeName string, mockClient *driver.MockKubernetesClient, mockCoreV1 *driver.MockCoreV1Interface, mockNode *driver.MockNodeInterface, mockVolumeAttachments *driver.MockVolumeAttachmentInterface, mockStorageV1Interface *driver.MockStorageV1Interface) error { - - fakeNode := &v1.Node{ - Spec: v1.NodeSpec{ - Taints: []v1.Taint{ - { - Key: v1.TaintNodeUnschedulable, - Effect: v1.TaintEffectNoSchedule, - }, - }, - }, - } - - fakeVolumeAttachments := &storagev1.VolumeAttachmentList{ - Items: []storagev1.VolumeAttachment{ - { - Spec: storagev1.VolumeAttachmentSpec{ - NodeName: "test-node", - }, - }, - }, - } - - mockClient.EXPECT().CoreV1().Return(mockCoreV1).AnyTimes() - mockClient.EXPECT().StorageV1().Return(mockStorageV1Interface).AnyTimes() - - mockCoreV1.EXPECT().Nodes().Return(mockNode).AnyTimes() - mockNode.EXPECT().Get(gomock.Any(), gomock.Eq(nodeName), gomock.Any()).Return(fakeNode, nil).AnyTimes() - - mockStorageV1Interface.EXPECT().VolumeAttachments().Return(mockVolumeAttachments).AnyTimes() - mockVolumeAttachments.EXPECT().List(gomock.Any(), gomock.Any()).Return(fakeVolumeAttachments, nil).AnyTimes() - mockVolumeAttachments.EXPECT().Watch(gomock.Any(), gomock.Any()).Return(watch.NewFake(), nil).AnyTimes() - - return nil - }, - }, { name: "TestPreStopHook: Node is drained before timeout", nodeName: "test-node", @@ -274,7 +233,7 @@ func TestPreStopHook(t *testing.T) { t.Setenv("CSI_NODE_NAME", tc.nodeName) } - err := PreStop(mockClient, 5*time.Second) + err := PreStop(mockClient) if tc.expErr != nil { assert.Error(t, err) diff --git a/cmd/options.go b/cmd/options.go index b8eccea19e..c10a4728ff 100644 --- a/cmd/options.go +++ b/cmd/options.go @@ -20,7 +20,6 @@ import ( "fmt" "os" "strings" - "time" flag "github.com/spf13/pflag" @@ -34,10 +33,6 @@ import ( "k8s.io/klog/v2" ) -const ( - preStopTimeout = 30 * time.Second -) - // Options is the combined set of options for all operating modes. type Options struct { DriverMode driver.Mode @@ -107,7 +102,7 @@ func GetOptions(fs *flag.FlagSet) *Options { if clientErr != nil { klog.ErrorS(err, "unable to communicate with k8s API") } else { - err = hooks.PreStop(clientset, preStopTimeout) + err = hooks.PreStop(clientset) if err != nil { klog.ErrorS(err, "failed to execute PreStop lifecycle hook") klog.FlushAndExit(klog.ExitFlushTimeout, 1)