From 74be9f04fab283b15d27ee995e021d60b93e5ee3 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Tue, 2 Jun 2020 12:40:32 -0400 Subject: [PATCH] Ensure CleanupActionHandle always completes The way gingko handles interrupts is: - It starts running AfterSuite hooks in a separate goroutine (this includes cleanupAction hooks) - Once AfterSuite hook is done executing it calls os.Exit(1) on test suite. So how cleanupFunc() that runs via defer in test can be interrupted is: - cleanupFunc starts running via defer (or AfterEach hook) but first thing that function does is to remove cleanupHandle from framework.RemoveCleanupAction. - Test suite receives interrupt from user and AfterSuite block starts executing - remember that while cleanupFunc is running in goroutine#1, AfterSuite is running concurrently in goroutine#2. - AfterSuite hook has bunch of CleanupActions it needs to run which were registered via framework.AddCleanupAction(cleanupFunc) but once cleanupFunc starts executing via defer in the test, it will remove the cleanupHandle from framework's aftersuite hooks. - So if AfterSuite did not had anything to run (because those actions were removed via framework.RemoveCleanupAction then it will simply go to the last framework.AfterEach action and call os.Exit(1) - So if os.Exit(1) is called before cleanupFunc has a chance to finish in defer, it will not complete. --- test/e2e/storage/drivers/csi.go | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/test/e2e/storage/drivers/csi.go b/test/e2e/storage/drivers/csi.go index 43a7358bafa..ebdddd0cc6e 100644 --- a/test/e2e/storage/drivers/csi.go +++ b/test/e2e/storage/drivers/csi.go @@ -210,7 +210,6 @@ func (h *hostpathCSIDriver) PrepareTest(f *framework.Framework) (*testsuites.Per // Cleanup CSI driver and namespaces. This function needs to be idempotent and can be // concurrently called from defer (or AfterEach) and AfterSuite action hooks. cleanupFunc := func() { - framework.RemoveCleanupAction(h.cleanupHandle) ginkgo.By(fmt.Sprintf("deleting the test namespace: %s", ns1)) // Delete the primary namespace but its okay to fail here because this namespace will // also be deleted by framework.Aftereach hook @@ -222,6 +221,12 @@ func (h *hostpathCSIDriver) PrepareTest(f *framework.Framework) (*testsuites.Per ginkgo.By(fmt.Sprintf("deleting the driver namespace: %s", ns2)) tryFunc(deleteNamespaceFunc(f.ClientSet, ns2, framework.DefaultNamespaceDeletionTimeout)) + // cleanup function has already ran and hence we don't need to run it again. + // We do this as very last action because in-case defer(or AfterEach) races + // with AfterSuite and test routine gets killed then this block still + // runs in AfterSuite + framework.RemoveCleanupAction(h.cleanupHandle) + } h.cleanupHandle = framework.AddCleanupAction(cleanupFunc) @@ -404,7 +409,6 @@ func (m *mockCSIDriver) PrepareTest(f *framework.Framework) (*testsuites.PerTest // Cleanup CSI driver and namespaces. This function needs to be idempotent and can be // concurrently called from defer (or AfterEach) and AfterSuite action hooks. cleanupFunc := func() { - framework.RemoveCleanupAction(m.cleanupHandle) ginkgo.By(fmt.Sprintf("deleting the test namespace: %s", ns1)) // Delete the primary namespace but its okay to fail here because this namespace will // also be deleted by framework.Aftereach hook @@ -422,6 +426,12 @@ func (m *mockCSIDriver) PrepareTest(f *framework.Framework) (*testsuites.PerTest tryFunc(cancelLogging) ginkgo.By(fmt.Sprintf("deleting the driver namespace: %s", ns2)) tryFunc(deleteNamespaceFunc(f.ClientSet, ns2, framework.DefaultNamespaceDeletionTimeout)) + // cleanup function has already ran and hence we don't need to run it again. + // We do this as very last action because in-case defer(or AfterEach) races + // with AfterSuite and test routine gets killed then this block still + // runs in AfterSuite + framework.RemoveCleanupAction(m.cleanupHandle) + } m.cleanupHandle = framework.AddCleanupAction(cleanupFunc) @@ -560,7 +570,6 @@ func (g *gcePDCSIDriver) PrepareTest(f *framework.Framework) (*testsuites.PerTes // Cleanup CSI driver and namespaces. This function needs to be idempotent and can be // concurrently called from defer (or AfterEach) and AfterSuite action hooks. cleanupFunc := func() { - framework.RemoveCleanupAction(g.cleanupHandle) ginkgo.By(fmt.Sprintf("deleting the test namespace: %s", ns1)) // Delete the primary namespace but its okay to fail here because this namespace will // also be deleted by framework.Aftereach hook @@ -572,6 +581,12 @@ func (g *gcePDCSIDriver) PrepareTest(f *framework.Framework) (*testsuites.PerTes ginkgo.By(fmt.Sprintf("deleting the driver namespace: %s", ns2)) tryFunc(deleteNamespaceFunc(f.ClientSet, ns2, framework.DefaultNamespaceDeletionTimeout)) + // cleanup function has already ran and hence we don't need to run it again. + // We do this as very last action because in-case defer(or AfterEach) races + // with AfterSuite and test routine gets killed then this block still + // runs in AfterSuite + framework.RemoveCleanupAction(g.cleanupHandle) + } g.cleanupHandle = framework.AddCleanupAction(cleanupFunc)