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.

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

MachineSet controller deletion requests don't need to run in a WaitGroup #2989

Closed
vincepri opened this issue Apr 30, 2020 · 4 comments · Fixed by #3092 or #3089
Closed

MachineSet controller deletion requests don't need to run in a WaitGroup #2989

vincepri opened this issue Apr 30, 2020 · 4 comments · Fixed by #3092 or #3089
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/backlog Higher priority than priority/awaiting-more-evidence.
Milestone

Comments

@vincepri
Copy link
Member

The MachineSet controller currently uses a WaitGroup when it scales down:

var wg sync.WaitGroup
wg.Add(diff)
for _, machine := range machinesToDelete {
go func(targetMachine *clusterv1.Machine) {
defer wg.Done()
err := r.Client.Delete(context.Background(), targetMachine)
if err != nil {
logger.Error(err, "Unable to delete Machine", "machine", targetMachine.Name)
r.recorder.Eventf(ms, corev1.EventTypeWarning, "FailedDelete", "Failed to delete machine %q: %v", targetMachine.Name, err)
errCh <- err
}
logger.Info("Deleted machine", "machine", targetMachine.Name)
r.recorder.Eventf(ms, corev1.EventTypeNormal, "SuccessfulDelete", "Deleted machine %q", targetMachine.Name)
}(machine)
}
wg.Wait()
close(errCh)

This was added around v1alpha1 timeframe because Delete at that time was blocking. Today, these Delete calls can be done sequentially because they return right away and don't have to wait for the delete to finish.

/kind cleanup
/milestone v0.3.x
/priority backlog
/help

@k8s-ci-robot k8s-ci-robot added this to the v0.3.x milestone Apr 30, 2020
@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/backlog Higher priority than priority/awaiting-more-evidence. labels Apr 30, 2020
@k8s-ci-robot
Copy link
Contributor

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

The MachineSet controller currently uses a WaitGroup when it scales down:

var wg sync.WaitGroup
wg.Add(diff)
for _, machine := range machinesToDelete {
go func(targetMachine *clusterv1.Machine) {
defer wg.Done()
err := r.Client.Delete(context.Background(), targetMachine)
if err != nil {
logger.Error(err, "Unable to delete Machine", "machine", targetMachine.Name)
r.recorder.Eventf(ms, corev1.EventTypeWarning, "FailedDelete", "Failed to delete machine %q: %v", targetMachine.Name, err)
errCh <- err
}
logger.Info("Deleted machine", "machine", targetMachine.Name)
r.recorder.Eventf(ms, corev1.EventTypeNormal, "SuccessfulDelete", "Deleted machine %q", targetMachine.Name)
}(machine)
}
wg.Wait()
close(errCh)

This was added around v1alpha1 timeframe because Delete at that time was blocking. Today, these Delete calls can be done sequentially because they return right away and don't have to wait for the delete to finish.

/kind cleanup
/milestone v0.3.x
/priority backlog
/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Apr 30, 2020
@vincepri vincepri changed the title MachineSet controller deletion request don't need to run in a WaitGroup MachineSet controller deletion requests don't need to run in a WaitGroup Apr 30, 2020
@akhil-rane
Copy link
Contributor

@vincepri Can I take this up?

@vincepri
Copy link
Member Author

go for it @akhil-rane !

@akhil-rane
Copy link
Contributor

/assign

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
3 participants