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.

Sign up for GitHub

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

Jump to bottom

Tests for multi-container readiness and liveness probes #15180

Merged
merged 10 commits into from
May 7, 2024
Merged

Tests for multi-container readiness and liveness probes #15180

merged 10 commits into from
May 7, 2024

Conversation

mgencur
Copy link
Contributor

@mgencur mgencur commented May 2, 2024 •
edited
Loading

Fixes #14869
Fixes #12480

Proposed Changes

  • Remove misleading comment about multi-container Probes (they're now allowed for all containers)
  • Test readiness/liveness probes with sidecars (sidecar 1: Readiness probe, sidecar 2: Liveness, sidecar 3: Readiness + Liveness)
    • The final HTTP requests propagates through all the containers
    • Delay Readiness for one of the sidecars
  • Test for different types of probes (TCPSocket, HTTPGet+Exec, GRPC)
  • Test sidecar container being ready and then going unready, checking that Endpoints are properly removed from the private service.
  • For liveness probe, test happy-path scenarios for different types of probes for sidecars.
  • Test liveness probe "failure" for HTTPGet probe, but only for the user-container.

Release Note

@knative-prow knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 2, 2024
@knative-prow knative-prow bot requested review from izabelacg and ReToCode May 2, 2024 12:40
@codecov Codecov
Copy link

codecov bot commented May 2, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 84.74%. Comparing base (c2d0af1) to head (0a8c9ac).
Report is 91 commits behind head on main.

Files Patch % Lines
pkg/testing/v1/service.go 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #15180      +/-   ##
==========================================
+ Coverage   84.11%   84.74%   +0.63%     
==========================================
  Files         213      218       +5     
  Lines       16783    13472    -3311     
==========================================
- Hits        14117    11417    -2700     
+ Misses       2315     1688     -627     
- Partials      351      367      +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mgencur
Pass FORWARD_PORT in service_test
ac8a915
@mgencur
Revert readiness image to only fail readiness probe
cf78a49 
handleMain does not reflect the readiness state like it was before
@mgencur
Use proxy to start failing sidecar
ee022b0 
This reverts commit c6929e6e0e1190b15c990c7873b269abdae9609d.
@mgencur
Test liveness failure
815d1e4
@knative-prow knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 3, 2024
@mgencur
Copy link
Contributor Author

mgencur commented May 3, 2024 •
edited
Loading

The test for liveness probe failure was inspired by #12497. The issues that were blocking the original PR were either resolved (the main one) or I found a workaround (the test issue).

@mgencur
Address review feedback
0a35ba3 
* start package-private functions with lowercase
* use already existing const for "user-container"
* rename getPort to getServerPort to make it more specific (avoid
confusion with FORWARD_PORT)
@ReToCode
Copy link
Member

ReToCode commented May 6, 2024

LGTM, I can approve once https://github.com/knative/serving/actions/runs/8965592118/job/24619389624?pr=15180 passes.

ReToCode
ReToCode approved these changes May 6, 2024
View reviewed changes
Copy link
Member

@ReToCode ReToCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2024
@ReToCode
Copy link
Member

ReToCode commented May 6, 2024

@dsimansk mind approving the above? @dprotaso is on PTO and the pkg/apis change is only a (now invalid) comment.

@mgencur
Copy link
Contributor Author

mgencur commented May 6, 2024 •
edited
Loading

This PR removes an invalid comment about container probes here
Multi-container probes were added in #14853

@dsimansk @dprotaso Could any of you approve this change? Thanks

@dsimansk
Copy link
Contributor

dsimansk commented May 7, 2024

/approve
/lgtm

@knative-prow Knative Prow
Copy link

knative-prow bot commented May 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, mgencur, ReToCode

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 knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2024
@knative-prow knative-prow bot merged commit 9046081 into knative:main May 7, 2024
67 checks passed
mgencur added a commit to mgencur/serving-1 that referenced this pull request May 9, 2024
@mgencur
Tests for multi-container readiness and liveness probes (knative#15180)
be530b6 
* Tests for multi-container readiness and liveness probes

* Use PORT instead of HEALTHCHECK_PORT for serving container

* Fixes

* Pass FORWARD_PORT in service_test

* Revert readiness image to only fail readiness probe

handleMain does not reflect the readiness state like it was before

* Use proxy to start failing sidecar

This reverts commit c6929e6e0e1190b15c990c7873b269abdae9609d.

* Test liveness failure

* Use /healthz/readiness in multicontainer test

* Address review feedback

* start package-private functions with lowercase
* use already existing const for "user-container"
* rename getPort to getServerPort to make it more specific (avoid
confusion with FORWARD_PORT)

* Fix lint
openshift-merge-bot bot pushed a commit to openshift-knative/serving that referenced this pull request May 13, 2024
@mgencur
Tests for multi-container readiness and liveness probes (#710)
538c4f0 
* Enable multi-container probing tests in openshift/e2e-common.sh

* Tests for multi-container readiness and liveness probes (knative#15180)

* Tests for multi-container readiness and liveness probes

* Use PORT instead of HEALTHCHECK_PORT for serving container

* Fixes

* Pass FORWARD_PORT in service_test

* Revert readiness image to only fail readiness probe

handleMain does not reflect the readiness state like it was before

* Use proxy to start failing sidecar

This reverts commit c6929e6e0e1190b15c990c7873b269abdae9609d.

* Test liveness failure

* Use /healthz/readiness in multicontainer test

* Address review feedback

* start package-private functions with lowercase
* use already existing const for "user-container"
* rename getPort to getServerPort to make it more specific (avoid
confusion with FORWARD_PORT)

* Fix lint
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/knative-serving that referenced this pull request May 13, 2024
@mgencur
Tests for multi-container readiness and liveness probes (knative#15180)
c3f29a9 
* Tests for multi-container readiness and liveness probes

* Use PORT instead of HEALTHCHECK_PORT for serving container

* Fixes

* Pass FORWARD_PORT in service_test

* Revert readiness image to only fail readiness probe

handleMain does not reflect the readiness state like it was before

* Use proxy to start failing sidecar

This reverts commit c6929e6e0e1190b15c990c7873b269abdae9609d.

* Test liveness failure

* Use /healthz/readiness in multicontainer test

* Address review feedback

* start package-private functions with lowercase
* use already existing const for "user-container"
* rename getPort to getServerPort to make it more specific (avoid
confusion with FORWARD_PORT)

* Fix lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ReToCode ReToCode ReToCode approved these changes

@izabelacg izabelacg Awaiting requested review from izabelacg

Assignees

@dsimansk dsimansk

@ReToCode ReToCode

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add more e2e tests for the different probes No test coverage for liveness probes
3 participants
@mgencur @ReToCode @dsimansk