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

Jump to bottom

Bugfix: Drain machines with only a valid NodeName #480

Merged
merged 4 commits into from
Jun 30, 2020
Merged

Bugfix: Drain machines with only a valid NodeName #480

merged 4 commits into from
Jun 30, 2020

Conversation

prashanth26
Copy link
Contributor

@prashanth26 prashanth26 commented Jun 26, 2020 •
edited
Loading

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #465

Special notes for your reviewer:

Release note:

Bugfix: Drain machines with only a valid node (name)

@prashanth26 prashanth26 requested review from ggaurav10 and a team as code owners June 26, 2020 08:22
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 26, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 26, 2020
@prashanth26 prashanth26 force-pushed the bugfix/drain-nodeless-machine branch from a5f18a5 to 2da2e4f Compare June 26, 2020 08:24
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 26, 2020
@prashanth26 prashanth26 force-pushed the bugfix/drain-nodeless-machine branch from 2da2e4f to 3484d7c Compare June 26, 2020 08:43
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 26, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 26, 2020
@prashanth26
Copy link
Contributor Author

prashanth26 commented Jun 26, 2020 •
edited
Loading

/kind regression
/priority critical

@gardener-robot
Copy link

@prashanth26 Command /king is not known.

@gardener-robot gardener-robot added priority/critical Needs to be resolved soon, because it impacts users negatively kind/regression Bug that hit us already in the past and that is reappearing/requires a proper solution labels Jun 26, 2020
hardikdr
hardikdr reviewed Jun 28, 2020
View reviewed changes
pkg/controller/machine.go
@@ -599,7 +599,9 @@ func (c *controller) machineDelete(machine *v1alpha1.Machine, driver driver.Driv
}
}

if machineID != "" {
if machineID != "" && nodeName != "" {
// Begin drain logic only when the nodeName & providerID exist's for the machine
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Begin drain logic only when the nodeName & providerID exist's for the machine
// Begin drain logic only when the nodeName & providerID exists for the machine

Copy link
Member

Choose a reason for hiding this comment

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

Do we need this change also in OOT?

hardikdr
hardikdr reviewed Jun 28, 2020
View reviewed changes
Copy link
Member

@hardikdr hardikdr left a comment

Choose a reason for hiding this comment

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

Looks good, just a minor comment.

pkg/controller/machine.go
@@ -599,7 +599,9 @@ func (c *controller) machineDelete(machine *v1alpha1.Machine, driver driver.Driv
}
}

if machineID != "" {
if machineID != "" && nodeName != "" {
// Begin drain logic only when the nodeName & providerID exist's for the machine
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this change also in OOT?

@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 28, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 28, 2020
@prashanth26 prashanth26 force-pushed the bugfix/drain-nodeless-machine branch from 893ee56 to 66fee5c Compare June 28, 2020 17:09
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 28, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 28, 2020
@prashanth26
Copy link
Contributor Author

Do we need this change also in OOT?

Nice catch. Even I thought about it, although I think the OOT will not run into such a situation. I have added this check just to be safe. Here - 66fee5c

Also if all looks good, please squash and merge it. Thanks.

@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 30, 2020
@prashanth26
Copy link
Contributor Author

Hi @ggaurav10 ,

Nice catch with this. Made the suggested change here - 0a7d3b5

@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 30, 2020
@prashanth26 prashanth26 force-pushed the bugfix/drain-nodeless-machine branch from 0a7d3b5 to b2ea619 Compare June 30, 2020 06:54
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 30, 2020
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 30, 2020
@hardikdr
Copy link
Member

/lgtm

@gardener-robot gardener-robot added the reviewed/lgtm Has approval for merging label Jun 30, 2020
@hardikdr hardikdr merged commit 8881b03 into gardener:master Jun 30, 2020
@prashanth26 prashanth26 deleted the bugfix/drain-nodeless-machine branch May 2, 2021 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hardikdr hardikdr hardikdr left review comments

@ggaurav10 ggaurav10 ggaurav10 approved these changes

Assignees

@hardikdr hardikdr

Labels
kind/regression Bug that hit us already in the past and that is reappearing/requires a proper solution needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) priority/critical Needs to be resolved soon, because it impacts users negatively reviewed/lgtm Has approval for merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MCM should not try to drain machines that have not joined the cluster
7 participants
@prashanth26 @gardener-robot @hardikdr @ggaurav10 @gardener-robot-ci-1 @gardener-robot-ci-2 @gardener-robot-ci-3