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

Close the WatchStart before starting the watch. It was anyways closed… #376

Closed
wants to merge 0 commits into from

Conversation

barney-s
Copy link
Contributor

@barney-s barney-s commented Jan 27, 2024

Setting up Watch() sometimes results in Watch() being a blocking call. Add a cancellable context that times out sooner when there is no activity in the Apply() path. Otherwise it timesout in 5mins. Cancellable context is better than watch options.Timeout since watch timeout happens even if there is activity on the watch channel.

What this PR does / why we need it:
In BeforeApply watch setup case, it blocks the Apply loop if the API server doesnt have any watch events for the first object being applied.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Additional documentation:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 27, 2024
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 27, 2024
@atoato88
Copy link
Contributor

atoato88 commented Feb 5, 2024

/assign @justinsb

@@ -35,8 +36,17 @@ import (
"sigs.k8s.io/controller-runtime/pkg/log"
)

// WatchDelay is the time between a Watch being dropped and attempting to resume it
const WatchDelay = 30 * time.Second
const (
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two const sections at here.
What is the reason to be splited to?

@atoato88
Copy link
Contributor

atoato88 commented Feb 5, 2024

Current title of this PR doesn't indicate the current contents, I think.
Could you update the PR title?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

The full list of commands accepted by this bot can be found 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 size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants