Repository: knative/pkg
Pull request #1977 (2020-12-28 21:55:57)

Drainer supports customized kubelet probe check
<!-- Thanks for sending a pull request! -->

# Changes

<!-- 
Describe your changes here- ideally you can get that description straight from
your descriptive commit message(s)! 

- :gift: Add new feature
- :bug: Fix bug
- :broom: Update or clean up current behavior
- :wastebasket: Remove feature or internal logic
-->

- Besides of directly returning "200 OK", drainer accepts customized kubelet probe check to perform when the handler is not in Drain status.

<!--
In addition, categorize the changes you're making using the "/kind" Prow command, example:

/kind <kind>

Supported kinds are: api-change, bug, cleanup, deprecation, removal, documentation, enhancement, performance

-->
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
<!-- Please include the 'why' behind your changes if no issue exists -->

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1977#" title="Author self-approved">grac3gao</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **vagababov** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @vagababov` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[network/OWNERS](https://github.com/knative/pkg/blob/master/network/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["vagababov"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/1977?src=pr&el=h1) Report
> Merging [#1977](https://codecov.io/gh/knative/pkg/pull/1977?src=pr&el=desc) (e5db6f6) into [master](https://codecov.io/gh/knative/pkg/commit/27db5ac24cfb43b1a40452df933fdf9922db4aba?el=desc) (27db5ac) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/pkg/pull/1977/graphs/tree.svg?width=650&height=150&src=pr&token=rlqpDnfxbO)](https://codecov.io/gh/knative/pkg/pull/1977?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1977      +/-   ##
==========================================
+ Coverage   69.06%   69.07%   +0.01%     
==========================================
  Files         210      210              
  Lines        8796     8799       +3     
==========================================
+ Hits         6075     6078       +3     
  Misses       2451     2451              
  Partials      270      270              
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/1977?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [network/handlers/drain.go](https://codecov.io/gh/knative/pkg/pull/1977/diff?src=pr&el=tree#diff-bmV0d29yay9oYW5kbGVycy9kcmFpbi5nbw==) | `83.72% <100.00%> (+1.22%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/1977?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/pkg/pull/1977?src=pr&el=footer). Last update [27db5ac...57be625](https://codecov.io/gh/knative/pkg/pull/1977?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

grac3gao: /cc @Harwayne 
grac3gao: /assign @vagababov 
knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1977#" title="Author self-approved">grac3gao</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mattmoor** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @mattmoor` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[network/OWNERS](https://github.com/knative/pkg/blob/master/network/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["mattmoor"]} -->
knative-prow-robot: New changes are detected. LGTM label has been removed.
knative-prow-robot: @grac3gao: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | 260a327d6cdfacb6ce09ac583145e4d28d89bae1 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1977/pull-knative-pkg-unit-tests/1346943594426011648) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1977). [Your PR dashboard](https://gubernator.knative.dev/pr/grac3gao).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
grac3gao: TestMetricsExport/OpenCensus failed, which seems has nothing to do with this change.
/test pull-knative-pkg-unit-tests
knative-prow-robot: @grac3gao: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | 260a327d6cdfacb6ce09ac583145e4d28d89bae1 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1977/pull-knative-pkg-unit-tests/1346947913644773376) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1977). [Your PR dashboard](https://gubernator.knative.dev/pr/grac3gao).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
vagababov: may be you still need the return after you report failure.
vagababov: Nah, that doesn't seem right....
grac3gao: /test pull-knative-pkg-unit-tests
grac3gao: /retest
grac3gao: /retest
vagababov: I kind of liked custom name, but 🤷 

Patch:
diff --git a/.github/workflows/knative-releasability.yaml b/.github/workflows/knative-releasability.yaml
index ecfd542a6..cecf0a52f 100644
--- a/.github/workflows/knative-releasability.yaml
+++ b/.github/workflows/knative-releasability.yaml
@@ -50,6 +50,9 @@ jobs:
           if [[ "${{ github.event.inputs.slackChannel }}" != "" ]]; then
             echo "SLACK_CHANNEL=${{ github.event.inputs.slackChannel }}" >> $GITHUB_ENV
           fi
+          if [[ "${{ secrets.SLACK_WEBHOOK }}" != "" ]]; then
+            echo "SLACK_WEBHOOK=exists" >> $GITHUB_ENV
+          fi
 
       - name: Set up Go 1.15.x
         uses: actions/setup-go@v2
@@ -142,7 +145,8 @@ jobs:
           echo 'SLACK_TITLE=Releasability for ${{ github.repository }} @ ${{ env.RELEASE }} is NO-GO!' >> $GITHUB_ENV
 
       - name: Post status to Slack
-        if: ${{ secrets.SLACK_WEBHOOK != '' && steps.exists.outputs.release-branch == 'false' }}
+        # Note: using env.SLACK_WEBHOOK here because secrets are not allowed in the if block.
+        if: env.SLACK_WEBHOOK != '' && steps.exists.outputs.release-branch == 'false'
         uses: rtCamp/action-slack-notify@v2.1.0
         env:
           SLACK_ICON: http://github.com/knative.png?size=48
diff --git a/network/handlers/drain.go b/network/handlers/drain.go
index 75919578c..0ed37ba6d 100644
--- a/network/handlers/drain.go
+++ b/network/handlers/drain.go
@@ -47,7 +47,8 @@ var newTimer = func(d time.Duration) timer {
 }
 
 // Drainer wraps an inner http.Handler to support responding to kubelet
-// probes and KProbes with a "200 OK" until the handler is told to Drain.
+// probes and KProbes with a "200 OK" until the handler is told to Drain,
+// or Drainer will optionally run the HealthCheck if it is defined.
 // When the Drainer is told to Drain, it will immediately start to fail
 // probes with a "500 shutting down", and the call will block until no
 // requests have been received for QuietPeriod (defaults to
@@ -56,6 +57,10 @@ type Drainer struct {
 	// Mutex guards the initialization and resets of the timer
 	sync.RWMutex
 
+	// HealthCheck is an optional health check that is performed until the drain signal is received.
+	// When unspecified, a "200 OK" is returned, otherwise this function is invoked.
+	HealthCheck http.HandlerFunc
+
 	// Inner is the http.Handler to which we delegate actual requests.
 	Inner http.Handler
 
@@ -78,6 +83,8 @@ func (d *Drainer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	if network.IsKubeletProbe(r) { // Respond to probes regardless of path.
 		if d.draining() {
 			http.Error(w, "shutting down", http.StatusServiceUnavailable)
+		} else if d.HealthCheck != nil {
+			d.HealthCheck(w, r)
 		} else {
 			w.WriteHeader(http.StatusOK)
 		}


Test patch:
diff --git a/network/handlers/drain_test.go b/network/handlers/drain_test.go
index dacf6eac7..43b6bd15e 100644
--- a/network/handlers/drain_test.go
+++ b/network/handlers/drain_test.go
@@ -19,6 +19,7 @@ package handlers
 import (
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"testing"
 	"time"
 
@@ -317,3 +318,47 @@ func TestDefaultQuietPeriod(t *testing.T) {
 	}
 	mt.advance(network.DefaultDrainTimeout)
 }
+
+func TestHealthCheck(t *testing.T) {
+	var (
+		w     http.ResponseWriter
+		req   = &http.Request{}
+		probe = &http.Request{
+			URL: &url.URL{
+				Path: "/healthz",
+			},
+			Header: http.Header{
+				"User-Agent": []string{network.KubeProbeUAPrefix},
+			},
+		}
+		cnt     = 0
+		inner   = http.HandlerFunc(func(http.ResponseWriter, *http.Request) { cnt++ })
+		checker = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+			if req.URL != nil && req.URL.Path == "/healthz" {
+				w.WriteHeader(http.StatusBadRequest)
+				return
+			}
+			w.WriteHeader(http.StatusAccepted)
+		})
+	)
+
+	drainer := &Drainer{
+		HealthCheck: checker,
+		Inner:       inner,
+	}
+
+	// Works before Drain is called.
+	drainer.ServeHTTP(w, req)
+	drainer.ServeHTTP(w, req)
+	drainer.ServeHTTP(w, req)
+	if cnt != 3 {
+		t.Error("Inner handler was not properly invoked")
+	}
+
+	// Works for HealthCheck.
+	resp := httptest.NewRecorder()
+	drainer.ServeHTTP(resp, probe)
+	if got, want := resp.Code, http.StatusBadRequest; got != want {
+		t.Errorf("Probe status = %d, wanted %d", got, want)
+	}
+}