Repository: gardener/machine-controller-manager (go)

Issue:
PDB violation blocks drain during forced machine deletion
### Context

The intended original behaviour of drain during hibernation and cluster deletion is that all the pods in the node will be evicted in parallel and PDBs (if any) would be violated. The `ForceDeletePods` flag is set by gardenet/worker-pool extension during hibernation/ cluster deletion to inform the drain logic to [take this path](https://github.com/gardener/machine-controller-manager/blob/2e4c6cf44d4e47e94cadefed47cb9ddcf00dc68f/pkg/controller/drain.go#L423). The `attemptEvict flag` still is [being passed](https://github.com/gardener/machine-controller-manager/blob/master/pkg/controller/drain.go#L369) based on [whether apiserver supports the eviction API](https://github.com/gardener/machine-controller-manager/blob/master/pkg/controller/drain.go#L360). This [flag is reset](https://github.com/gardener/machine-controller-manager/blob/2e4c6cf44d4e47e94cadefed47cb9ddcf00dc68f/pkg/controller/drain.go#L823) after `MaxEvictRetries` to finally force a deletion. This was intended as a way to be nice to the pods even in this scenario.

### Problem
This was fine when the `MaxEvictRetries` value was [small earlier](https://github.com/gardener/machine-controller-manager/blob/6b7e1f350ae4bb5a5a40f70af424b586e0fb2892/pkg/controller/drain.go#L89). But after we [increased it to match the drain timeout](https://github.com/gardener/machine-controller-manager/blob/master/pkg/controller/drain.go#L116), the logic gets no chance to  do the force deletion until the drain timeout happens.
 
So, it is a bug that got introduced by the change to the `MaxEvictRetries` value recently (which was required because of PDB violations during regular machine deletion).

### Proposed Solution
This can be fixed by one of the following approaches if `ForceDeletePods` option is enabled.
1. Having a smaller `MaxEvictRetries` value
2.  always passing the attemptEvict as a false

I think option 2 would be simpler.

Pull request:
Skip eviction during drain if ForceDeletePods.
**What this PR does / why we need it**:

**Which issue(s) this PR fixes**:
Fixes #415 

**Special notes for your reviewer**:
No eviction is attempted if `ForceDeletePods` is enabled. Direct pod deletion is done instead. The unit tests have been adjusted for the new behaviour. The rest of the logic remains the same.

**Release note**:
<!--  Write your release note:
1. Enter your release note in the below block.
2. If no release note is required, just write "NONE" within the block.

Format of block header: <category> <target_group>
Possible values:
- category:       improvement|noteworthy
- target_group:   user|operator|developer
-->
```improvement operator
Skip eviction during drain if ForceDeletePods is enabled.
```


Patch:
diff --git a/.ci/build b/.ci/build
index 699f09f2f..f9948553d 100755
--- a/.ci/build
+++ b/.ci/build
@@ -49,7 +49,7 @@ if [[ -z "$LOCAL_BUILD" ]]; then
     -a \
     -v \
     -mod=vendor \
-    -o ${BINARY_PATH}/rel/machine-controller-manager \
+    -o "${BINARY_PATH}/rel/machine-controller-manager" \
     cmd/machine-controller-manager/controller_manager.go
 
 # If the LOCAL_BUILD environment variable is set, we simply run `go build`.
@@ -57,6 +57,6 @@ else
   GO111MODULE=on go build \
     -v \
     -mod=vendor \
-    -o ${BINARY_PATH}/machine-controller-manager \
+    -o "${BINARY_PATH}/machine-controller-manager" \
     cmd/machine-controller-manager/controller_manager.go
 fi
diff --git a/pkg/controller/drain.go b/pkg/controller/drain.go
index 492aae4f4..e29e08f8c 100644
--- a/pkg/controller/drain.go
+++ b/pkg/controller/drain.go
@@ -366,7 +366,9 @@ func (o *DrainOptions) deleteOrEvictPods(pods []api.Pod) error {
 		return o.client.CoreV1().Pods(namespace).Get(name, metav1.GetOptions{})
 	}
 
-	return o.evictPods(len(policyGroupVersion) > 0, pods, policyGroupVersion, getPodFn)
+	attemptEvict := !o.ForceDeletePods && len(policyGroupVersion) > 0
+
+	return o.evictPods(attemptEvict, pods, policyGroupVersion, getPodFn)
 }
 
 func volIsPvc(vol *corev1.Volume) bool {


Test patch:
diff --git a/pkg/controller/drain_test.go b/pkg/controller/drain_test.go
index 9c033fc98..464cd47f1 100644
--- a/pkg/controller/drain_test.go
+++ b/pkg/controller/drain_test.go
@@ -642,7 +642,7 @@ var _ = Describe("drain", func() {
 				},
 				attemptEviction:        false,
 				terminationGracePeriod: terminationGracePeriodShort,
-				force: true,
+				force:                  true,
 			},
 			nil,
 			&expectation{
@@ -668,7 +668,7 @@ var _ = Describe("drain", func() {
 				},
 				attemptEviction:        true,
 				terminationGracePeriod: terminationGracePeriodShort,
-				force: true,
+				force:                  true,
 			},
 			[]podDrainHandler{deletePod},
 			&expectation{
@@ -681,7 +681,7 @@ var _ = Describe("drain", func() {
 				timeout:          terminationGracePeriodShort,
 				drainTimeout:     false,
 				drainError:       nil,
-				nEvictions:       12,
+				nEvictions:       0,
 				minDrainDuration: 0,
 			}),
 		Entry("Successful forced drain with support for eviction of pods with and without volume when eviction fails",
@@ -695,8 +695,8 @@ var _ = Describe("drain", func() {
 				maxEvictRetries:        1,
 				attemptEviction:        true,
 				terminationGracePeriod: terminationGracePeriodShort,
-				force:      true,
-				evictError: apierrors.NewTooManyRequestsError(""),
+				force:                  true,
+				evictError:             apierrors.NewTooManyRequestsError(""),
 			},
 			nil,
 			&expectation{
@@ -707,7 +707,7 @@ var _ = Describe("drain", func() {
 					nPodsWithExclusiveAndSharedPV: 0,
 				},
 				timeout:          terminationGracePeriodMedium,
-				drainTimeout:     true,
+				drainTimeout:     false,
 				drainError:       nil,
 				nEvictions:       0,
 				minDrainDuration: 0,