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.

Sign up for GitHub

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

Clean a healthcheck todo and register healthcheck for the shoot managedresource #16

Merged
merged 2 commits into from
Jun 2, 2023
Merged

Clean a healthcheck todo and register healthcheck for the shoot managedresource #16

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
872d1e4
Remove temporary code cleaning MR heathcheck
vpnachev Jun 2, 2023
a48adc1
Register healthcheck for the shoot's managedresource
vpnachev Jun 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions pkg/controller/healthcheck/add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

},
},
// TODO(shafeeqes): Remove this condition in a future version.
sets.New(gardencorev1beta1.ShootSystemComponentsHealthy),
sets.New[gardencorev1beta1.ConditionType](),
)
}

Expand Down