From 0c77efc2b3a5ace0a31b160d19e57bc93dd16bcc Mon Sep 17 00:00:00 2001 From: Oleksandr Garagatyi Date: Thu, 12 Oct 2017 16:19:58 +0300 Subject: [PATCH] Workspace start code cleanup Moved async operations from WorkspaceManager to WorkspaceRuntimes to have async facility in one place instead of two. Moved workspace start/stop logging from WorkspaceManager to WorkspaceRuntimes since WorkspaceManager can not correctly log them. Improved logging of workspace start/stop including addition of new logs. Fixed logging of exception thrown by RuntimeInfrastructure on runtime start/stop. Fix docker image deletion bug on stop of a workspace. Signed-off-by: Oleksandr Garagatyi --- .../infrastructure/docker/DockerMachine.java | 3 +- .../docker/DockerMachineCreator.java | 2 +- .../LimitsCheckingWorkspaceManager.java | 4 +- .../LimitsCheckingWorkspaceManagerTest.java | 1 - .../workspace/server/WorkspaceManager.java | 130 ++--------- .../workspace/server/WorkspaceRuntimes.java | 219 +++++++++++++----- .../bootstrap/AbstractBootstrapper.java | 2 +- .../workspace/server/spi/InternalRuntime.java | 14 +- .../server/WorkspaceManagerTest.java | 41 +--- .../server/spi/InternalRuntimeTest.java | 5 +- 10 files changed, 201 insertions(+), 220 deletions(-) diff --git a/infrastructures/docker/src/main/java/org/eclipse/che/workspace/infrastructure/docker/DockerMachine.java b/infrastructures/docker/src/main/java/org/eclipse/che/workspace/infrastructure/docker/DockerMachine.java index 9472c39406..5fd947d2b3 100644 --- a/infrastructures/docker/src/main/java/org/eclipse/che/workspace/infrastructure/docker/DockerMachine.java +++ b/infrastructures/docker/src/main/java/org/eclipse/che/workspace/infrastructure/docker/DockerMachine.java @@ -154,8 +154,7 @@ public class DockerMachine implements Machine { try { docker.removeImage(RemoveImageParams.create(image).withForce(false)); } catch (IOException e) { - // TODO make log level warning if we ignoring it or remove ignoring phrase - LOG.error("IOException during destroy(). Ignoring.", e); + LOG.warn("IOException during destroy(). Ignoring.", e); } } diff --git a/infrastructures/docker/src/main/java/org/eclipse/che/workspace/infrastructure/docker/DockerMachineCreator.java b/infrastructures/docker/src/main/java/org/eclipse/che/workspace/infrastructure/docker/DockerMachineCreator.java index 2248ea384d..6f7f6b30fa 100644 --- a/infrastructures/docker/src/main/java/org/eclipse/che/workspace/infrastructure/docker/DockerMachineCreator.java +++ b/infrastructures/docker/src/main/java/org/eclipse/che/workspace/infrastructure/docker/DockerMachineCreator.java @@ -67,7 +67,7 @@ public class DockerMachineCreator { return new DockerMachine( container.getId(), - container.getImage(), + container.getConfig().getImage(), docker, new ServersMapper(hostname).map(networkSettings.getPorts(), configs), registry, diff --git a/multiuser/api/che-multiuser-api-resource/src/main/java/org/eclipse/che/multiuser/resource/api/workspace/LimitsCheckingWorkspaceManager.java b/multiuser/api/che-multiuser-api-resource/src/main/java/org/eclipse/che/multiuser/resource/api/workspace/LimitsCheckingWorkspaceManager.java index 12a746ce4d..11cd830b0b 100644 --- a/multiuser/api/che-multiuser-api-resource/src/main/java/org/eclipse/che/multiuser/resource/api/workspace/LimitsCheckingWorkspaceManager.java +++ b/multiuser/api/che-multiuser-api-resource/src/main/java/org/eclipse/che/multiuser/resource/api/workspace/LimitsCheckingWorkspaceManager.java @@ -33,7 +33,6 @@ import org.eclipse.che.api.core.model.workspace.config.Environment; import org.eclipse.che.api.core.notification.EventService; import org.eclipse.che.api.workspace.server.WorkspaceManager; import org.eclipse.che.api.workspace.server.WorkspaceRuntimes; -import org.eclipse.che.api.workspace.server.WorkspaceSharedPool; import org.eclipse.che.api.workspace.server.WorkspaceValidator; import org.eclipse.che.api.workspace.server.model.impl.WorkspaceImpl; import org.eclipse.che.api.workspace.server.spi.WorkspaceDao; @@ -76,13 +75,12 @@ public class LimitsCheckingWorkspaceManager extends WorkspaceManager { EventService eventService, AccountManager accountManager, WorkspaceValidator workspaceValidator, - WorkspaceSharedPool sharedPool, //own injects @Named("che.limits.workspace.env.ram") String maxRamPerEnv, EnvironmentRamCalculator environmentRamCalculator, ResourceUsageManager resourceUsageManager, ResourcesLocks resourcesLocks) { - super(workspaceDao, runtimes, eventService, accountManager, workspaceValidator, sharedPool); + super(workspaceDao, runtimes, eventService, accountManager, workspaceValidator); this.environmentRamCalculator = environmentRamCalculator; this.maxRamPerEnvMB = "-1".equals(maxRamPerEnv) ? -1 : Size.parseSizeToMegabytes(maxRamPerEnv); this.resourceUsageManager = resourceUsageManager; diff --git a/multiuser/api/che-multiuser-api-resource/src/test/java/org/eclipse/che/multiuser/resource/api/workspace/LimitsCheckingWorkspaceManagerTest.java b/multiuser/api/che-multiuser-api-resource/src/test/java/org/eclipse/che/multiuser/resource/api/workspace/LimitsCheckingWorkspaceManagerTest.java index 06924ff859..ab53562072 100644 --- a/multiuser/api/che-multiuser-api-resource/src/test/java/org/eclipse/che/multiuser/resource/api/workspace/LimitsCheckingWorkspaceManagerTest.java +++ b/multiuser/api/che-multiuser-api-resource/src/test/java/org/eclipse/che/multiuser/resource/api/workspace/LimitsCheckingWorkspaceManagerTest.java @@ -274,7 +274,6 @@ public class LimitsCheckingWorkspaceManagerTest { null, null, null, - null, maxRamPerEnv, environmentRamCalculator, resourceUsageManager, diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java index 7a355d5edc..be8497e42a 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java @@ -11,20 +11,17 @@ package org.eclipse.che.api.workspace.server; import static com.google.common.base.MoreObjects.firstNonNull; -import static com.google.common.base.Throwables.getCausalChain; import static java.lang.String.format; import static java.lang.System.currentTimeMillis; import static java.util.Objects.requireNonNull; import static org.eclipse.che.api.core.model.workspace.WorkspaceStatus.RUNNING; import static org.eclipse.che.api.core.model.workspace.WorkspaceStatus.STARTING; import static org.eclipse.che.api.core.model.workspace.WorkspaceStatus.STOPPED; -import static org.eclipse.che.api.workspace.shared.Constants.WORKSPACE_STOPPED_BY; import com.google.inject.Inject; import java.util.Collections; import java.util.List; import java.util.Map; -import java.util.concurrent.CompletableFuture; import javax.inject.Singleton; import org.eclipse.che.account.api.AccountManager; import org.eclipse.che.account.shared.model.Account; @@ -38,8 +35,6 @@ import org.eclipse.che.api.core.model.workspace.WorkspaceStatus; import org.eclipse.che.api.core.notification.EventService; import org.eclipse.che.api.workspace.server.model.impl.WorkspaceConfigImpl; import org.eclipse.che.api.workspace.server.model.impl.WorkspaceImpl; -import org.eclipse.che.api.workspace.server.spi.InfrastructureException; -import org.eclipse.che.api.workspace.server.spi.InternalInfrastructureException; import org.eclipse.che.api.workspace.server.spi.WorkspaceDao; import org.eclipse.che.api.workspace.shared.event.WorkspaceCreatedEvent; import org.eclipse.che.commons.annotation.Nullable; @@ -69,7 +64,6 @@ public class WorkspaceManager { private final WorkspaceDao workspaceDao; private final WorkspaceRuntimes runtimes; private final AccountManager accountManager; - private final WorkspaceSharedPool sharedPool; private final EventService eventService; private final WorkspaceValidator validator; @@ -79,13 +73,11 @@ public class WorkspaceManager { WorkspaceRuntimes runtimes, EventService eventService, AccountManager accountManager, - WorkspaceValidator validator, - WorkspaceSharedPool sharedPool) { + WorkspaceValidator validator) { this.workspaceDao = workspaceDao; this.runtimes = runtimes; this.accountManager = accountManager; this.eventService = eventService; - this.sharedPool = sharedPool; this.validator = validator; } @@ -273,7 +265,7 @@ public class WorkspaceManager { } workspaceDao.remove(workspaceId); - LOG.info("Workspace '{}' removed by user '{}'", workspaceId, sessionUserNameOr("undefined")); + LOG.info("Workspace '{}' removed by user '{}'", workspaceId, sessionUserNameOrUndefined()); } /** @@ -344,13 +336,24 @@ public class WorkspaceManager { requireNonNull(workspaceId, "Required non-null workspace id"); final WorkspaceImpl workspace = normalizeState(workspaceDao.get(workspaceId), true); - checkWorkspaceIsRunningOrStarting(workspace, "stop"); - stopAsync(workspace, options); + checkWorkspaceIsRunningOrStarting(workspace); + if (!workspace.isTemporary()) { + workspace.getAttributes().put(UPDATED_ATTRIBUTE_NAME, Long.toString(currentTimeMillis())); + workspaceDao.update(workspace); + } + + runtimes + .stopAsync(workspace, options) + .whenComplete( + (aVoid, throwable) -> { + if (workspace.isTemporary()) { + removeWorkspaceQuietly(workspace); + } + }); } /** Asynchronously starts given workspace. */ - private CompletableFuture startAsync( - WorkspaceImpl workspace, String envName, Map options) + private void startAsync(WorkspaceImpl workspace, String envName, Map options) throws ConflictException, NotFoundException, ServerException { if (envName != null && !workspace.getConfig().getEnvironments().containsKey(envName)) { throw new NotFoundException( @@ -362,81 +365,23 @@ public class WorkspaceManager { workspaceDao.update(workspace); final String env = firstNonNull(envName, workspace.getConfig().getDefaultEnv()); - return runtimes + runtimes .startAsync(workspace, env, firstNonNull(options, Collections.emptyMap())) - .thenRun( - () -> - LOG.info( - "Workspace '{}:{}' with id '{}' started by user '{}'", - workspace.getNamespace(), - workspace.getConfig().getName(), - workspace.getId(), - sessionUserNameOr("undefined"))) .exceptionally( ex -> { if (workspace.isTemporary()) { removeWorkspaceQuietly(workspace); } - for (Throwable cause : getCausalChain(ex)) { - if (cause instanceof InfrastructureException - && !(cause instanceof InternalInfrastructureException)) { - - // InfrastructureException is supposed to be an exception that can't be solved - // by the admin, so should not be logged (but not InternalInfrastructureException). - // It will prevent bothering the admin when user made a mistake in WS configuration. - return null; - } - } - LOG.error(ex.getLocalizedMessage(), ex); return null; }); } - private CompletableFuture stopAsync(WorkspaceImpl workspace, Map options) - throws ConflictException, NotFoundException, ServerException { - if (!workspace.isTemporary()) { - workspace.getAttributes().put(UPDATED_ATTRIBUTE_NAME, Long.toString(currentTimeMillis())); - workspaceDao.update(workspace); - } - String stoppedBy = sessionUserNameOr(workspace.getAttributes().get(WORKSPACE_STOPPED_BY)); - LOG.info( - "Workspace '{}/{}' with id '{}' is being stopped by user '{}'", - workspace.getNamespace(), - workspace.getConfig().getName(), - workspace.getId(), - firstNonNull(stoppedBy, "undefined")); - - return sharedPool.runAsync( - () -> { - try { - runtimes.stop(workspace.getId(), options); - - LOG.info( - "Workspace '{}/{}' with id '{}' stopped by user '{}'", - workspace.getNamespace(), - workspace.getConfig().getName(), - workspace.getId(), - firstNonNull(stoppedBy, "undefined")); - } catch (Exception ex) { - LOG.error(ex.getLocalizedMessage(), ex); - } finally { - if (workspace.isTemporary()) { - removeWorkspaceQuietly(workspace); - } - } - }); - } - - private void checkWorkspaceIsRunningOrStarting(WorkspaceImpl workspace, String operation) - throws ConflictException { + private void checkWorkspaceIsRunningOrStarting(WorkspaceImpl workspace) throws ConflictException { if (workspace.getStatus() != RUNNING && workspace.getStatus() != STARTING) { throw new ConflictException( format( - "Could not %s the workspace '%s/%s' because its status is '%s'.", - operation, - workspace.getNamespace(), - workspace.getConfig().getName(), - workspace.getStatus())); + "Could not stop the workspace '%s/%s' because its status is '%s'.", + workspace.getNamespace(), workspace.getConfig().getName(), workspace.getStatus())); } } @@ -448,12 +393,12 @@ public class WorkspaceManager { } } - private String sessionUserNameOr(String nameIfNoUser) { + private String sessionUserNameOrUndefined() { final Subject subject = EnvironmentContext.getCurrent().getSubject(); if (!subject.isAnonymous()) { return subject.getUserName(); } - return nameIfNoUser; + return "undefined"; } private WorkspaceImpl normalizeState(WorkspaceImpl workspace, boolean includeRuntimes) @@ -486,7 +431,7 @@ public class WorkspaceManager { account.getName(), workspace.getConfig().getName(), workspace.getId(), - sessionUserNameOr("undefined")); + sessionUserNameOrUndefined()); eventService.publish(new WorkspaceCreatedEvent(workspace)); return workspace; } @@ -515,31 +460,4 @@ public class WorkspaceManager { } return workspaceDao.get(wsName, namespace); } - - // FIXME: this code is from master version of runtimes, where - // WorkspaceRuntimes is responsible for statuses management. - // - // /** Adds runtime data (whole or status only) and extra attributes to each of the given workspaces. */ - // private void injectRuntimeAndAttributes(List workspaces, boolean statusOnly) throws SnapshotException { - // if (statusOnly) { - // for (WorkspaceImpl workspace : workspaces) { - // workspace.setStatus(runtimes.getStatus(workspace.getId())); - // addExtraAttributes(workspace); - // } - // } else { - // for (WorkspaceImpl workspace : workspaces) { - // runtimes.injectRuntime(workspace); - // addExtraAttributes(workspace); - // } - // } - // } - // - // /** Adds attributes that are not originally stored in workspace but should be published. */ - // private void addExtraAttributes(WorkspaceImpl workspace) throws SnapshotException { - // // snapshotted_at - // List snapshots = snapshotDao.findSnapshots(workspace.getId()); - // if (!snapshots.isEmpty()) { - // workspace.getAttributes().put(SNAPSHOTTED_AT_ATTRIBUTE_NAME, Long.toString(snapshots.get(0).getCreationDate())); - // } - // } } diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceRuntimes.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceRuntimes.java index 8b0949cf28..700ffa6d8a 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceRuntimes.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceRuntimes.java @@ -10,12 +10,14 @@ */ package org.eclipse.che.api.workspace.server; +import static com.google.common.base.MoreObjects.firstNonNull; import static java.lang.String.format; import static java.util.Objects.requireNonNull; import static org.eclipse.che.api.core.model.workspace.WorkspaceStatus.RUNNING; import static org.eclipse.che.api.core.model.workspace.WorkspaceStatus.STARTING; import static org.eclipse.che.api.core.model.workspace.WorkspaceStatus.STOPPED; import static org.eclipse.che.api.core.model.workspace.WorkspaceStatus.STOPPING; +import static org.eclipse.che.api.workspace.shared.Constants.WORKSPACE_STOPPED_BY; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableMap; @@ -64,8 +66,6 @@ import org.eclipse.che.dto.server.DtoFactory; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -// TODO: spi: deal with exceptions - /** * Defines an internal API for managing {@link RuntimeImpl} instances. * @@ -167,15 +167,13 @@ public class WorkspaceRuntimes { * Starts all machines from specified workspace environment, creates workspace runtime instance * based on that environment. * - *

- * *

During the start of the workspace its runtime is visible with {@link * WorkspaceStatus#STARTING} status. * * @param workspace workspace which environment should be started * @param envName the name of the environment to start * @param options whether machines should be recovered(true) or not(false) - * @return the workspace runtime instance with machines set. + * @return completable future of start execution. * @throws ConflictException when workspace is already running * @throws ConflictException when start is interrupted * @throws NotFoundException when any not found exception occurs during environment start @@ -237,32 +235,18 @@ public class WorkspaceRuntimes { throw new ConflictException( "Could not start workspace '" + workspaceId + "' because it is not in 'STOPPED' state"); } - eventService.publish( - DtoFactory.newDto(WorkspaceStatusEvent.class) - .withWorkspaceId(workspaceId) - .withStatus(STARTING) - .withPrevStatus(STOPPED)); + LOG.info( + "Starting workspace '{}/{}' with id '{}' by user '{}'", + workspace.getNamespace(), + workspace.getConfig().getName(), + workspace.getId(), + sessionUserNameOr("undefined")); + + publishWorkspaceStatusEvent(workspaceId, STARTING, STOPPED, null); + return CompletableFuture.runAsync( - ThreadLocalPropagateContext.wrap( - () -> { - try { - runtime.start(options); - runtimes.replace(workspaceId, new RuntimeState(runtime, RUNNING)); - eventService.publish( - DtoFactory.newDto(WorkspaceStatusEvent.class) - .withWorkspaceId(workspaceId) - .withStatus(RUNNING) - .withPrevStatus(STARTING)); - } catch (InfrastructureException e) { - runtimes.remove(workspaceId); - if (!(e instanceof RuntimeStartInterruptedException)) { - publishWorkspaceStoppedEvent(workspaceId, STARTING, e.getMessage()); - } - throw new RuntimeException(e); - } - }), + ThreadLocalPropagateContext.wrap(new StartRuntimeTask(workspace, options, runtime)), sharedPool.getExecutor()); - //TODO made complete rework of exceptions. } catch (ValidationException e) { LOG.error(e.getLocalizedMessage(), e); throw new ConflictException(e.getLocalizedMessage()); @@ -272,23 +256,73 @@ public class WorkspaceRuntimes { } } + private class StartRuntimeTask implements Runnable { + private final Workspace workspace; + private final Map options; + private final InternalRuntime runtime; + + public StartRuntimeTask( + Workspace workspace, Map options, InternalRuntime runtime) { + this.workspace = workspace; + this.options = options; + this.runtime = runtime; + } + + @Override + public void run() { + String workspaceId = workspace.getId(); + try { + runtime.start(options); + runtimes.replace(workspaceId, new RuntimeState(runtime, RUNNING)); + + LOG.info( + "Workspace '{}:{}' with id '{}' started by user '{}'", + workspace.getNamespace(), + workspace.getConfig().getName(), + workspaceId, + sessionUserNameOr("undefined")); + publishWorkspaceStatusEvent(workspaceId, RUNNING, STARTING, null); + } catch (InfrastructureException e) { + runtimes.remove(workspaceId); + String failureCause = "failed"; + if (e instanceof RuntimeStartInterruptedException) { + failureCause = "interrupted"; + } + LOG.info( + "Workspace '{}:{}' with id '{}' start {}", + workspace.getNamespace(), + workspace.getConfig().getName(), + workspaceId, + failureCause); + // InfrastructureException is supposed to be an exception that can't be solved + // by Che admin, so should not be logged (but not InternalInfrastructureException). + // It will prevent bothering the admin when user made a mistake in WS configuration. + if (e instanceof InternalInfrastructureException) { + LOG.error(e.getLocalizedMessage(), e); + } + publishWorkspaceStatusEvent(workspaceId, STOPPED, STARTING, e.getMessage()); + throw new RuntimeException(e); + } + } + } + /** - * Stops running workspace runtime. - * - *

+ * Stops running workspace runtime asynchronously. * *

Stops environment in an implementation specific way. During the stop of the workspace its * runtime is accessible with {@link WorkspaceStatus#STOPPING stopping} status. Workspace may be - * stopped only if its status is {@link WorkspaceStatus#RUNNING}. + * stopped only if its status is {@link WorkspaceStatus#RUNNING} or {@link + * WorkspaceStatus#STARTING}. * - * @param workspaceId identifier of workspace which should be stopped + * @param workspace workspace which runtime should be stopped * @throws NotFoundException when workspace with specified identifier does not have runtime * @throws ConflictException when running workspace status is different from {@link - * WorkspaceStatus#RUNNING} + * WorkspaceStatus#RUNNING} or {@link WorkspaceStatus#STARTING} * @see WorkspaceStatus#STOPPING */ - public void stop(String workspaceId, Map options) + public CompletableFuture stopAsync(Workspace workspace, Map options) throws NotFoundException, ConflictException { + String workspaceId = workspace.getId(); RuntimeState state = runtimes.get(workspaceId); if (state == null) { throw new NotFoundException("Workspace with id '" + workspaceId + "' is not running."); @@ -305,25 +339,74 @@ public class WorkspaceRuntimes { throw new ConflictException( format("Could not stop workspace '%s' because its state is '%s'", workspaceId, status)); } - eventService.publish( - DtoFactory.newDto(WorkspaceStatusEvent.class) - .withWorkspaceId(workspaceId) - .withPrevStatus(state.status) - .withStatus(STOPPING)); + String stoppedBy = + firstNonNull( + sessionUserNameOr(workspace.getAttributes().get(WORKSPACE_STOPPED_BY)), "undefined"); + LOG.info( + "Workspace '{}/{}' with id '{}' is being stopped by user '{}'", + workspace.getNamespace(), + workspace.getConfig().getName(), + workspace.getId(), + stoppedBy); + publishWorkspaceStatusEvent(workspaceId, STOPPING, state.status, null); - try { - state.runtime.stop(options); + return CompletableFuture.runAsync( + ThreadLocalPropagateContext.wrap(new StopRuntimeTask(workspace, options, stoppedBy, state)), + sharedPool.getExecutor()); + } - // remove before firing an event to have consistency between state and the event - runtimes.remove(workspaceId); - publishWorkspaceStoppedEvent(workspaceId, STOPPING, null); - } catch (InfrastructureException e) { - // remove before firing an event to have consistency between state and the event - runtimes.remove(workspaceId); - publishWorkspaceStoppedEvent( - workspaceId, - STOPPING, - "Error occurs on workspace runtime stop. Error: " + e.getMessage()); + private class StopRuntimeTask implements Runnable { + private final Workspace workspace; + private final Map options; + private final String stoppedBy; + private final RuntimeState state; + + public StopRuntimeTask( + Workspace workspace, Map options, String stoppedBy, RuntimeState state) { + this.workspace = workspace; + this.options = options; + this.stoppedBy = stoppedBy; + this.state = state; + } + + @Override + public void run() { + String workspaceId = workspace.getId(); + try { + state.runtime.stop(options); + + // remove before firing an event to have consistency between state and the event + runtimes.remove(workspaceId); + LOG.info( + "Workspace '{}/{}' with id '{}' stopped by user '{}'", + workspace.getNamespace(), + workspace.getConfig().getName(), + workspaceId, + stoppedBy); + publishWorkspaceStatusEvent(workspaceId, STOPPED, STOPPING, null); + } catch (InfrastructureException e) { + // remove before firing an event to have consistency between state and the event + runtimes.remove(workspaceId); + LOG.info( + "Error occurs on workspace '{}/{}' with id '{}' stopped by user '{}'. Error: {}", + workspace.getNamespace(), + workspace.getConfig().getName(), + workspaceId, + stoppedBy, + e); + publishWorkspaceStatusEvent( + workspaceId, + STOPPED, + STOPPING, + "Error occurs on workspace runtime stop. Error: " + e.getMessage()); + // InfrastructureException is supposed to be an exception that can't be solved + // by Che admin, so should not be logged (but not InternalInfrastructureException). + // It will prevent bothering the admin when user made a mistake in WS configuration. + if (e instanceof InternalInfrastructureException) { + LOG.error(e.getLocalizedMessage(), e); + } + throw new RuntimeException(e); + } } } @@ -350,8 +433,9 @@ public class WorkspaceRuntimes { LOG.warn("Not recoverable infrastructure: '{}'", infra.getName()); } catch (InternalInfrastructureException x) { LOG.error( - "An error occurred while attempted to recover runtimes using infrastructure '{}'. Reason: '{}'", - infra.getName(), + format( + "An error occurred while attempted to recover runtimes using infrastructure '%s'", + infra.getName()), x); } catch (ServerException | InfrastructureException x) { LOG.error( @@ -419,14 +503,14 @@ public class WorkspaceRuntimes { eventService.subscribe(new CleanupRuntimeOnAbnormalRuntimeStop()); } - private void publishWorkspaceStoppedEvent( - String workspaceId, WorkspaceStatus previous, String errorMsg) { + private void publishWorkspaceStatusEvent( + String workspaceId, WorkspaceStatus status, WorkspaceStatus previous, String errorMsg) { eventService.publish( DtoFactory.newDto(WorkspaceStatusEvent.class) .withWorkspaceId(workspaceId) .withPrevStatus(previous) .withError(errorMsg) - .withStatus(STOPPED)); + .withStatus(status)); } private static EnvironmentImpl copyEnv(Workspace workspace, String envName) { @@ -484,18 +568,25 @@ public class WorkspaceRuntimes { return Optional.of(state.runtime.getContext()); } + private String sessionUserNameOr(String nameIfNoUser) { + final Subject subject = EnvironmentContext.getCurrent().getSubject(); + if (!subject.isAnonymous()) { + return subject.getUserName(); + } + return nameIfNoUser; + } + private class CleanupRuntimeOnAbnormalRuntimeStop implements EventSubscriber { @Override public void onEvent(RuntimeStatusEvent event) { if (event.isFailed()) { RuntimeState state = runtimes.remove(event.getIdentity().getWorkspaceId()); if (state != null) { - eventService.publish( - DtoFactory.newDto(WorkspaceStatusEvent.class) - .withWorkspaceId(state.runtime.getContext().getIdentity().getWorkspaceId()) - .withPrevStatus(RUNNING) - .withStatus(STOPPED) - .withError("Error occurs on workspace runtime stop. Error: " + event.getError())); + publishWorkspaceStatusEvent( + state.runtime.getContext().getIdentity().getWorkspaceId(), + STOPPED, + RUNNING, + "Error occurs on workspace runtime stop. Error: " + event.getError()); } } } diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/bootstrap/AbstractBootstrapper.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/bootstrap/AbstractBootstrapper.java index c9875b49e5..68babe8844 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/bootstrap/AbstractBootstrapper.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/bootstrap/AbstractBootstrapper.java @@ -53,7 +53,7 @@ public abstract class AbstractBootstrapper { BootstrapperStatus status = event.getStatus(); //skip starting status event if (status.equals(BootstrapperStatus.DONE) || status.equals(BootstrapperStatus.FAILED)) { - //check boostrapper belongs to current runtime and machine + //check bootstrapper belongs to current runtime and machine RuntimeIdentityDto runtimeId = event.getRuntimeId(); if (event.getMachineName().equals(machineName) && runtimeIdentity.getEnvName().equals(runtimeId.getEnvName()) diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/spi/InternalRuntime.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/spi/InternalRuntime.java index 8e47763f20..714497fca9 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/spi/InternalRuntime.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/spi/InternalRuntime.java @@ -11,7 +11,6 @@ package org.eclipse.che.api.workspace.server.spi; import static java.util.stream.Collectors.toMap; -import static org.slf4j.LoggerFactory.getLogger; import java.util.HashMap; import java.util.List; @@ -27,7 +26,6 @@ import org.eclipse.che.api.workspace.server.URLRewriter; import org.eclipse.che.api.workspace.server.model.impl.MachineImpl; import org.eclipse.che.api.workspace.server.model.impl.ServerImpl; import org.eclipse.che.api.workspace.server.model.impl.WarningImpl; -import org.slf4j.Logger; /** * Implementation of concrete Runtime @@ -36,8 +34,6 @@ import org.slf4j.Logger; */ public abstract class InternalRuntime implements Runtime { - private static final Logger LOG = getLogger(InternalRuntime.class); - private final T context; private final URLRewriter urlRewriter; private final List warnings; @@ -139,15 +135,9 @@ public abstract class InternalRuntime implements Runti try { internalStop(stopOptions); - } catch (InternalInfrastructureException e) { - LOG.error( - "Error occurs on stop of workspace {}. Error: {}", - context.getIdentity().getWorkspaceId(), - e.getMessage()); - } catch (InfrastructureException e) { - LOG.debug(e.getMessage(), e); + } finally { + status = WorkspaceStatus.STOPPED; } - status = WorkspaceStatus.STOPPED; } /** diff --git a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java index 301bb8ff7a..94590288f1 100644 --- a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java +++ b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java @@ -23,7 +23,6 @@ import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyObject; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; -import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; @@ -95,21 +94,18 @@ public class WorkspaceManagerTest { @Mock private WorkspaceDao workspaceDao; @Mock private WorkspaceRuntimes runtimes; @Mock private AccountManager accountManager; - @Mock private WorkspaceSharedPool sharedPool; @Mock private EventService eventService; @Mock private WorkspaceValidator validator; @Mock private RuntimeInfrastructure infrastructure; @Captor private ArgumentCaptor workspaceCaptor; - @Captor private ArgumentCaptor taskCaptor; private WorkspaceManager workspaceManager; @BeforeMethod public void setUp() throws Exception { workspaceManager = - new WorkspaceManager( - workspaceDao, runtimes, eventService, accountManager, validator, sharedPool); + new WorkspaceManager(workspaceDao, runtimes, eventService, accountManager, validator); when(accountManager.getByName(NAMESPACE_1)) .thenReturn(new AccountImpl("accountId", NAMESPACE_1, "test")); when(accountManager.getByName(NAMESPACE_2)) @@ -429,36 +425,25 @@ public class WorkspaceManagerTest { public void stopsWorkspace() throws Exception { final WorkspaceImpl workspace = createAndMockWorkspace(createConfig(), NAMESPACE_1); mockRuntime(workspace, RUNNING); + mockAnyWorkspaceStop(); workspaceManager.stopWorkspace(workspace.getId(), emptyMap()); - captureRunAsyncCallsAndRunSynchronously(); - verify(runtimes).stop(workspace.getId(), emptyMap()); + verify(runtimes).stopAsync(workspace, emptyMap()); verify(workspaceDao).update(workspaceCaptor.capture()); assertNotNull(workspaceCaptor.getValue().getAttributes().get(UPDATED_ATTRIBUTE_NAME)); } - @Test( - expectedExceptions = ConflictException.class, - expectedExceptionsMessageRegExp = - "Could not stop the workspace " + "'.*' because its status is 'STOPPED'." - ) - public void throwsConflictExceptionWhenStoppingWorkspaceWithoutRuntime() throws Exception { - final WorkspaceImpl workspace = createAndMockWorkspace(); - - workspaceManager.stopWorkspace(workspace.getId(), emptyMap()); - } - @Test public void removesTemporaryWorkspaceAfterStop() throws Exception { final WorkspaceImpl workspace = createAndMockWorkspace(); workspace.setTemporary(true); mockRuntime(workspace, RUNNING); + mockAnyWorkspaceStop(); workspaceManager.stopWorkspace(workspace.getId(), emptyMap()); - captureRunAsyncCallsAndRunSynchronously(); - verify(runtimes).stop(workspace.getId(), emptyMap()); + verify(runtimes).stopAsync(workspace, emptyMap()); verify(workspaceDao).remove(workspace.getId()); } @@ -469,10 +454,10 @@ public class WorkspaceManagerTest { mockRuntime(workspace, RUNNING); doThrow(new ConflictException("runtime stop failed")) .when(runtimes) - .stop(workspace.getId(), emptyMap()); + .stopAsync(workspace, emptyMap()); + mockAnyWorkspaceStop(); workspaceManager.stopWorkspace(workspace.getId(), emptyMap()); - captureRunAsyncCallsAndRunSynchronously(); verify(workspaceDao).remove(workspace.getId()); } @@ -488,13 +473,6 @@ public class WorkspaceManagerTest { verify(workspaceDao, times(1)).remove(anyString()); } - private void captureRunAsyncCallsAndRunSynchronously() { - verify(sharedPool, atLeastOnce()).runAsync(taskCaptor.capture()); - for (Runnable runnable : taskCaptor.getAllValues()) { - runnable.run(); - } - } - private void mockRuntimeStatus(WorkspaceImpl workspace, WorkspaceStatus status) { when(runtimes.getStatus(workspace.getId())).thenReturn(status); } @@ -562,6 +540,11 @@ public class WorkspaceManagerTest { when(runtimes.startAsync(any(), anyString(), any())).thenReturn(cmpFuture); } + private void mockAnyWorkspaceStop() throws Exception { + CompletableFuture cmpFuture = CompletableFuture.completedFuture(null); + when(runtimes.stopAsync(any(), any())).thenReturn(cmpFuture); + } + private void mockAnyWorkspaceStartFailed(Exception cause) throws Exception { final CompletableFuture cmpFuture = new CompletableFuture<>(); cmpFuture.completeExceptionally(cause); diff --git a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/spi/InternalRuntimeTest.java b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/spi/InternalRuntimeTest.java index da99788b25..598a40c550 100644 --- a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/spi/InternalRuntimeTest.java +++ b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/spi/InternalRuntimeTest.java @@ -225,7 +225,10 @@ public class InternalRuntimeTest { // given setRunningRuntime(); doThrow(new InfrastructureException("")).when(internalRuntime).internalStop(any()); - internalRuntime.stop(emptyMap()); + try { + internalRuntime.stop(emptyMap()); + } catch (InfrastructureException ignore) { + } // when internalRuntime.start(emptyMap());