feat: delete clusters when shoot is deleted (#45)#69
Open
jellonek wants to merge 1 commit into
Open
Conversation
Signed-off-by: Piotr Skamruk <piotr.skamruk@gmail.com>
Contributor
There was a problem hiding this comment.
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
Clusterwhen the Shoot can no longer be fetched due to NotFound. - Emit a new
ClusterDeletedevent 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
existingClusterFoundis computed incorrectly and will befalseboth when the Cluster exists (err == nil) and when it is NotFound, buttruefor 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.
Comment on lines
134
to
+138
| if client.IgnoreNotFound(err) == nil { | ||
| // Shoot was deleted | ||
| if existingClusterFound { | ||
| if err := r.EnsureClusterDeleted(ctx, existingCluster); err != nil { | ||
| return ctrl.Result{}, err |
Comment on lines
+1049
to
+1069
| Expect(test.GardenK8sClient.Create(test.Ctx, shoot)).To(Succeed(), "should create Shoot resource") | ||
| shoot.Status = gardenerv1beta1.ShootStatus{ | ||
| AdvertisedAddresses: []gardenerv1beta1.ShootAdvertisedAddress{ | ||
| { | ||
| Name: "external", | ||
| URL: "https://api-server.test-shoot-ann.example.com", | ||
| }, | ||
| }, | ||
| } | ||
| Expect(test.GardenK8sClient.Status().Update(test.Ctx, shoot)).To(Succeed(), "should update Shoot status") | ||
|
|
||
| Expect(test.GardenK8sClient.Delete(test.Ctx, shoot)).To(Succeed(), "should remove Shoot resource") | ||
|
|
||
| Eventually(func(g Gomega) bool { | ||
| var existingCluster greenhousev1alpha1.Cluster | ||
| if err := test.K8sClient.Get(test.Ctx, client.ObjectKeyFromObject(shoot), &existingCluster); err != nil { | ||
| return client.IgnoreNotFound(err) == nil | ||
| } | ||
| return false | ||
| }).Should(BeTrue(), "should eventually notice removal of Cluster resource") | ||
|
|
| 7. **Maintains synchronization**: Keeps Greenhouse Cluster resources in sync with their corresponding Shoots | ||
|
|
||
| shoot-grafter currently only creates clusters matching Shoots but does not automatically clean up clusters when Shoot labels change or Shoots are deleted. Manual cleanup of Greenhouse Cluster resources is required in these scenarios. | ||
| shoot-grafter currently does not automatically clean up clusters when Shoot labels change. Manual cleanup of Greenhouse Cluster resources is required in such scenario. |
| - Generates Greenhouse Cluster resources with appropriate labels | ||
| - Optionally configures OIDC authentication on Shoot clusters for Greenhouse access. Also see respective [Greenhouse docs](https://cloudoperators.github.io/greenhouse/docs/user-guides/cluster/oidc_connectivity/) and [Gardener docs](https://gardener.cloud/docs/guides/administer-shoots/oidc-login/#configure-the-shoot-cluster) | ||
| - Optionally configures RBAC on the Shoot cluster for Greenhouse access | ||
| - Cleans up Greenhouse cluster on Gardener Shoot cluster removal |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #45