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

Reject negative Retry config #4216

Merged

Conversation

pierDipi
Copy link
Member

@pierDipi pierDipi commented Oct 5, 2020

A user can specify a negative number of retries without getting any
validation error. This PR adds a check to validate that
deliverySpec.retry is a non negative integer.

Proposed Changes

  • Reject negative Retry config

Release Note

- 🧽 Update or clean up current behavior
DeliverySpec validation rejects a negative retry config.

A user can specify a negative number of retries without getting any
validation error. This PR adds a check to validate that
`deliverySpec.retry` is a non negative integer.

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 5, 2020
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 5, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2020
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/duck/v1/delivery_types.go 92.3% 93.3% 1.0
pkg/apis/duck/v1beta1/delivery_types.go 92.3% 93.3% 1.0

@pierDipi
Copy link
Member Author

pierDipi commented Oct 5, 2020

Ref: #4165

http: TLS handshake error from 10.24.1.1:44280: server key missing

/retest

@knative-test-reporter-robot

The following jobs failed:

Test name Triggers Retries
pull-knative-eventing-upgrade-tests 0/3

Failed non-flaky tests preventing automatic retry of pull-knative-eventing-upgrade-tests:

.Test

@pierDipi
Copy link
Member Author

pierDipi commented Oct 5, 2020

Ref: #4165

http: TLS handshake error from 10.24.1.1:44280: server key missing

/retest

@lionelvillard
Copy link
Member

@pierDipi #4165 has been merged. Are you suggesting it didn't fix the infinite webhook crash loop?

/cc @ian-mi

@pierDipi
Copy link
Member Author

pierDipi commented Oct 5, 2020

It hasn't been backported to 0.18, so the upgrade test fails when it installs eventing from the latest public release.

@pierDipi
Copy link
Member Author

pierDipi commented Oct 5, 2020

From logs here: https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_eventing/4216/pull-knative-eventing-upgrade-tests/1313122358009008128#1:build-log.txt%3A137, I see:

=====>>=======================================================
==== >> INSTALLING KNATIVE EVENTING LATEST PUBLIC RELEASE ====
=====>>=======================================================
==== Mon Oct  5 07:26:32 PDT 2020
=====>>=======================================================
// ...
namespace/knative-eventing created
// ...
Waiting until all pods in namespace knative-eventing-wvozzrkvkv are up......................................................................................................................................................
ERROR: timeout waiting for pods to come up
eventing-controller-6fc9c6cfc4-4kfsn    1/1   Running            0     5m18s
eventing-controller-6fc9c6cfc4-qk6h8    1/1   Running            0     5m18s
eventing-controller-6fc9c6cfc4-sh9fc    1/1   Running            0     5m18s
eventing-webhook-667c8f6dc4-gvsqj       0/1   CrashLoopBackOff   5     5m23s
eventing-webhook-667c8f6dc4-mrpfr       0/1   CrashLoopBackOff   5     5m23s
eventing-webhook-667c8f6dc4-pwdfk       0/1   CrashLoopBackOff   5     5m23s
imc-controller-6c4f87765c-7r242         1/1   Running            0     5m30s
imc-dispatcher-74dcf4647f-gnwlg         1/1   Running            0     5m29s
mt-broker-controller-6d789b944d-f4cd2   1/1   Running            0     5m31s
mt-broker-filter-6bbcc67bc5-bqb7b       1/1   Running            0     5m31s
mt-broker-ingress-64987f6f4-fhszq       1/1   Running            0     5m31s

// Failed pod logs
{"level":"info","ts":"2020-10-05T14:30:58.272Z","logger":"eventing-webhook","caller":"webhook/webhook.go:226","msg":"Starting to fail readiness probes...","commit":"74b9bed","knative.dev/pod":"eventing-webhook-667c8f6dc4-gvsqj"}
{"level":"info","ts":"2020-10-05T14:30:58.273Z","logger":"eventing-webhook.sinkbindings.webhook.sources.knative.dev","caller":"controller/controller.go:458","msg":"Shutting down workers","commit":"74b9bed","knative.dev/pod":"eventing-webhook-667c8f6dc4-gvsqj"}
2020/10/05 14:30:58 http: TLS handshake error from 10.24.0.1:40222: server key missing

@lionelvillard
Copy link
Member

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2020
@knative-prow-robot knative-prow-robot merged commit f0dc15b into knative:master Oct 5, 2020
@pierDipi pierDipi deleted the validate-negative-retry branch October 5, 2020 15:12
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. cla: yes Indicates the PR's author has signed the CLA. 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.

6 participants