Improve abstract InternalRuntime to be more flexible for recovery functionality #9345
Conversation
benoitf
added
status/code-review
sleshchenko
force-pushed
the
spi-changes
branch
2 times, most recently
from
ea2e431
to
b0b5a3b
Compare
| @@ -144,16 +144,25 @@ public void validate(Environment environment) | |||
| * | |||
| * @param workspace the workspace to inject runtime into | |||
| */ | |||
| public void injectRuntime(WorkspaceImpl workspace) { | |||
| public void injectRuntime(WorkspaceImpl workspace) throws ServerException { | |||
There was a problem hiding this comment.
I'd rather use InfrastructureException. We have discussed that ServerException if rather REST exception.
There was a problem hiding this comment.
I agree that ServerException is REST exception, but InfrastructureException is an exception that is supposed to be thrown by Infrastructure. As you can see, now WorkspaceRuntimes throw only REST exceptions (Conflict, NotFound, Server).
I'd say that WorkspaceRuntimes should not throw REST exception but and Infrastructure exception isn't the right one here too. I guess there is an issue for reworking Che Master exception approach and this issue could be solved there.
@garagatyi Does it makes sense?
There was a problem hiding this comment.
Yep, you are completely right. I missed that.
| throws InfrastructureException; | ||
|
|
||
| /** | ||
| * Returns workspace status. |
|
ci-test |
|
ci-test build report: |
sleshchenko
force-pushed
the
spi-changes
branch
from
5ef1775
to
45ec5f4
Compare
|
ci-test |
|
ci-test build report: |
sleshchenko
force-pushed
the
spi-changes
branch
from
45ec5f4
to
c6efb24
Compare
|
ci-test |
|
ci-test build report: |
sleshchenko
force-pushed
the
spi-changes
branch
from
c6efb24
to
fa2fcbb
Compare
benoitf
removed
the
status/code-review
What does this PR do?
Improve abstract InternalRuntime to be more flexible for recovery functionality. It includes:
Add an ability to store status by an implementation of InternalRuntime. It allows infrastructure provide an actual status after runtime recovery. Also, it adds an ability to deny stopping of STARTING workspaces if it is not implemented like in Kubernetes/OpenShift infrastructures.
Unfortunately, denying stop of STARTING workspace will not work correctly. Here is a registered bug for that Workspace is considered as STOPPED if start interruption is not implemented #9354.
Add an ability to throw InfrastructureException by InternalRuntimes#getMachines method. It is needed because Runtime may store machines information in storage that may throw some exceptions (like database as storage may throw connection related exceptions).
What issues does this PR fix or reference?
#5919
It's needed for OpenShift recovery functionality changes #9301
Release Notes
Docs PR