Clean a healthcheck todo and register healthcheck for the shoot managedresource #16
Conversation
vpnachev
force-pushed
the
clean-healthcheck-todo
branch
from
58a1bdd to
a48adc1
Compare
gardener-robot
added
the
size/xs
gardener-robot-ci-2
added
the
reviewed/ok-to-test
gardener-robot-ci-1
added
needs/ok-to-test
vpnachev
force-pushed
the
clean-healthcheck-todo
branch
from
a48adc1 to
872d1e4
Compare
vpnachev
changed the title
gardener-robot-ci-3
added
the
reviewed/ok-to-test
gardener-robot-ci-2
removed
the
reviewed/ok-to-test
vpnachev
changed the title
gardener-robot-ci-2
added
the
reviewed/ok-to-test
|
/ready |
gardener-robot
added
reviewed/lgtm
gardener-robot
added
the
status/closed
| @@ -44,9 +44,12 @@ func RegisterHealthChecks(mgr manager.Manager, opts healthcheck.DefaultAddArgs) | |||
| ConditionType: string(gardencorev1beta1.ShootControlPlaneHealthy), | |||
| HealthCheck: general.CheckManagedResource(constants.ManagedResourceNamesSeed), | |||
| }, | |||
| { | |||
| ConditionType: string(gardencorev1beta1.ShootSystemComponentsHealthy), | |||
| HealthCheck: general.CheckManagedResource(constants.ManagedResourceNamesShoot), | |||
There was a problem hiding this comment.
Why was this added back? The shoot care controller already health-checks all the ManagedResources in the shoot control-plane which does not specify spec.class, so these MRs are already covered.
For more details, see gardener/gardener#7462.
The cleanup code was to remove this condition from the extensions, since the extensions do not need to maintain this condition anymore.
We just had to remove the cleanup code in this release, not add the health check back.
There was a problem hiding this comment.
Good question. I am not aware of all changes in g/g and cannot keep myself informed for any single change there.
From my point of view, when I am registering a healthcheck for the one managed resource, I asked myself why I am not doing the same for the other managed resource too. Looking into the documentation of https://pkg.go.dev/github.com/gardener/[email protected]/extensions/pkg/controller/healthcheck#DefaultRegistration and https://pkg.go.dev/github.com/gardener/[email protected]/extensions/pkg/controller/healthcheck#ConditionTypeToHealthCheck
I do not see any recommendation that I must not register healthchecks for class=shoot. Also, when I tested the change - I have not seen any error or warning in the controller logs that I am registering a healthcheck that I should not. That is something that you should think about how to improve because now you noticed the change, but I am sure in 2 years nobody would have noticed it.
There was a problem hiding this comment.
From the release notes of gardener v1.65,
Now by default, Gardener performs health check for all the `ManagedResource`s with `.spec.class=nil` created in the shoot namespaces. Extensions using Gardener `v1.65.0` onwards can drop the health check for the MangedResource.
It's also mentioned in https://github.com/gardener/gardener/blob/32baa826773add7c273ac185b5e15e3ef5d536c4/docs/extensions/healthcheck-library.md?plain=1#L99.
Adding the healthcheck to the extension won't break anything, but just that it is not necessary.
There was a problem hiding this comment.
After you noticed this yesterday I also found https://github.com/gardener/gardener/blob/32baa826773add7c273ac185b5e15e3ef5d536c4/docs/extensions/healthcheck-library.md?plain=1#L99 and to be honest even if I was aware about it, I would have registered again the health check as it does not provide any guidance, just claims that Gardener does the health check but there is no statement that the extension controller should/must not do the same health check.
What this PR does / why we need it:
Clean a healthcheck todo and register healthcheck for the shoot managedresource
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: