Skip to content
This repository has been archived by the owner on Nov 9, 2022. It is now read-only.

Prevent context deadlines exceedances #111

Merged
merged 2 commits into from
Feb 17, 2021

Conversation

rfranzke
Copy link
Contributor

How to categorize this PR?

/area scalability
/kind enhancement
/priority normal

What this PR does / why we need it:
@timebertt suggested to use the timeout context introduced with #102 for the reconciliations/health checks themselves, but not for the status updates. This PR is implementing the suggestion.

Along the way, I found that the QPS/burst settings for the client are only applied for the uncached client. However, Gardener disabled the cached client for GRMs deployed as part of the shoot cluster, so the default QPS/burst settings apply which are very low. I've adapted it and do now apply the settings for both cases.

Special notes for your reviewer:
/invite @timebertt @timuthy

Release note:

The client QPS and burst settings do now also apply for the uncached client.

@rfranzke rfranzke requested a review from a team as a code owner February 17, 2021 12:53
@gardener-robot gardener-robot added area/scalability Scalability related kind/enhancement Enhancement, improvement, extension needs/review Needs review priority/normal size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Feb 17, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 17, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 17, 2021
@timuthy
Copy link
Contributor

timuthy commented Feb 17, 2021

@timebertt suggested to use the timeout context introduced with #102 for the reconciliations/health checks themselves, but not for the status updates. This PR is implementing the suggestion.

Have we seen timeout problems due to conflicts or what's the motivation? Shouldn't we then rather switch to patching the conditions?

@rfranzke
Copy link
Contributor Author

Have we seen timeout problems due to conflicts or what's the motivation?

I think the motivation is to always propagate a reconciliation result to the status even if the reconciliation itself timed out (for whatever reason). If the same context is used then the status update call will not succeed if the context timed out.

Shouldn't we then rather switch to patching the conditions?

Probably yes, but I guess this can be done separately in the future / is not really related to this PR.

Copy link
Contributor

@timuthy timuthy left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

@timebertt timebertt left a comment

Choose a reason for hiding this comment

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

Thank you!
/lgtm

@timebertt timebertt merged commit 08d1cb9 into gardener-attic:master Feb 17, 2021
@rfranzke rfranzke deleted the enh/timeout branch February 17, 2021 15:27
@gardener-robot gardener-robot added priority/3 Priority (lower number equals higher priority) and removed priority/3 Priority (lower number equals higher priority) labels Mar 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/scalability Scalability related kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/s Size of pull request is small (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants