feat: delete clusters when shoot is deleted (#45)

[jellonek]

Closes #45

[Copilot]

Pull request overview

This PR implements cleanup of Greenhouse Cluster resources when the corresponding Gardener Shoot is deleted (404 on previously existing Shoot), and documents the new behavior/events.

Changes:

• Add logic in the Shoot controller to delete an existing Greenhouse Cluster when the Shoot can no longer be fetched due to NotFound.
• Emit a new ClusterDeleted event and document it in the README.
• Add a test intended to validate cluster deletion behavior on Shoot removal.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
README.md	Updates documentation to reflect cluster cleanup behavior and adds the ClusterDeleted event.
controller/shoot/shoot_controller.go	Implements deletion of the Greenhouse Cluster when the Shoot is deleted and emits a ClusterDeleted event.
controller/shoot/shoot_controller_test.go	Adds a new test case intended to cover Shoot removal leading to Cluster cleanup.

Comments suppressed due to low confidence (1)

controller/shoot/shoot_controller.go:120

• existingClusterFound is computed incorrectly and will be false both when the Cluster exists (err == nil) and when it is NotFound, but true for non-NotFound errors. This prevents cluster deletion on Shoot deletion and can also cause an attempted delete of an empty Cluster object when the initial Get fails for transient reasons.

Handle the Cluster Get result explicitly: set existingClusterFound = true only when err == nil, return early on non-NotFound errors, and keep it false on NotFound.

	existingClusterFound := false
	err := r.GreenhouseClient.Get(ctx, client.ObjectKey{Name: req.Name, Namespace: r.CareInstruction.Namespace}, &existingCluster)
	if err == nil {
		// Cluster exists - check ownership
		if ownerLabel, hasLabel := existingCluster.Labels[v1alpha1.CareInstructionLabel]; hasLabel && ownerLabel != r.CareInstruction.Name {

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

controller/shoot/shoot_controller.go:150

• Shoot deletion cleanup in Reconcile only runs when the controller receives a reconcile request. With the current watch predicates, delete events for a Shoot whose labels no longer match the CareInstruction selector will likely be filtered out (SetupWithManager uses predicate.LabelSelectorPredicate(...)), so the Cluster may never be deleted even though the Shoot is gone (common flow: labels change -> no longer matched -> later Shoot deleted). Consider ensuring delete events always enqueue reconciliation (e.g., custom predicate that applies the label selector for create/update but returns true for delete) or moving the label-selection filtering into Reconcile so deletes are still handled.

	if err := r.GardenClient.Get(ctx, client.ObjectKey{Namespace: req.Namespace, Name: req.Name}, &shoot); err != nil {
		r.Info("unable to fetch Shoot")
		if client.IgnoreNotFound(err) == nil {
			// Shoot was deleted
			if existingClusterFound && hasLabel && ownerLabel == r.CareInstruction.Name {
				if err := r.RequestClusterDeletion(ctx, existingCluster); err != nil {
					return ctrl.Result{}, err
				}
			}
			r.emitEvent(r.CareInstruction, corev1.EventTypeNormal, "ShootDeleted",
				fmt.Sprintf("Shoot %s/%s was deleted", req.Namespace, req.Name))
		}
		return ctrl.Result{}, client.IgnoreNotFound(err)

[uwe-mayer]

Some naming comments and nit pics. No blockers.
🚀

[uwe-mayer]

Let's log every error here, not just isnotFound?

[jellonek]

Done with improved log message.

[uwe-mayer]

OK, we also need an error event. On a more general note:
I originally opted for emitting events only on function returns for failures and info logging all happy paths within a function. E.g. https://github.com/cloudoperators/shoot-grafter/blob/main/controller/careinstruction/careinstruction_controller.go#L125-L135
Maybe we can consolidate here?

[jellonek]

No problem. I can add even emission there.

If I understand correctly the pointed code - it is catching errors from the time of setting up the managers for shoots, but not catching errors from reconciliation of these shoots, so it's not the single place from which we could be always emitting such events.

BTW. @abhijith-darshan was pointing that we should always log when we are emitting events, as events are quite quickly disappearing.

[uwe-mayer]

OK. let's do it like this:
let's log and emit event on the calling function depending on the error returned. WDYT?

[jellonek]

The easiest way of consolidating event creation on any kind of failure would be to rename Reconcile method to some internal name, then call it from new Reconcile method done as a wrapper around calling old one which would check for error, emit event or error, then return what was returned from original method.
Is that something about what you are asking?

The issue there would be - we are loosing a detailed context about the event as it had to be based only on current CareInstruction and an error content, but - tbh. we are not relying on that context right now too much and we are not adding too much custom event fields.

[jellonek]

Event emission added.

[uwe-mayer]

Sorry, I meant just on the RequestClusterDeletion call in line 144

[uwe-mayer]

I would say, we let this func just return the err and log and emit event on the calling function.
Both success and failure.
That should make sense and consolidate?
WDYT?

[jellonek]

Makes sense. I'll refactor the code to that shape in few minutes.

[jellonek]

Done. The disadvantage of doing events and logging in main reconcile function is that this function was already long and now it contains yet more lines and nested ifs.

It should not be a problem for now, as that should be handled during #70.

[github-actions]

Merging this branch will decrease overall coverage

Impacted Packages	Coverage Δ	🤖
shoot-grafter/controller/shoot	78.26% (-0.83%)	👎

---

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
shoot-grafter/controller/shoot/shoot_controller.go	82.52% (-2.10%)	143 (+13)	118 (+8)	25 (+5)	👎

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

• shoot-grafter/controller/shoot/shoot_controller_fake_client_test.go
• shoot-grafter/controller/shoot/shoot_controller_test.go