Skip to content

Deployment probe fixes #13885

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
Merged
knative-prow merged 4 commits into from
Apr 24, 2023
Merged

Deployment probe fixes #13885

knative-prow merged 4 commits into from
Apr 24, 2023

Conversation

@skonto
Copy link
Contributor

@skonto skonto commented Apr 14, 2023 •
edited
Loading

@knative-prow knative-prow bot requested review from KauzClay and jsanin-vmw April 14, 2023 12:25
@knative-prow knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/API API objects and controllers labels Apr 14, 2023
@skonto skonto mentioned this pull request Apr 14, 2023
Merged
@skonto skonto requested review from ReToCode and nak3 April 14, 2023 12:26
@codecov
Copy link

codecov bot commented Apr 14, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (c377269) 86.23% compared to head (8245c67) 86.23%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #13885   +/-   ##
=======================================
  Coverage   86.23%   86.23%           
=======================================
  Files         199      199           
  Lines       14706    14749   +43     
=======================================
+ Hits        12681    12719   +38     
- Misses       1725     1729    +4     
- Partials      300      301    +1

see 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

dprotaso
dprotaso reviewed Apr 14, 2023
View reviewed changes
cmd/domain-mapping-webhook/main.go Outdated
})

sharedmain.WebhookMainWithContext(ctx, "domainmapping-webhook",
sharedmain.WebhookMainWithContext(sharedmain.WithHealthProbesDisabled(ctx), "domainmapping-webhook",
Copy link
Member

@dprotaso dprotaso Apr 14, 2023

Choose a reason for hiding this comment

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

can we wrap the context on a separate line so it reads better

ctx = sharedmain.WithHealthProbesDisabled(ctx)

evankanderson
evankanderson reviewed Apr 15, 2023
View reviewed changes
config/core/deployments/domainmapping-controller.yaml Outdated
scheme: HTTP
periodSeconds: 5
failureThreshold: 5
failureThreshold: 6
Copy link
Member

@evankanderson evankanderson Apr 15, 2023

Choose a reason for hiding this comment

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

I think we want the failureThreshold for readinessProbes to be lower than the failureThreshold for livenessProbes. Otherwise, we're choosing to terminate the container before we remove it from the active set (which was the issue that Julz filed).

Copy link
Contributor Author

@skonto skonto Apr 24, 2023

Choose a reason for hiding this comment

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

Correct the order of probes tricked me.

ReToCode
ReToCode reviewed Apr 17, 2023
View reviewed changes
Copy link
Contributor

@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.

Other than existing comments, LGTM.

@dprotaso
Copy link
Member

dprotaso commented Apr 24, 2023

bump @skonto - release is tomorrow

@skonto
Copy link
Contributor Author

skonto commented Apr 24, 2023

@dprotaso I will update shortly.

@skonto
Copy link
Contributor Author

skonto commented Apr 24, 2023

@dprotaso this is ready to stamp.

@dprotaso
Copy link
Member

dprotaso commented Apr 24, 2023

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2023
@knative-prow
Copy link

knative-prow bot commented Apr 24, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, skonto

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

The pull request process is described here

Details 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 Apr 24, 2023
@knative-prow knative-prow bot merged commit 22783d6 into knative:main Apr 24, 2023
skonto added a commit to skonto/serving that referenced this pull request Apr 25, 2023
@skonto
Deployment probe fixes (knative#13885)
c7a7ff0 
* probe fixes

* fixes

* fix threshold

* cleanup
@skonto skonto mentioned this pull request Apr 25, 2023
Merged
openshift-merge-robot pushed a commit to openshift-knative/serving that referenced this pull request Apr 25, 2023
@skonto
Deployment probe fixes (knative#13885) (#290)
a234d97 
* probe fixes

* fixes

* fix threshold

* cleanup
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/knative-serving that referenced this pull request Apr 26, 2023
@skonto
Deployment probe fixes (knative#13885)
1f744b2 
* probe fixes

* fixes

* fix threshold

* cleanup
ReToCode pushed a commit to ReToCode/serving that referenced this pull request Apr 27, 2023
@skonto @ReToCode
Deployment probe fixes (knative#13885)
dde65c4 
* probe fixes

* fixes

* fix threshold

* cleanup
@ReToCode ReToCode mentioned this pull request Apr 27, 2023
Merged
openshift-merge-robot pushed a commit to openshift-knative/serving that referenced this pull request Apr 27, 2023
@ReToCode @skonto
[RELEASE-1.9] [SRVKS-1046] Deployment probe fixes (#296)
b42e479 
* Deployment probe fixes (knative#13885)

* probe fixes

* fixes

* fix threshold

* cleanup

* Run `openshift/release/generate-release.sh release-v1.9`

---------

Co-authored-by: Stavros Kontopoulos <st.kontopoulos@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dprotaso dprotaso dprotaso left review comments

@evankanderson evankanderson evankanderson left review comments

@jsanin-vmw jsanin-vmw Awaiting requested review from jsanin-vmw

@KauzClay KauzClay Awaiting requested review from KauzClay

@nak3 nak3 Awaiting requested review from nak3

+1 more reviewer

@ReToCode ReToCode ReToCode left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@dprotaso dprotaso

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@skonto @dprotaso @evankanderson @ReToCode