From d227aaccc22aed6cfd3fb67cbec77c4c38f16465 Mon Sep 17 00:00:00 2001 From: Lucas Koehler Date: Wed, 2 Aug 2023 15:04:58 +0200 Subject: [PATCH] #200: Apply review feedback - AppDefinition resource version fix after rebase - Catch all exceptions when adding session, workspace resources and add error state - Do not handle resources in HANDLED state again. - Smaller fixes --- .../common/k8s/client/ResourceClient.java | 2 +- .../common/k8s/resource/AppDefinition.java | 4 +- .../k8s/resource/AppDefinitionStatus.java | 5 ++- .../common/k8s/resource/OperatorStatus.java | 9 ++++- .../common/k8s/resource/SessionStatus.java | 5 ++- .../cloud/common/k8s/resource/StatusStep.java | 4 ++ .../handler/impl/LazySessionHandler.java | 38 +++++++++++++++---- .../impl/LazyStartAppDefinitionHandler.java | 18 ++++++++- .../handler/impl/LazyWorkspaceHandler.java | 35 +++++++++++++---- 9 files changed, 98 insertions(+), 22 deletions(-) diff --git a/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/client/ResourceClient.java b/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/client/ResourceClient.java index 75848fe3..d1f71945 100644 --- a/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/client/ResourceClient.java +++ b/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/client/ResourceClient.java @@ -63,7 +63,7 @@ default T edit(String correlationId, String name, Consumer consumer) { } default T editStatus(String correlationId, String name, Consumer consumer) { - trace(correlationId, "Edit status of" + name); + trace(correlationId, "Edit status of " + name); Resource resource = resource(name); if (resource.get() == null) { return null; diff --git a/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/AppDefinition.java b/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/AppDefinition.java index 28cda915..d0ae6d84 100644 --- a/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/AppDefinition.java +++ b/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/AppDefinition.java @@ -25,14 +25,14 @@ import io.fabric8.kubernetes.model.annotation.Singular; import io.fabric8.kubernetes.model.annotation.Version; -@Version("v8beta") +@Version("v7beta") @Group("theia.cloud") @Singular("appdefinition") @Plural("appdefinitions") public class AppDefinition extends CustomResource implements Namespaced { private static final long serialVersionUID = 8749670583218521755L; - public static final String API = "theia.cloud/v8beta"; + public static final String API = "theia.cloud/v7beta"; public static final String KIND = "AppDefinition"; public static final String CRD_NAME = "appdefinitions.theia.cloud"; diff --git a/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/AppDefinitionStatus.java b/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/AppDefinitionStatus.java index 154c7bb9..f9df3ceb 100644 --- a/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/AppDefinitionStatus.java +++ b/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/AppDefinitionStatus.java @@ -19,5 +19,8 @@ @JsonDeserialize public class AppDefinitionStatus extends ResourceStatus { - + // This class is empty as only the common properties of the super class are + // used. Already define a specific class to allow easier extension, properly + // type the resources and resource clients. + // It is planned to extend this later with AppDefinition specific status steps. } diff --git a/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/OperatorStatus.java b/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/OperatorStatus.java index acf7fb6c..2d16e750 100644 --- a/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/OperatorStatus.java +++ b/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/OperatorStatus.java @@ -1,5 +1,5 @@ /******************************************************************************** - * Copyright (C) 2022 EclipseSource and others. + * Copyright (C) 2023 EclipseSource and others. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v. 2.0 which is available at @@ -20,11 +20,18 @@ */ public interface OperatorStatus { + /** + * The default status describing that the resource is new and was not handled + * before. + */ String NEW = "NEW"; + /** The operator tried to handle this resource but an error occurred. */ String ERROR = "ERROR"; + /** The operator started handling this resource. */ String HANDLING = "HANDLING"; + /** The operator successfully finished handling this resource. */ String HANDLED = "HANDLED"; } diff --git a/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/SessionStatus.java b/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/SessionStatus.java index df73c15d..128f1366 100644 --- a/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/SessionStatus.java +++ b/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/SessionStatus.java @@ -19,5 +19,8 @@ @JsonDeserialize public class SessionStatus extends ResourceStatus { - + // This class is empty as only the common properties of the super class are + // used. Already define a specific class to allow easier extension, properly + // type the resources and resource clients. + // It is planned to extend this later with Session specific status steps. } diff --git a/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/StatusStep.java b/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/StatusStep.java index be8bf905..e8f27132 100644 --- a/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/StatusStep.java +++ b/java/common/org.eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/StatusStep.java @@ -27,6 +27,10 @@ public class StatusStep { public StatusStep() { } + public StatusStep(String status) { + this(status, ""); + } + public StatusStep(String status, String message) { this.status = status; this.message = message; diff --git a/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/impl/LazySessionHandler.java b/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/impl/LazySessionHandler.java index 3801f238..50651f0b 100644 --- a/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/impl/LazySessionHandler.java +++ b/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/impl/LazySessionHandler.java @@ -1,5 +1,5 @@ /******************************************************************************** - * Copyright (C) 2022 EclipseSource, Lockular, Ericsson, STMicroelectronics and + * Copyright (C) 2022-2023 EclipseSource, Lockular, Ericsson, STMicroelectronics and * others. * * This program and the accompanying materials are made available under the @@ -90,15 +90,36 @@ public class LazySessionHandler implements SessionHandler { @Override public boolean sessionAdded(Session session, String correlationId) { + try { + return doSessionAdded(session, correlationId); + } catch (Throwable ex) { + LOGGER.error(formatLogMessage(correlationId, + "An unexpected exception occurred while adding Session: " + session), ex); + client.sessions().updateStatus(correlationId, session, status -> { + status.setOperatorStatus(OperatorStatus.ERROR); + status.setOperatorMessage( + "Unexpected error. Please check the logs for correlationId: " + correlationId); + }); + return false; + } + } + protected boolean doSessionAdded(Session session, String correlationId) { /* session information */ String sessionResourceName = session.getMetadata().getName(); String sessionResourceUID = session.getMetadata().getUid(); - // Check current session status and ignore if handling failed before + // Check current session status and ignore if handling failed or finished before Optional status = Optional.ofNullable(session.getStatus()); String operatorStatus = status.map(ResourceStatus::getOperatorStatus).orElse(OperatorStatus.NEW); + if (OperatorStatus.HANDLED.equals(operatorStatus)) { + LOGGER.trace(formatLogMessage(correlationId, + "Session was successfully handled before and is skipped now. Session: " + session)); + return true; + } if (OperatorStatus.ERROR.equals(operatorStatus) || OperatorStatus.HANDLING.equals(operatorStatus)) { + // TODO In the HANDLING case we should not return but continue where we left + // off. LOGGER.warn(formatLogMessage(correlationId, "Session could not be handled before and is skipped now. Current status: " + operatorStatus + ". Session: " + session)); @@ -162,6 +183,8 @@ public boolean sessionAdded(Session session, String correlationId) { s.setOperatorStatus(OperatorStatus.HANDLED); s.setOperatorMessage("Service already exists."); }); + // TODO do not return true if the sessions was in handling state at the start of + // this handler return true; } @@ -187,6 +210,8 @@ public boolean sessionAdded(Session session, String correlationId) { s.setOperatorStatus(OperatorStatus.HANDLED); s.setOperatorMessage("Configmaps already exist."); }); + // TODO do not return true if the sessions was in handling state at the start of + // this handler return true; } createAndApplyEmailConfigMap(correlationId, sessionResourceName, sessionResourceUID, session); @@ -352,8 +377,7 @@ protected void createAndApplyEmailConfigMap(String correlationId, String session return; } K8sUtil.loadAndCreateConfigMapWithOwnerReference(client.kubernetes(), client.namespace(), correlationId, - configMapYaml, Session.API, Session.KIND, sessionResourceName, sessionResourceUID, 0, - configmap -> { + configMapYaml, Session.API, Session.KIND, sessionResourceName, sessionResourceUID, 0, configmap -> { configmap.setData(Collections.singletonMap(AddedHandlerUtil.FILENAME_AUTHENTICATED_EMAILS_LIST, session.getSpec().getUser())); }); @@ -372,8 +396,7 @@ protected void createAndApplyProxyConfigMap(String correlationId, String session return; } K8sUtil.loadAndCreateConfigMapWithOwnerReference(client.kubernetes(), client.namespace(), correlationId, - configMapYaml, Session.API, Session.KIND, sessionResourceName, sessionResourceUID, 0, - configMap -> { + configMapYaml, Session.API, Session.KIND, sessionResourceName, sessionResourceUID, 0, configMap -> { String host = arguments.getInstancesHost() + ingressPathProvider.getPath(appDefinition, session); int port = appDefinition.getSpec().getPort(); AddedHandlerUtil.updateProxyConfigMap(client.kubernetes(), client.namespace(), configMap, host, @@ -396,8 +419,7 @@ protected void createAndApplyDeployment(String correlationId, String sessionReso return; } K8sUtil.loadAndCreateDeploymentWithOwnerReference(client.kubernetes(), client.namespace(), correlationId, - deploymentYaml, Session.API, Session.KIND, sessionResourceName, sessionResourceUID, 0, - deployment -> { + deploymentYaml, Session.API, Session.KIND, sessionResourceName, sessionResourceUID, 0, deployment -> { pvName.ifPresent(name -> addVolumeClaim(deployment, name, appDefinition.getSpec())); bandwidthLimiter.limit(deployment, appDefinition.getSpec().getDownlinkLimit(), appDefinition.getSpec().getUplinkLimit(), correlationId); diff --git a/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/impl/LazyStartAppDefinitionHandler.java b/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/impl/LazyStartAppDefinitionHandler.java index 0a3a00c1..20dc7a54 100644 --- a/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/impl/LazyStartAppDefinitionHandler.java +++ b/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/impl/LazyStartAppDefinitionHandler.java @@ -1,5 +1,5 @@ /******************************************************************************** - * Copyright (C) 2022 EclipseSource, Lockular, Ericsson, STMicroelectronics and + * Copyright (C) 2022-2023 EclipseSource, Lockular, Ericsson, STMicroelectronics and * others. * * This program and the accompanying materials are made available under the @@ -41,6 +41,22 @@ public class LazyStartAppDefinitionHandler implements AppDefinitionHandler { @Override public boolean appDefinitionAdded(AppDefinition appDefinition, String correlationId) { + try { + return appDefinitionAdded(appDefinition, correlationId); + } catch (Throwable ex) { + LOGGER.error(formatLogMessage(correlationId, + "An unexpected exception occurred while adding AppDefinition: " + appDefinition), ex); + // TODO update status +// client.appDefinitions().updateStatus(correlationId, appDefinition, status -> +// { +// status.setOperatorStatus(OperatorStatus.ERROR); +// status.setOperatorMessage("Unexpected error. Please check the logs for correlationId " + correlationId); +// }); + return false; + } + } + + protected boolean doAppDefinitionAdded(AppDefinition appDefinition, String correlationId) { LOGGER.info(formatLogMessage(correlationId, "Handling " + appDefinition)); // TODO Check current session status and ignore if handling failed before diff --git a/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/impl/LazyWorkspaceHandler.java b/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/impl/LazyWorkspaceHandler.java index 9d98ea24..819cd789 100644 --- a/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/impl/LazyWorkspaceHandler.java +++ b/java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/impl/LazyWorkspaceHandler.java @@ -1,5 +1,5 @@ /******************************************************************************** - * Copyright (C) 2022 EclipseSource and others. + * Copyright (C) 2022-2023 EclipseSource and others. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v. 2.0 which is available at @@ -44,12 +44,34 @@ public class LazyWorkspaceHandler implements WorkspaceHandler { @Override public boolean workspaceAdded(Workspace workspace, String correlationId) { + try { + return doWorkspaceAdded(workspace, correlationId); + } catch (Throwable ex) { + LOGGER.error(formatLogMessage(correlationId, + "An unexpected exception occurred while adding Workspace: " + workspace), ex); + client.workspaces().updateStatus(correlationId, workspace, status -> { + status.setOperatorStatus(OperatorStatus.ERROR); + status.setOperatorMessage( + "Unexpected error. Please check the logs for correlationId: " + correlationId); + }); + return false; + } + } + + protected boolean doWorkspaceAdded(Workspace workspace, String correlationId) { LOGGER.info(formatLogMessage(correlationId, "Handling " + workspace)); // Check current session status and ignore if handling failed before Optional status = Optional.ofNullable(workspace.getStatus()); String operatorStatus = status.map(ResourceStatus::getOperatorStatus).orElse(OperatorStatus.NEW); + if (OperatorStatus.HANDLED.equals(operatorStatus)) { + LOGGER.trace(formatLogMessage(correlationId, + "Workspace was successfully handled before and is skipped now. Workspace: " + workspace)); + return true; + } if (OperatorStatus.ERROR.equals(operatorStatus) || OperatorStatus.HANDLING.equals(operatorStatus)) { + // TODO In the HANDLING case we should not return but continue where we left + // off. LOGGER.warn(formatLogMessage(correlationId, "Workspace could not be handled before and is skipped now. Current status: " + operatorStatus + ". Workspace: " + workspace)); @@ -62,8 +84,7 @@ public boolean workspaceAdded(Workspace workspace, String correlationId) { }); String storageName = WorkspaceUtil.getStorageName(workspace); - client.workspaces().updateStatus(correlationId, workspace, - s -> s.setVolumeClaim(new StatusStep("started", null))); + client.workspaces().updateStatus(correlationId, workspace, s -> s.setVolumeClaim(new StatusStep("started"))); if (!client.persistentVolumes().has(storageName)) { LOGGER.trace(formatLogMessage(correlationId, "Creating new persistent volume named " + storageName)); @@ -71,8 +92,8 @@ public boolean workspaceAdded(Workspace workspace, String correlationId) { } client.workspaces().updateStatus(correlationId, workspace, s -> { - s.setVolumeClaim(new StatusStep("finished", null)); - s.setVolumeAttach(new StatusStep("started", null)); + s.setVolumeClaim(new StatusStep("finished")); + s.setVolumeAttach(new StatusStep("started")); }); if (!client.persistentVolumeClaims().has(storageName)) { @@ -81,7 +102,7 @@ public boolean workspaceAdded(Workspace workspace, String correlationId) { } client.workspaces().updateStatus(correlationId, workspace, s -> { - s.setVolumeAttach(new StatusStep("claimed", null)); + s.setVolumeAttach(new StatusStep("claimed")); }); LOGGER.trace(formatLogMessage(correlationId, "Set workspace storage " + storageName)); @@ -89,7 +110,7 @@ public boolean workspaceAdded(Workspace workspace, String correlationId) { toEdit -> toEdit.getSpec().setStorage(storageName)); client.workspaces().updateStatus(correlationId, workspace, s -> { - s.setVolumeAttach(new StatusStep("finished", null)); + s.setVolumeAttach(new StatusStep("finished")); }); client.workspaces().updateStatus(correlationId, workspace, s -> {