Skip to content

Conversation

ApsTomar
Copy link
Contributor

What issue type does this pull request address? (keep at least one, remove the others)
/kind bugfix

What does this pull request do? Which issues does it resolve? (use resolves #<issue_number> if possible)
resolves ENG-7242

Please provide a short message that should be published in the vcluster release notes
Fixed an issue where coreDNS components don't get deleted during vcluster delete operation

What else do we need to know?

@ApsTomar ApsTomar requested review from hidalgopl and a team as code owners June 25, 2025 14:34
@ApsTomar ApsTomar requested review from a team and FabianKramm as code owners June 26, 2025 12:47
hidalgopl
hidalgopl previously approved these changes Jun 26, 2025
}

var errs []error
for _, deleteFn := range deleteFuncs {
Copy link
Member

@FabianKramm FabianKramm Jun 26, 2025

Choose a reason for hiding this comment

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

I think we can simplify this, we can just do

 err := client.AppsV1().Deployments(namespace).DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{LabelSelector: labelSelector})
if err != nil {
  errs = append(errs, err)
}

 err = client.CoreV1().Pods(namespace).DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{LabelSelector: labelSelector})
if err != nil {
  errs = append(errs, err)
}
			services, err := client.CoreV1().Services(namespace).List(ctx, metav1.ListOptions{LabelSelector: labelSelector})
			if err != nil {
				return err
			}
			if len(services.Items) != 0 {
				for _, svc := range services.Items {
					err = client.CoreV1().Services(namespace).Delete(ctx, svc.Name, metav1.DeleteOptions{})
					if err != nil {
  errs = append(errs, err)
					}
				}

I don't see the reason why we need an array of functions here to iterate over, just makes the code harder to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FabianKramm Made the suggested changes, please take a look.

@ApsTomar ApsTomar force-pushed the delete-coredns-during-vcluster-delete-op branch from a3cdc0a to b71f46b Compare June 26, 2025 14:12
address review comments
@ApsTomar ApsTomar force-pushed the delete-coredns-during-vcluster-delete-op branch from b71f46b to 4ce4ff7 Compare June 26, 2025 14:16
@FabianKramm FabianKramm merged commit afc68e2 into loft-sh:main Jun 27, 2025
33 checks passed
Ravio1i pushed a commit to Ravio1i/vcluster that referenced this pull request Jul 15, 2025
* delete coredns components during vcluster delete op

* address review comments

address review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants