-
Notifications
You must be signed in to change notification settings - Fork 117
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
Avoid the deletion of the machines in CrashLoopBackoff state by the safety controller #589
Conversation
Hi @AxiomSamarth , We would also need this fix and test on the OOT code path here - https://github.com/gardener/machine-controller-manager/blob/master/pkg/util/provider/machinecontroller/machine_safety.go#L222. |
Sure, @prashanth26 I will make the changes and update the PR. Thank you :) |
@amshuman-kr @prashanth26 For writing this UT, I had to introduce a new method Add() in the driver interface. Should it be retained or now that we understand how to handle the scenario, should we remove the Add() method from the interface and of course driver implementations? I also see a similar thing to do in the out of tree code as well. I will have to add a new method in the interface. Am I doing it correctly or is there a different approach? |
@AxiomSamarth Thanks for the unit test!
I don't think it is a good idea to enhance the You can enhance just the
You can use pretty much the same approach as I mentioned above for OOT as well. Or you can chose to even generate mocks for the OOT interface and simulate the responses that way. I am fine either way. |
I think the mock approach is more suitable for OOT. |
The recommended changes are made and pushed to this PR branch.
Looking forward for review comments/suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for a small suggestion to move the fakeVMs
as a private field in the FakeDriver
struct for the OoT case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the instance construction inside the NewFakeDriver
func. The func can take what parameters are required.
@AxiomSamarth Thanks for the changes. I am not sure there is a good mapping between the in-tree and the out-of-tree the way VMs are added and verified. The in-tree test case is clear to read. The way the fake driver in OoT stores, nodeName etcd in the struct, is the problem I think. I would suggest to improve the fake driver struct and the implementation to be more general (handle multiple VMs/nodes etc.) |
Hi Amshu, I have not really made changes to the structure of the Also, with this change, I could add multiple fakeVMs and see through the test results that the machine in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes. LGTM :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Just a minor change. Also, squash all the commits once you are done.
9332f87
to
e882106
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
This fix should also resolve https://github.com/gardener/machine-controller-manager/issues/544 according to colleagues from MS. |
What this PR does / why we need it:
This PR will avoid the deletion of the machines in CrashLoopBackoff state by the safety controller
Which issue(s) this PR fixes:
Fixes #583
Special notes for your reviewer:
More UTs shall be added to test this scenario in the near future. This one is an immediate solution to mitigate the issue.
Release note: