knative / serving

Kubernetes-based, scale-to-zero, request-driven compute
https://knative.dev/docs/serving/
Apache License 2.0
5.56k stars 1.16k forks source link

Optimize DryRun #8857

Open vagababov opened 4 years ago

vagababov commented 4 years ago

While investigating https://github.com/knative/pkg/issues/1509 we stumbled upon problem that DryRun validation of podspecs is not really working at scale, since we're overwhelming the RateLimiter in the k8s API client that is used to invoke dry run.

Right now we moved it again to be disabled by default, but it is possible we can investigate the avenues to avoid unnecessary dry runs. E.g. avoid them when pod-spec hasn't changed in updates or completely ignore when the only update is status update.

In what area(s)?

/area API

/assign @whaught

whaught commented 4 years ago

https://github.com/knative/serving/blob/2644ad1fbed6eb753080b4789595182cfdb23563/pkg/webhook/validate_unstructured.go#L104

I believe we ignore no-op updates. That said there may still be common-cases where we can avoid dry-run on Create.

github-actions[bot] commented 4 years ago

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

vagababov commented 4 years ago

/lifecycle frozen

we can't really enable this right now

evankanderson commented 3 years ago

/triage needs-user-input

Is this still important?

evankanderson commented 3 years ago

@dprotaso

Is this still important (/remove-triage needs-user-input, /triage accepted, /help) or should we close it?

dprotaso commented 2 years ago

/triage accepted /good-first-issue

Moving to Icebox and labeling this as a good first issue.

My only opinion here is to have PodSpec dry run occur when the user has request dry-runs for our CRD types - Service and Configuration. For example using kubectl create or kubectl apply there is a --dry-run=server flag. Users shouldn't need the annotation on the Knative Service.

note: I haven't tested whether the dry-run flag works for CRs. If not an alternative could be to add support for dry-run in kn

To implement this we know from the AdmissionRequest whether it's a DryRun or not

knative-prow[bot] commented 2 years ago

@dprotaso: This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-good-first-issue command.

In response to [this](https://github.com/knative/serving/issues/8857): >/triage accepted >/good-first-issue > >Moving to Icebox and labeling this as a good first issue. > >My only opinion here is to have PodSpec dry run occur when the user has request dry-runs for our CRD types - Service and Configuration. For example using `kubectl create` or `kubectl apply` there is a `--dry-run=server` flag. Users shouldn't need the annotation on the Knative Service. > >note: I haven't tested whether the dry-run flag works for CRs. If not an alternative could be to add support for dry-run in `kn` > >To implement this we know from the [AdmissionRequest](https://pkg.go.dev/k8s.io/kubernetes/pkg/apis/admission#AdmissionRequest) whether it's a [DryRun](https://github.com/kubernetes/kubernetes/blob/ad3338546da947756e8a88aa6822e9c11e7eac22/pkg/apis/admission/types.go#L104-L108 >) or not > > > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
ashutosh887 commented 1 year ago

I would Like to work on this @dprotaso @ReToCode @vagababov Please assign

dprotaso commented 1 year ago

/assign @ashutosh887

dprotaso commented 1 year ago

@ashutosh887 are you still working on this?

dprotaso commented 1 year ago

/unassign @ashutosh887

xiangpingjiang commented 1 year ago

@dprotaso

/triage accepted /good-first-issue

Moving to Icebox and labeling this as a good first issue.

My only opinion here is to have PodSpec dry run occur when the user has request dry-runs for our CRD types - Service and Configuration. For example using kubectl create or kubectl apply there is a --dry-run=server flag. Users shouldn't need the annotation on the Knative Service.

note: I haven't tested whether the dry-run flag works for CRs. If not an alternative could be to add support for dry-run in kn

To implement this we know from the AdmissionRequest whether it's a DryRun or not

When run kubectl apply -f deployment.yaml --dry-run=server -oyaml , It will return the deployment yaml, do we also need implement this for Service and Configuration?

Priyansurout commented 1 year ago

/assign

dprotaso commented 1 year ago

@Priyansurout are you still working on this?

Priyansurout commented 1 year ago

@dprotaso no sir