Make machine present in workspace runtime when starting event is sent #8210
Conversation
garagatyi
added
kind/bug
|
ci-build |
|
ci-test |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4412/ |
| throws InternalInfrastructureException { | ||
| if (machines != null) { | ||
| if (!machines.containsKey(name)) { | ||
| throw new InternalInfrastructureException("Machine is not in a STARTING state"); |
There was a problem hiding this comment.
I think it doesn't really mean that machine is not STARTING. The condition will be true even when a machine is RUNNING. But is this check critical?
What we can do
- add checking that a machine has
STARTINGstatus; - do not check that and put a new machine in any case.
There was a problem hiding this comment.
RunningDockerMachine everywhere looks not so good. And StaringDockerMachine is a stub that cannot be used outside of DockerInternalRuntime.
I propose to consider the following way:
- Create
StartingDockerMachineinternal class of DockerInternalRuntime that will extendMachine. - Have two methods in
StartSynchronizer: one for gettingMachine(public representation) and another one for getting DockerMachine (internal representation).
Then we won't need a refactoring and having an interface for DockerMachine.
Let me know if I didn't explain good enough.
WDYT?
|
ci-test build report: |
…arting event is sent Also set machine RUNNING status before bootstrapping installers and checking servers statuses. Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
garagatyi
force-pushed
the
machineStatusFix
branch
from
ec6cf9e
to
304f0b6
Compare
|
@sleshchenko I reworked the PR in accordance with our discussion. Please, take a look |
|
ci-test |
…when starting event is sent
| /** Name of the latest tag used in Docker image. */ | ||
| public static final String LATEST_TAG = "latest"; | ||
|
|
||
| private static final Logger LOG = getLogger(DockerMachine.class); |
| * @throws InternalInfrastructureException if machine is not present in this storage which means | ||
| * that correct runtime start flow is broken | ||
| */ | ||
| public synchronized void replaceMachine(String name, DockerMachine machine) |
There was a problem hiding this comment.
method name says nothing about STARTING machines. Why do we need this method?
Maybe it would be good enough to have only putMachine(String name, DockerMachine machine) instead of addMachine and replaceMachine.
There was a problem hiding this comment.
I was thinking about preventing misuse of the method
…untime when starting event is sent
| try { | ||
| startMachine(machineName, config); | ||
| DockerMachine machine = startMachine(machineName, containerEntry.getValue()); |
There was a problem hiding this comment.
Let's put running machine into runtimeMachines here instead of in startMachine method. I think it will make it more clear that starting machine will be updated.
|
|
||
| /** Returns machines map. If runtime start is canceled returns empty map. */ | ||
| public synchronized Map<String, DockerMachine> getMachines() { | ||
| return machines != null ? machines : emptyMap(); |
There was a problem hiding this comment.
As I understand here it is better to return the copy of original machines, because of guarantee that it won't be modified by RuntimeMachines and you can traverse on it(like here) without risking of concurrent modification.
There was a problem hiding this comment.
The copy is done here, whereas this component is kinda internal, so I would leave it as is, for now
|
ci-test build report: |
…space runtime when starting event is sent
|
@akorneta you are right. I've fixed that |
benoitf
removed
the
status/code-review
What does this PR do?
Make machine present in workspace runtime when starting event is sent.
Also set machine RUNNING status before bootstrapping installers and
checking servers statuses.
What issues does this PR fix or reference?
Fixes #7900
Fixes #8056
Release Notes
Docs PR