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

Add missing deployment probes #13563

Merged
merged 1 commit into from
Mar 6, 2023
Merged

Conversation

skonto
Copy link
Contributor

@skonto skonto commented Dec 19, 2022

partially fixes knative/pkg#1048 for now.

Proposed Changes

Release Note

Controllers now have liveness and readiness probes

@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/API API objects and controllers area/autoscale labels Dec 19, 2022
@codecov
Copy link

codecov bot commented Dec 19, 2022

Codecov Report

Base: 86.19% // Head: 86.19% // No change to project coverage 👍

Coverage data is based on head (9afed75) compared to base (8c282ba).
Patch has no changes to coverable lines.

❗ Current head 9afed75 differs from pull request most recent head f69f7cc. Consider uploading reports for the commit f69f7cc to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #13563   +/-   ##
=======================================
  Coverage   86.19%   86.19%           
=======================================
  Files         197      197           
  Lines       14775    14775           
=======================================
  Hits        12736    12736           
  Misses       1736     1736           
  Partials      303      303           

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@skonto skonto force-pushed the add_missing_probes branch 3 times, most recently from 2b1a805 to c8a61fd Compare December 19, 2022 13:45
@skonto
Copy link
Contributor Author

skonto commented Dec 19, 2022

/retest

@skonto
Copy link
Contributor Author

skonto commented Dec 19, 2022

Failures seem irrelevant:

jq: error (at <stdin>:1): Cannot index string with string "tag_name"```

@skonto
Copy link
Contributor Author

skonto commented Dec 20, 2022

@dprotaso @psschwei gentle ping

Copy link
Member

@dprotaso dprotaso left a comment

Choose a reason for hiding this comment

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

It seems like the intent of the linked issue was to add this logic to the sharedmain package - why not make the necessary changes in knative/pkg ?

@@ -226,5 +219,4 @@ func main() {
}

logger.Info("Updated default domain to: ", domain)
server.Shutdown(context.Background())
Copy link
Member

Choose a reason for hiding this comment

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

Shutdown was blocking before and now it's not. This means main will exit and run defer's etc.

Copy link
Contributor Author

@skonto skonto Jan 18, 2023

Choose a reason for hiding this comment

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

Ah yes, correct will fix. Just need to block on the context: <-ctx.Done() or something.

@skonto
Copy link
Contributor Author

skonto commented Jan 18, 2023

Ok I will move the logic in the knative/pkg I agree it would be useful there and then update the PR here to test it.

@dprotaso
Copy link
Member

/hold

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2023
@evankanderson
Copy link
Member

(When you update pkg, can you reference this PR, so we can track when this should be revised?)

@skonto skonto changed the title Add missing deployment probes [wip] Add missing deployment probes Jan 26, 2023
@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 26, 2023
@skonto skonto force-pushed the add_missing_probes branch 2 times, most recently from 8c6c986 to e89fd48 Compare February 8, 2023 15:23
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2023
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2023
@knative-prow knative-prow bot 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 17, 2023
@skonto skonto changed the title [wip] Add missing deployment probes Add missing deployment probes Feb 17, 2023
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 17, 2023
@skonto
Copy link
Contributor Author

skonto commented Feb 17, 2023

@dprotaso gentle ping, this should be quick.

@skonto
Copy link
Contributor Author

skonto commented Feb 20, 2023

@dprotaso @evankanderson gentle ping.

Copy link
Member

@evankanderson evankanderson 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 Feb 20, 2023
@knative-prow
Copy link

knative-prow bot commented Feb 20, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Feb 20, 2023
@skonto
Copy link
Contributor Author

skonto commented Feb 20, 2023

@dprotaso is it ok to unhold?

@skonto
Copy link
Contributor Author

skonto commented Feb 21, 2023

@dprotaso gentle ping.

@dprotaso
Copy link
Member

dprotaso commented Mar 6, 2023

/unhold

/lgtm thanks

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2023
@knative-prow knative-prow bot merged commit 587f587 into knative:main Mar 6, 2023
nak3 pushed a commit to nak3/serving that referenced this pull request Mar 8, 2023
openshift-merge-robot pushed a commit to openshift-knative/serving that referenced this pull request Mar 8, 2023
* add probes to all deployments (#176)

* add missing probes (knative#13563)

* Bump manifests

---------

Co-authored-by: Stavros Kontopoulos <st.kontopoulos@gmail.com>
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/knative-serving that referenced this pull request Mar 8, 2023
openshift-merge-robot pushed a commit to openshift-knative/serving that referenced this pull request Mar 9, 2023
* add probes to all deployments (#176)

* add missing probes (knative#13563)

* Bump manifests

---------

Co-authored-by: Stavros Kontopoulos <st.kontopoulos@gmail.com>
Co-authored-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com>
@skonto skonto mentioned this pull request Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/autoscale lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt health checks in sharedmain
4 participants