Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TCP probe the user-container from the queue-proxy before marking the pod ready. #2915

Merged
merged 20 commits into from Jan 17, 2019

Conversation

@markusthoemmes
Copy link
Contributor

markusthoemmes commented Jan 15, 2019

As we cannot easily run the TCP probe from kubernetes to check the user-container is up because of Istio interfering, this adds a TCP probe that's run as part of the queue-proxy's readinessProbe. The queue-proxy will only report itself ready once the TCP probe to the user-container has succeeded.

This also externalizes the health state as kept in the queue-proxy to enable testability of that code.

Fixes #2912

Release Note

Hardened container startup and actually check the user-container for general availability.
Copy link

knative-prow-robot left a comment

@markusthoemmes: 1 warning.

In response to this:

As we cannot easily run the TCP probe from kubernetes to check the user-container is up because of Istio interfering, this adds a TCP probe that's run as part of the queue-proxy's readinessProbe. The queue-proxy will only report itself ready once the TCP probe to the user-container has succeeded.

This also externalizes the health state as kept in the queue-proxy to enable testability of that code.

Fixes #2912

TODO:

  • Tests

Release Note

Hardened container startup and actually check the user-container for general availability.

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.

pkg/queue/health/health_state.go Outdated Show resolved Hide resolved
@markusthoemmes markusthoemmes force-pushed the markusthoemmes:tcp-probe branch from 9a0d6b3 to 17bb88d Jan 15, 2019
Copy link
Contributor

vagababov left a comment

Thanks!

cmd/queue/main.go Outdated Show resolved Hide resolved
cmd/queue/main.go Outdated Show resolved Hide resolved
cmd/queue/main.go Outdated Show resolved Hide resolved
cmd/queue/main.go Outdated Show resolved Hide resolved
pkg/queue/health/health_state.go Outdated Show resolved Hide resolved
pkg/queue/health/health_state.go Outdated Show resolved Hide resolved
pkg/queue/health/health_state.go Outdated Show resolved Hide resolved
@markusthoemmes markusthoemmes force-pushed the markusthoemmes:tcp-probe branch 3 times, most recently from 8f97764 to 5a80457 Jan 16, 2019
@markusthoemmes markusthoemmes changed the title [WIP] TCP probe the user-container from the queue-proxy before marking the pod ready. TCP probe the user-container from the queue-proxy before marking the pod ready. Jan 16, 2019
@markusthoemmes

This comment has been minimized.

Copy link
Contributor Author

markusthoemmes commented Jan 16, 2019

Finally the thing works!

/test pull-knative-serving-integration-tests

@markusthoemmes

This comment has been minimized.

Copy link
Contributor Author

markusthoemmes commented Jan 16, 2019

/assign @tcnghia

cmd/queue/main.go Outdated Show resolved Hide resolved
pkg/queue/health/health_state.go Show resolved Hide resolved
pkg/queue/health/health_state_test.go Outdated Show resolved Hide resolved
@markusthoemmes

This comment has been minimized.

Copy link
Contributor Author

markusthoemmes commented Jan 16, 2019

@vagababov thanks for your feedback, addressed all your comments I believe.

Copy link
Contributor

vagababov left a comment

Just nits. Thanks for bearing with me :-)

cmd/queue/main.go Outdated Show resolved Hide resolved
pkg/queue/health/probe_test.go Outdated Show resolved Hide resolved
@vagababov

This comment has been minimized.

Copy link
Contributor

vagababov commented Jan 16, 2019

/test pull-knative-serving-integration-tests

…pod as ready.

As we cannot easily run the TCP probe from kubernetes to check the user-container is up because of Istio interfering, this adds a TCP probe that's run as part of the queue-proxy's readinessProbe. The queue-proxy will only report itself ready once the TCP probe to the user-container has succeeded.

This also externalizes the health state as kept in the queue-proxy to enable testability of that code.
@markusthoemmes markusthoemmes force-pushed the markusthoemmes:tcp-probe branch from c6c91f2 to 92dfc1d Jan 17, 2019
@knative-metrics-robot

This comment has been minimized.

Copy link

knative-metrics-robot commented Jan 17, 2019

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/queue/health/health_state.go Do not exist 100.0%
pkg/queue/health/probe.go Do not exist 100.0%
@markusthoemmes

This comment has been minimized.

Copy link
Contributor Author

markusthoemmes commented Jan 17, 2019

TestRunLatestService flake

/test pull-knative-serving-integration-tests

1 similar comment
@markusthoemmes

This comment has been minimized.

Copy link
Contributor Author

markusthoemmes commented Jan 17, 2019

TestRunLatestService flake

/test pull-knative-serving-integration-tests

@tcnghia

This comment has been minimized.

Copy link
Contributor

tcnghia commented Jan 17, 2019

/lgtm
/approve
Thanks @markusthoemmes @vagababov

@knative-prow-robot

This comment has been minimized.

Copy link

knative-prow-robot commented Jan 17, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes, tcnghia, vagababov

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:

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

@knative-prow-robot knative-prow-robot merged commit 72b23f0 into knative:master Jan 17, 2019
7 checks passed
7 checks passed
cla/google All necessary CLAs are signed
pull-knative-serving-build-tests Job succeeded.
Details
pull-knative-serving-go-coverage Job succeeded.
Details
pull-knative-serving-integration-tests Job succeeded.
Details
pull-knative-serving-unit-tests Job succeeded.
Details
pull-knative-serving-upgrade-tests Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.