opened
ialidzhikov
username_0
**What happened**:
The Shoot care test scales the kube-apiserver deploy to 0 replicas and waits until this is reflected in the  appropriate Shoot condition. In a defer, the test executes clean up logic and scales back the kube-apiserver replicas to the original count. Ref https://github.com/gardener/gardener/blob/v1.3.1/test/integration/shoots/care/shoot_care.go.

We see cases in which the whole test suite times out and this may sometimes interrupt the Shoot care test during its execution. As the clean up logic is executed in `defer` (not in `AfterSuite`), the test is interrupted and the defer is not executed leaving a Shoot with 0 kube-apiserver replicas (similar Shoot subsequently will fail to delete because of this reason).

**What you expected to happen**:
gingko ensures that it will execute `AfterSuite` function when interrupt signal is received. Our clean up logic should be in `AfterSuite` block, not in `defer`.

**How to reproduce it (as minimally and precisely as possible)**:
1. Run the test.

```
go test -timeout=0 -mod=vendor ./test/suites/shoot \
      --v -ginkgo.v -ginkgo.progress -ginkgo.noColor \
      --report-file=/tmp/report.json \
      -kubecfg=$KUBECONFIG \
      -shoot-name=$SHOOT_NAME \
      -project-namespace=$PROJECT_NAMESPACE \
      -ginkgo.focus="Should observe failed health condition"
```

2. Interrupt.

```
=== RUN   TestGardenerSuite
Running Suite: Shoot Test Suite
===============================
Random Seed: 1588519529
Will run 1 of 19 specs

S
------------------------------
Shoot Care testing
  [BETA] [SERIAL] [SHOOT] Should observe failed health condition in the Shoot when scaling down the API Server of the Shoot
  /home/go/src/github.com/gardener/gardener/test/framework/gingko_utils.go:26
[BeforeEach] Shoot Care testing
  /home/go/src/github.com/gardener/gardener/test/framework/commonframework.go:59
[BeforeEach] Shoot Care testing
  /home/go/src/github.com/gardener/gardener/test/framework/shootframework.go:79
[It] [BETA] [SERIAL] [SHOOT] Should observe failed health condition in the Shoot when scaling down the API Server of the Shoot
  /home/go/src/github.com/gardener/gardener/test/framework/gingko_utils.go:26
time="2020-05-03T18:25:35+03:00" level=info msg="Waiting for shoot aws-local to have expected condition (APIServerAvailable: False). Currently: (APIServerAvailable: True)"
^C
---------------------------------------------------------
Received interrupt.  Running AfterSuite...
^C again to terminate immediately

Ran 1 of 2 Specs in 24.908 seconds
FAIL! -- 1 Passed | 0 Failed | 0 Pending | 1 Skipped
FAIL	github.com/gardener/gardener/test/suites/shoot	25.580s
```

3. Ensure that the clean up logic is not executed.

**Anything else we need to know?**:

**Environment**:

- Gardener version:
- Kubernetes version (use `kubectl version`):
- Cloud provider or hardware configuration:
- Others:
Shoot care test clean up logic is not executed on interrupt signal
issue
created
danielfoehrKn
username_1
Thanks for finding  the issue!
The reason why it is implemented with a defer function and not with ginko.AfterSuite() is that the [shoot suite](https://github.com/gardener/gardener/blob/34f3c865f5bd9fb21a9554fa7a92e4aa2f8ed6a6/test/suites/shoot/run_suite_test.go) already registers a ginko.AfterSuite() and the test machinery currently does not allow to add logic  to it. There can be only on ginko.AfterSuite().

To workaround, the Shoot care test can create its own Test suite.

I see that multiple integration tests have the need to execute cleanup code in addition to the[ common after suite](https://github.com/gardener/gardener/blob/73734a51aabb692a59abf0b5cb93b56ab596b7f1/test/framework/commonframework.go#L94) . I do not think that it is a good design if these tests then have to create their own Suite - wasn't having one Shoot Suite not one of the reasons to introduce the new integration test framework?

I think it would be nice if each integration test would be able to execute code in the ginko.AfterSuite() - independent how it is implemented (register code / no common ginko.AfterSuite() / one Suite for all integration tests that need ginko.AfterSuite()/ ...)
I already mentioned the same to @username_2 - maybe this is another reason to thunk about it again.
comment
created
schrodit
username_2
We should avoid having own testdefinitions. Therefore, making this feature available in the testsuite seems more clean to me.
comment
closed
rfranzke
username_3
issue