Refactor controller-manager: turn Serve func into handlerchain builder #60197

hzxuzhonghu · 2018-02-22T09:34:55Z

What this PR does / why we need it:
follow up #59582 fix

turn Serve func into a handler chain builder #59582 (comment)

Special notes for your reviewer:

Release note:

NONE

hzxuzhonghu · 2018-02-22T09:35:12Z

/assign @sttts @deads2k

hzxuzhonghu · 2018-02-22T09:36:25Z

/assign @lavalamp

hzxuzhonghu · 2018-02-22T09:52:16Z

/cc @stewart-yu

k8s-ci-robot · 2018-02-22T09:52:17Z

@hzxuzhonghu: GitHub didn't allow me to request PR reviews from the following users: stewart-yu.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @stewart-yu

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

deads2k · 2018-02-22T12:54:24Z

cmd/controller-manager/app/serve.go

-// Serve creates a base handler chain for a controller manager. It runs the
-// the chain with the given serveFunc.
-func Serve(c *CompletedConfig, serveFunc serveFunc, stopCh <-chan struct{}) error {
+func DefaultBuildHandlerChain(c *CompletedConfig) http.Handler {


take a handler in? Trying to match func(apiHandler http.Handler, c *Config) (secure http.Handler)

No, remove serverFunc. DefaultBuildHandlerChain only takes in config and return a handler like it does in apiserver.

github does not compare accurately

deads2k · 2018-02-22T13:25:13Z

cmd/controller-manager/app/serve.go

-// the chain with the given serveFunc.
-func Serve(c *CompletedConfig, serveFunc serveFunc, stopCh <-chan struct{}) error {
+// DefaultBuildHandlerChain takes in CompletedConfig and returns a handler.
+func DefaultBuildHandlerChain(c *CompletedConfig) http.Handler {


We're trying to match this: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/server/config.go#L130 so that we'll have choices about using this instead.

Thanks for your explanation. And will think about tomorrow morning。

@deads2k PTAL

lavalamp · 2018-02-24T00:14:41Z

/assign @cheftako

sttts · 2018-02-27T11:51:12Z

cmd/controller-manager/app/serve.go

-// the chain with the given serveFunc.
-func Serve(c *CompletedConfig, serveFunc serveFunc, stopCh <-chan struct{}) error {
+// NewDebugHandler takes in CompletedConfig and returns a handler.
+func NewDebugHandler(c *CompletedConfig) http.Handler {


healthz is not really debug. Can we call it NewBaseHandler or something like that?

sttts · 2018-02-27T11:51:32Z

Lgtm, only the name DebugHandler is not so good IMO.

hzxuzhonghu · 2018-02-27T12:26:44Z

Lgtm, only the name DebugHandler is not so good IMO.

Done, renamed to NewBaseHandler

sttts · 2018-02-27T13:14:05Z

cmd/controller-manager/app/serve.go

@@ -33,11 +32,23 @@ import (
 	"k8s.io/kubernetes/pkg/util/configz"
 )

-type serveFunc func(handler http.Handler, shutdownTimeout time.Duration, stopCh <-chan struct{}) error
+// DefaultBuildHandlerChain takes in handler and CompletedConfig and returns a handler.
+func DefaultBuildHandlerChain(apiHandler http.Handler, c *CompletedConfig) http.Handler {


nit: as this is hardcoded anyway, we could just call is BuildHandlerChain. In the apiserver it can be customized for the config. So we called it DefaultBuildHandlerChain because it is the default func.

oh, ic, missed it. fixed now

sttts · 2018-02-27T13:14:26Z

/approve
Lgtm, only little nit: https://github.com/kubernetes/kubernetes/pull/60197/files#r170916600

hzxuzhonghu · 2018-02-28T02:21:39Z

/retest

deads2k · 2018-03-05T15:43:42Z

/lgtm

thockin · 2018-03-05T15:43:56Z

This change is

hzxuzhonghu · 2018-03-06T01:41:22Z

rebased @thockin for approval

hzxuzhonghu · 2018-03-22T01:33:19Z

kindly ping @thockin for approval

sttts · 2018-03-22T09:50:37Z

@luxas can you approve?

hzxuzhonghu · 2018-04-09T08:03:30Z

ping @luxas can you approve?

luxas

/approve

Thanks @sttts for the ping on Slack. I'm and have been OOO for the first four months of the year, so I don't watch Github or notification email. Sorry for not seeing this.

Thanka for the patch!

sttts · 2018-04-10T09:07:53Z

/lgtm

k8s-ci-robot · 2018-04-10T09:08:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, hzxuzhonghu, luxas, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/cloud-controller-manager/OWNERS~~ [luxas]
~~cmd/controller-manager/OWNERS~~ [deads2k,sttts]
~~cmd/kube-controller-manager/OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2018-04-10T14:12:52Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-04-10T15:51:16Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-04-10T15:56:15Z

Automatic merge from submit-queue (batch tested with PRs 60197, 61614, 62074, 62071, 62301). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 22, 2018

k8s-ci-robot assigned deads2k and sttts Feb 22, 2018

k8s-ci-robot requested review from thockin and wlan0 February 22, 2018 09:35

k8s-ci-robot assigned lavalamp Feb 22, 2018

hzxuzhonghu force-pushed the controller-manager-handler-chain branch from dfb7068 to d20e4e5 Compare February 22, 2018 09:43

deads2k reviewed Feb 22, 2018

View reviewed changes

hzxuzhonghu force-pushed the controller-manager-handler-chain branch from 400d5d6 to 5654c2d Compare February 22, 2018 13:18

deads2k reviewed Feb 22, 2018

View reviewed changes

hzxuzhonghu force-pushed the controller-manager-handler-chain branch from 5654c2d to d7ac2be Compare February 23, 2018 06:17

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 23, 2018

hzxuzhonghu force-pushed the controller-manager-handler-chain branch from d7ac2be to e32aebe Compare February 23, 2018 08:50

k8s-ci-robot assigned cheftako Feb 24, 2018

sttts reviewed Feb 27, 2018

View reviewed changes

hzxuzhonghu force-pushed the controller-manager-handler-chain branch from e32aebe to d3b3db9 Compare February 27, 2018 12:09

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 27, 2018

hzxuzhonghu force-pushed the controller-manager-handler-chain branch from d3b3db9 to 51c6043 Compare February 27, 2018 12:14

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2018

hzxuzhonghu force-pushed the controller-manager-handler-chain branch from 51c6043 to 9243339 Compare February 27, 2018 12:17

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 27, 2018

sttts reviewed Feb 27, 2018

View reviewed changes

hzxuzhonghu force-pushed the controller-manager-handler-chain branch from 9243339 to 1233b17 Compare February 28, 2018 01:42

hzxuzhonghu closed this Feb 28, 2018

hzxuzhonghu reopened this Feb 28, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2018

Refactor controller-manager: turn Serve func into handlerchain builder

540a1de

hzxuzhonghu force-pushed the controller-manager-handler-chain branch from 1233b17 to 540a1de Compare March 6, 2018 01:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 6, 2018

luxas approved these changes Apr 10, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 10, 2018

k8s-github-robot merged commit 60c6d0e into kubernetes:master Apr 10, 2018

hzxuzhonghu deleted the controller-manager-handler-chain branch April 11, 2018 01:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor controller-manager: turn Serve func into handlerchain builder #60197

Refactor controller-manager: turn Serve func into handlerchain builder #60197

hzxuzhonghu commented Feb 22, 2018

hzxuzhonghu commented Feb 22, 2018

hzxuzhonghu commented Feb 22, 2018

hzxuzhonghu commented Feb 22, 2018

k8s-ci-robot commented Feb 22, 2018

deads2k Feb 22, 2018

hzxuzhonghu Feb 22, 2018

hzxuzhonghu Feb 22, 2018

deads2k Feb 22, 2018

hzxuzhonghu Feb 22, 2018

hzxuzhonghu Feb 23, 2018

lavalamp commented Feb 24, 2018

sttts Feb 27, 2018

hzxuzhonghu Feb 27, 2018

sttts commented Feb 27, 2018

hzxuzhonghu commented Feb 27, 2018

sttts Feb 27, 2018

hzxuzhonghu Feb 28, 2018

sttts commented Feb 27, 2018

hzxuzhonghu commented Feb 28, 2018

deads2k commented Mar 5, 2018

thockin commented Mar 5, 2018

hzxuzhonghu commented Mar 6, 2018

hzxuzhonghu commented Mar 22, 2018

sttts commented Mar 22, 2018

hzxuzhonghu commented Apr 9, 2018

luxas left a comment

sttts commented Apr 10, 2018

k8s-ci-robot commented Apr 10, 2018

fejta-bot commented Apr 10, 2018

k8s-github-robot commented Apr 10, 2018

k8s-github-robot commented Apr 10, 2018

Refactor controller-manager: turn Serve func into handlerchain builder #60197

Refactor controller-manager: turn Serve func into handlerchain builder #60197

Conversation

hzxuzhonghu commented Feb 22, 2018

hzxuzhonghu commented Feb 22, 2018

hzxuzhonghu commented Feb 22, 2018

hzxuzhonghu commented Feb 22, 2018

k8s-ci-robot commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Feb 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Feb 27, 2018

hzxuzhonghu commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Feb 27, 2018

hzxuzhonghu commented Feb 28, 2018

deads2k commented Mar 5, 2018

thockin commented Mar 5, 2018

hzxuzhonghu commented Mar 6, 2018

hzxuzhonghu commented Mar 22, 2018

sttts commented Mar 22, 2018

hzxuzhonghu commented Apr 9, 2018

luxas left a comment

Choose a reason for hiding this comment

sttts commented Apr 10, 2018

k8s-ci-robot commented Apr 10, 2018

fejta-bot commented Apr 10, 2018

k8s-github-robot commented Apr 10, 2018

k8s-github-robot commented Apr 10, 2018