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 the ability to set watch timeouts. #392

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

barney-s
Copy link
Contributor

@barney-s barney-s commented Jun 6, 2024

Allow setting shorter watch timeout for the first time alone.

What this PR does / why we need it:
We have seen in some evnironments (where requests are tunnelled) setting up the (first) watch is blocking if there are no resources available to watch. This causes the apply loop to be blocked when setting up watches before apply.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 6, 2024
@barney-s
Copy link
Contributor Author

barney-s commented Jun 6, 2024

The test that is failing here is passing when i run it locally. Not sure what is happening here. !!
@justinsb

❯ go test -count=1 -v ./...
?       sigs.k8s.io/kubebuilder-declarative-pattern/applylib    [no test files]

?       sigs.k8s.io/kubebuilder-declarative-pattern/applylib/forked/github.com/kubernetes/kubectl/pkg/cmd/apply [no test files]
?       sigs.k8s.io/kubebuilder-declarative-pattern/applylib/forked/k8s.io/apimachinery/pkg/util/sets   [no test files]
?       sigs.k8s.io/kubebuilder-declarative-pattern/applylib/testutils  [no test files]
=== RUN   TestApplySet
I0606 01:47:38.192436 1169995 apiserver.go:87] kubeapiserver request: GET /api/v1
I0606 01:47:38.193302 1169995 apiserver.go:87] kubeapiserver request: POST /api/v1/namespaces/default/configmaps
I0606 01:47:38.259500 1169995 apiserver.go:87] kubeapiserver request: GET /api?timeout=32s
I0606 01:47:38.260049 1169995 apiserver.go:87] kubeapiserver request: GET /apis?timeout=32s
I0606 01:47:38.260730 1169995 apiserver.go:87] kubeapiserver request: GET /apis/v1?timeout=32s
W0606 01:47:38.260759 1169995 apiserver.go:310] 404 for GET /apis/v1?timeout=32s tokens=[]string{"apis", "v1"}
I0606 01:47:38.261379 1169995 apiserver.go:87] kubeapiserver request: GET /api/v1?timeout=32s
E0606 01:47:38.261563 1169995 memcache.go:287] couldn't get resource list for /v1: the server could not find the requested resource
I0606 01:47:38.263659 1169995 apiserver.go:87] kubeapiserver request: PUT /api/v1/namespaces/default/configmaps/test
I0606 01:47:38.263708 1169995 putresource.go:60] put request "{\"apiVersion\":\"v1\",\"kind\":\"ConfigMap\",\"metadata\":{\"annotations\":{\"applyset.kubernetes.io/additional-namespaces\":\"test-applyset\",\"applyset.kubernetes.io/contains-group-kinds\":\"ConfigMap,Namespace\",\"applyset.kubernetes.io/tooling\":\"ConfigMap/\"},\"labels\":{\"applyset.kubernetes.io/id\":\"applyset-XYWvxXDUlCqMdjmmY1arThcdGiF0cvBW6sAfSMWYUdE-v1\"},\"name\":\"test\",\"namespace\":\"default\"}}\n"
I0606 01:47:38.263766 1169995 putresource.go:88] patch did not change object
I0606 01:47:38.264225 1169995 apiserver.go:87] kubeapiserver request: PATCH /api/v1/namespaces/test-applyset?fieldManager=kops&force=true
I0606 01:47:38.264277 1169995 patchresource.go:66] patch request "{\"apiVersion\":\"v1\",\"kind\":\"Namespace\",\"metadata\":{\"labels\":{\"applyset.kubernetes.io/part-of\":\"applyset-XYWvxXDUlCqMdjmmY1arThcdGiF0cvBW6sAfSMWYUdE-v1\"},\"name\":\"test-applyset\"}}"
I0606 01:47:38.264922 1169995 apiserver.go:87] kubeapiserver request: PATCH /api/v1/namespaces/test-applyset/configmaps/foo?fieldManager=kops&force=true
I0606 01:47:38.265001 1169995 patchresource.go:66] patch request "{\"apiVersion\":\"v1\",\"data\":{\"foo\":\"bar\"},\"kind\":\"ConfigMap\",\"metadata\":{\"labels\":{\"applyset.kubernetes.io/part-of\":\"applyset-XYWvxXDUlCqMdjmmY1arThcdGiF0cvBW6sAfSMWYUdE-v1\"},\"name\":\"foo\",\"namespace\":\"test-applyset\"}}"
I0606 01:47:38.265565 1169995 apiserver.go:87] kubeapiserver request: PUT /api/v1/namespaces/default/configmaps/test
I0606 01:47:38.265611 1169995 putresource.go:60] put request "{\"apiVersion\":\"v1\",\"kind\":\"ConfigMap\",\"metadata\":{\"annotations\":{\"applyset.kubernetes.io/additional-namespaces\":\"test-applyset\",\"applyset.kubernetes.io/contains-group-kinds\":\"ConfigMap,Namespace\",\"applyset.kubernetes.io/tooling\":\"ConfigMap/\"},\"creationTimestamp\":\"2022-01-01T00:00:00Z\",\"labels\":{\"applyset.kubernetes.io/id\":\"applyset-XYWvxXDUlCqMdjmmY1arThcdGiF0cvBW6sAfSMWYUdE-v1\"},\"name\":\"test\",\"namespace\":\"default\",\"resourceVersion\":\"1\",\"uid\":\"00000000-0000-0000-0000-000000000001\"}}\n"
I0606 01:47:38.265682 1169995 putresource.go:88] patch did not change object
I0606 01:47:38.266104 1169995 apiserver.go:87] kubeapiserver request: GET /api/v1/namespaces/test-applyset
I0606 01:47:38.266716 1169995 apiserver.go:87] kubeapiserver request: GET /api/v1/namespaces/test-applyset/configmaps/foo
--- PASS: TestApplySet (0.08s)
PASS
ok      sigs.k8s.io/kubebuilder-declarative-pattern/applylib/applyset   0.250s
❯ 

@barney-s
Copy link
Contributor Author

barney-s commented Jun 6, 2024

/assign @justinsb
/assign @tomasaschan

@barney-s
Copy link
Contributor Author

barney-s commented Jun 6, 2024

/test pull-declarative-test

const WatchDelay = 30 * time.Second
var (
// WatchActivityTimeout sets a timeout for a Watch activity under normal operation
WatchActivityTimeout = 300 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

We could have a method to enable the watch activity timeout, but I actually think it's a really good defensive feature so I think we should enable it by default anyway. (If anyone disagrees, please comment!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

Allow setting shorter watch timeout for the firt time alone.

What this PR does / why we need it:
We have seen in some evnironments (where requests are tunnelled) setting up the (first) watch is blocking if there are no resources available to watch. This causes the apply loop to be blocked when setting up watches before apply.
@justinsb
Copy link
Contributor

Thanks @barney-s

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: barney-s, justinsb

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2024
@k8s-ci-robot k8s-ci-robot merged commit 53ff542 into kubernetes-sigs:master Jun 14, 2024
6 checks passed
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", 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.

None yet

4 participants