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

failureThreshold and timeoutSeconds parameter should be optional as per k8s doc #5732

Closed
savitaashture opened this issue Oct 1, 2019 · 6 comments · Fixed by #10700
Closed
Labels
area/API API objects and controllers good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/feature Well-understood/specified features, ready for coding. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Milestone

Comments

@savitaashture
Copy link
Contributor

savitaashture commented Oct 1, 2019

In what area(s)?

/area API
/kind good-first-issue

/area autoscale
/area build
/area monitoring
/area networking
/area test-and-release

Describe the feature

Currently when create a service with below provided info

apiVersion: serving.knative.dev/v1alpha1
kind: Service
metadata:
  name: hello
spec:
  template:
    metadata:
      name: hello-test
      annotations:
    spec:
      containers:
      - image: savita3020/helloworld
        readinessProbe:
          exec:
            command:
            - cat
            - /tmp/healthy
          initialDelaySeconds: 5
          periodSeconds: 5

Actual Output:

Error from server (BadRequest): error when creating "vol.yaml": admission webhook "resource.webhook.serving.knative.dev" denied the request: mutation failed: expected 1 <= 0 <= 2147483647: spec.template.spec.containers[0].readinessProbe.failureThreshold, spec.template.spec.containers[0].readinessProbe.timeoutSeconds

Expected Output:
Should create service successfully because failureThreshold and timeoutSeconds are optional fields if don't specify should take the default values as per doc

But in k8s_validation.go because check its failing.

Implementation:
Should provide a default values for failureThreshold and timeoutSeconds same like SuccessThreshold

So even if we don't specify failureThreshold and timeoutSeconds in service yaml it should create successfully

@danielqsj
Copy link

/assign

@dgerd
Copy link

dgerd commented Oct 15, 2019

This seems to be coming up more often recently so we can potentially revisit the decision here, but this is purposeful. Knative Serving does not necessarily follow K8s defaults or validation.

We have chosen to deviate here to enable sub-second probing by default. K8s does not allow for setting a granularity less than 1 second, but we needed to go lower and we wanted this experience by default as cold-start is critical for serverless workloads. Because we deviated here defaulting and validation logic also needed to change.

See #4780 (comment) and kubernetes/kubernetes#76951

@eallred-google eallred-google added this to the Needs Triage milestone Oct 23, 2019
@knative-housekeeping-robot

Issues go stale after 90 days of inactivity.
Mark the issue as fresh by adding the comment /remove-lifecycle stale.
Stale issues rot after an additional 30 days of inactivity and eventually close.
If this issue is safe to close now please do so by adding the comment /close.

Send feedback to Knative Productivity Slack channel or file an issue in knative/test-infra.

/lifecycle stale

@knative-prow-robot knative-prow-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 22, 2020
@dgerd
Copy link

dgerd commented Jan 24, 2020

/remove-lifecycle stale
as there is an open PR referencing this issue.

@knative-prow-robot knative-prow-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 24, 2020
@ahmetb
Copy link
Contributor

ahmetb commented Apr 15, 2020

/lifecycle frozen
as it's needed.

@knative-prow-robot knative-prow-robot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Apr 15, 2020
@dprotaso dprotaso added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. and removed kind/good-first-issue labels Jan 19, 2021
@dprotaso
Copy link
Member

dprotaso commented Feb 5, 2021

Note the properties are optional if you're defaulting into k8s style probe (ie. periodSeconds > 0)

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 good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/feature Well-understood/specified features, ready for coding. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
8 participants