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

Improve the error messages from our readiness probe validation #5382

Closed
mattmoor opened this issue Sep 4, 2019 · 0 comments · Fixed by #5385
Closed

Improve the error messages from our readiness probe validation #5382

mattmoor opened this issue Sep 4, 2019 · 0 comments · Fixed by #5385
Labels
area/API API objects and controllers kind/bug Categorizes issue or PR as related to a bug.

Comments

@mattmoor
Copy link
Member

mattmoor commented Sep 4, 2019

In what area(s)?

/area API

What version of Knative?

HEAD

Expected Behavior

When validating readiness probe fields, e.g. failureThreshold we should make clear that it is not wholly disallowed, but disallowed when periodSeconds is zero (and that either periodSeconds should be specified, or failureThreshold omitted).

Actual Behavior

When failureThreshold is provided without periodSeconds we state that failureThreshold is outright disallowed

@mattmoor mattmoor added the kind/bug Categorizes issue or PR as related to a bug. label Sep 4, 2019
@knative-prow-robot knative-prow-robot added area/API API objects and controllers kind/good-first-issue labels Sep 4, 2019
nak3 added a commit to nak3/serving that referenced this issue Sep 4, 2019
As described in knative#5382,
`failureThreshold` and `timeoutSeconds` in readinessprobe are
disapplowed only when `periodSeconds` is zero. This patch improves the
error.

- Error message after this patch:
```
error: services.serving.knative.dev "hello-example" could not be patched: Internal error occurred: admission webhook "webhook.serving.knative.dev" denied the request: mutation failed: timeoutSeconds is disallowed when periodSeconds is zero: spec.template.spec.containers[0].readinessProbe.failureThreshold, spec.template.spec.containers[0].readinessProbe.periodSeconds
```
knative-prow-robot pushed a commit that referenced this issue Sep 4, 2019
* Improve error from readiness probe validation

As described in #5382,
`failureThreshold` and `timeoutSeconds` in readinessprobe are
disapplowed only when `periodSeconds` is zero. This patch improves the
error.

- Error message after this patch:
```
error: services.serving.knative.dev "hello-example" could not be patched: Internal error occurred: admission webhook "webhook.serving.knative.dev" denied the request: mutation failed: timeoutSeconds is disallowed when periodSeconds is zero: spec.template.spec.containers[0].readinessProbe.failureThreshold, spec.template.spec.containers[0].readinessProbe.periodSeconds
```

* Remove periodSeconds from path in readiness validation error
knative-prow-robot pushed a commit to knative/pkg that referenced this issue Sep 12, 2019
)

* Introduce error util ErrInvalidCombination for invalid combination

Sometimes valid value becomes invalid value by combination.

example 1. knative/serving#5382
example 2. following combination in `spec.traffic`.

```
  traffic:
  - latestRevision: true
    revisionName: hello-example-dk7nd
    percent: 100
```

But there are no error util for them, so we need to create
custom error like knative/serving@c1583f3
or `ErrInvalidValue`.

The custom error will make code complicated and `ErrInvalidValue` is
not debug friendly.

To solve it, this patch introduces an util func `ErrInvalidCombination`.

* Introduce ErrGeneric instead of ErrInvalidCombination
knative-prow-robot pushed a commit to knative/pkg that referenced this issue Sep 13, 2019
* Introduce error util ErrInvalidCombination for invalid combination (#638)

* Introduce error util ErrInvalidCombination for invalid combination

Sometimes valid value becomes invalid value by combination.

example 1. knative/serving#5382
example 2. following combination in `spec.traffic`.

```
  traffic:
  - latestRevision: true
    revisionName: hello-example-dk7nd
    percent: 100
```

But there are no error util for them, so we need to create
custom error like knative/serving@c1583f3
or `ErrInvalidValue`.

The custom error will make code complicated and `ErrInvalidValue` is
not debug friendly.

To solve it, this patch introduces an util func `ErrInvalidCombination`.

* Introduce ErrGeneric instead of ErrInvalidCombination

* golang format tools (#672)

Produced via:
  `gofmt -s -w $(find -path './vendor' -prune -o -type f -name '*.go' -print))`
  `goimports -w $(find -name '*.go' | grep -v vendor)`
/assign mattmoor
nak3 added a commit to nak3/serving that referenced this issue Sep 17, 2019
This PR is basically same idea with he fix of knative#5382.

When invalid combination is configured, the ErrInvalidValue is printed for now.
It is not accurate message and will mislead users.

To fix it, this patch prints accurate message with ErrGeneric.
nak3 added a commit to nak3/serving that referenced this issue Sep 17, 2019
This PR is basically same idea with the fix of knative#5382.

When invalid combination is configured, the ErrInvalidValue is printed for now.
It is not accurate message and will mislead users.

To fix it, this patch prints accurate message with ErrGeneric.
knative-prow-robot pushed a commit that referenced this issue Oct 3, 2019
* Improve the error message when invalid combination

This PR is basically same idea with the fix of #5382.

When invalid combination is configured, the ErrInvalidValue is printed for now.
It is not accurate message and will mislead users.

To fix it, this patch prints accurate message with ErrGeneric.

* Start log messages with lowercase character
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API API objects and controllers kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants