Gardener: shoot care health checks sometimes broken

[rfranzke]

How to categorize this issue?

/area ops-productivity auto-scaling
/kind enhancement
/priority normal
/exp expert
/platform all
/topology seed

What would you like to be added:
I don't remember if it only happened when the CachedRuntimeClient feature gate was enabled, but I observed that the shoot care health checks break when a v1.8 gardenlet first starts up. See this:

After a minute or so it self-heals again, i.e., the condition statuses can be correctly computed and are also reported correctly. However, we should avoid this flickering to prevent alerts and end-user confusion.

See also the following logs of gardenlet:

Could not initialize Shoot client for health check: error creating new ClientSet for key "garden-foo/bar": failed to get Shoot object "garden-foo/bar": rate: Wait(n=1) would exceed context deadline
Could not initialize Shoot client for health check: error creating new ClientSet for key "garden-baz/foo": failed to get Shoot object "garden-baz/foo": context deadline exceeded

/cc @timebertt

Why is this needed:
Reduce ops effort and improve overall quality/robustness, smoother roll outs.

[timebertt]

/assign

[timebertt]

After a short investigation, it seems to me that this is not related to the Shoot care controller specifically, but rather to the gardenlet's gardenClientConnection settings in general.
So to circumvent this issue, one could increase those qps/burst settings. But as you said, it will also auto-resolve after a short period of time.

[timebertt]

As discussed, let's add some jitter duration when enqueueing the Shoots in the care controller so we don't try to check on all of them at the same time. This should already help in limiting the effects on the rate limiter.

[rfranzke]

I have to
/reopen
this one as it's still happening, sorry.

[timebertt]

Quick update on this:
My current hypothesis is, that the gardenlet is rate limited pretty quick on startup in large Seeds because of the federated seed controller, as it statically enqueues a lot of resources on startup which each cause a few API calls to the garden cluster.
This grows exponentially with the number of Shoots/Seed, so we can easily hit the rate limit for the garden connection on large Seed clusters.
Rough calculation would be, that on a Seed with 120 Shoots this controller alone can cause up to 12.000 request on startup (depending on several factors).

I will try to get more evidence on this hypothesis and try to think about a fix.

[timebertt]

Unfortunately we also observed this during runtime of the gardenlet, where it was not rolled/restarted or something similar.
In this case, the problem was not rate limiting while talking to the Garden cluster but rather that the Seed's API was server under high load and was scaled at the time of the issue.
This resulted in the same symptoms like described above, although it's a different root cause.

Some logs of the gardenlet (errors coming from different shoots)

time="2020-09-04T07:48:44Z" level=error msg="Could not initialize Shoot client for constraints check: failed to calculate new hash for ClientSet: context deadline exceeded" shoot=garden-some-project/some-shoot
time="2020-09-04T07:48:44Z" level=error msg="Failed to execute control plane health checks: context deadline exceeded" shoot=garden-some-project/some-shoot
time="2020-09-04T07:48:44Z" level=error msg="Failed to execute control plane health checks: context deadline exceeded" shoot=garden-some-project/some-shoot
time="2020-09-04T07:48:44Z" level=error msg="Could not initialize Shoot client for health check: failed to calculate new hash for ClientSet: context deadline exceeded" shoot=garden-some-project/some-shoot
time="2020-09-04T07:48:44Z" level=error msg="Error during seed garbage collection: context canceled" shoot=garden-some-project/some-shoot
time="2020-09-04T07:48:44Z" level=error msg="Failed to execute control plane health checks: context deadline exceeded" shoot=garden-some-project/some-shoot
time="2020-09-04T07:48:44Z" level=error msg="Failed to execute control plane health checks: context deadline exceeded" shoot=garden-some-project/some-shoot
time="2020-09-04T07:48:44Z" level=error msg="Could not initialize Shoot client for garbage collection of shoot garden-some-project/some-shoot
time="2020-09-04T07:48:44Z" level=error msg="Failed to execute control plane health checks: context deadline exceeded" shoot=garden-some-project/some-shoot

/title Shoot care health checks sometimes broken

[timebertt]

Though, the common denominator of both problems is, that the Shoot health checks timeout (with context deadline exceeded) (either while talking to the garden cluster or the seed cluster), which only occurs because we have a context timeout of shootCare.syncPeriod since #2635 (relevant change).
Before this change, the health checks just weren't cancelled and the conditions were not updated, although the problem was already present, but just not visible.

I think we should keep the timeout for the care controller, but we might think about not setting the shoot conditions to Unknown in case we hit the timeout (probably rather Progressing?). This way, the error wouldn't be visible immediately.
@rfranzke @timuthy WDYT?

[timebertt]

Other improvements, that we should make, regardless of the changes to the care controller, to fix the root causes:

• [x] Use client.Patch instead of CreateOrUpdate for syncing the extensions' state to the ShootState object here similar to #2798 (@plkokanov)
• [x] Enqueue the extension kinds in the ControllerInstallationRequired controller rather than the objects themselves, as the reconciler ignores the request anyways (#2816)

[rfranzke]

Hm, not sure, I'm not so much in favour of hiding this problem (by setting the status to Progressing) - let's rather work on better observability/metrics to understand where this problem is coming from and fix its root. Even if you suggest this only for the mean time I would rather vote against it.

[timuthy]

Enqueue the extension kinds in the ControllerInstallationRequired controller rather than the objects themselves, as the the reconciler ignores the request anyways (I can take this)

This sounds interesting but what do you mean by enqueue extension kinds? How can you do that and how does it help to improve the situation?

[plkokanov]

Yep I can work on replacing CreateOrUpdate with Patch for the extensions state as well. I think we initially went with CreateOrUpdate to create the ShootState for already existing shoots as it would not exist for those shoots when this feature was introduced.

[timebertt]

let's rather work on better observability/metrics to understand where this problem is coming from and fix its root

Agreed. I would definitely like to fix the root causes, too.
So do I understand correctly, that you would also vote for keeping the timeout in the care controller?

I'm not so much in favour of hiding this problem

The proposal was rather just an idea. What I also meant was, that we might use the condition thresholds to not "hide" the issue permanently, but to set it to Progressing for the configured amount of time to have a bit smother rollout.

[timebertt]

Enqueue the extension kinds in the ControllerInstallationRequired controller rather than the objects themselves, as the the reconciler ignores the request anyways (I can take this)

This sounds interesting but what do you mean by enqueue extension kinds? How can you do that and how does it help to improve the situation?

I would probably do this by setting the namespace and name of the object to enqueue to a constant value, because there is no way of enqueueing only a kind directly (as you correctly figured).
This would help, because we would then only do the computation of the required installations for an extension kind only once (which is independent of the specific extensions object) rather than doing the same computation for every single extension object over and over again.

[timuthy]

This would help, because we would then only do the computation of the required installations for an extension kind only once (which is independent of the specific extensions object) rather than doing the same computation for every single extension object over and over again.

IIUC, we need to recalculate with every change of the extension object and this is done by intention. For example, an installation may only not be required any more if all extensions of all kind/type combinations are gone. But let's have a direct discussion about the details. In general, I'm happy to see simplifications in this area :)

[timebertt]

2816 is merged

/unassign

[timebertt]

Yep I can work on replacing CreateOrUpdate with Patch for the extensions state as well. I think we initially went with CreateOrUpdate to create the ShootState for already existing shoots as it would not exist for those shoots when this feature was introduced.

/ping @plkokanov

[gardener-robot]

@plkokanov

Message

Yep I can work on replacing CreateOrUpdate with Patch for the extensions state as well. I think we initially went with CreateOrUpdate to create the ShootState for already existing shoots as it would not exist for those shoots when this feature was introduced.

/ping @plkokanov

[plkokanov]

Will start on it next week. One thing that worries me though is the keys for merging entries during a patch -> for gardener resources it is easy as there the names are unique. However, for extensions it's a unique combination of name, purpose and kind.

[rfranzke]

@plkokanov what's the progress?

[plkokanov]

@rfranzke still working on it as I was in vacation.