-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
CODENVY-1403: fix bug in machine removal #4154
Conversation
Bug occurs when container is unavailable without stop of machine. In that case we don't stop machine instance and not cleanup resources. Signed-off-by: Alexander Garagatyi
.withError(message); | ||
|
||
try { | ||
if (!Strings.isNullOrEmpty(message)) { |
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.
How it can became empty ??
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.
😄 copied from older code. Good catch!
@@ -158,7 +160,12 @@ public CheEnvironmentEngine(SnapshotDao snapshotDao, | |||
"/recipe/.*$)|(^/recipe/.*$)"); | |||
this.containerNameGenerator = containerNameGenerator; | |||
|
|||
eventService.subscribe(new MachineCleaner()); | |||
eventService.subscribe(event -> { |
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.
Can you please subscribe outside the constructor e.g.
@PostConstruct
private void subscribe() {
eventSubscriber.subscribe(...)
}
As you implicitly expose instance which hasn't been fully constructed yet
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.
ok
"MB. Create a new machine configuration that allocates additional RAM or increase " + | ||
"the workspace RAM limit in the user dashboard."; | ||
} | ||
MachineStatusEvent destroyedEvent = newDto(MachineStatusEvent.class) |
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.
What if we move this event to the place where it's published
eventService.publish(newDto(...));
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1984/ |
Signed-off-by: Alexander Garagatyi <[email protected]>
… Alexander Garagatyi <[email protected]>
@mshaposhnik @evoevodin I updated PR |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1987/ |
Bug occurs when container is unavailable without stop of machine. In that case we don't stop machine instance and not cleanup resources. Signed-off-by: Alexander Garagatyi
What does this PR do?
Fix bug in machine removal.
Bug occurs when container is unavailable without stop of machine.
In that case we don't stop machine instance and not cleanup
resources.
What issues does this PR fix or reference?
Related to codenvy/codenvy#1403
Changelog
Fix machine stop in case docker container was stopped or killed not by Codenvy.
Release Notes
Docs PR