Skip to content

Commit

Permalink
Fix lastActivity not being initialized properly
Browse files Browse the repository at this point in the history
We need to set it after the resource has been created.
Therefore it is now handled by the operator not the service.
Add warning in case resources are edited before being applied to the cluster.
Only check HANDLED sessions from the activity tracking.
Other resources should be cleaned up differently.
  • Loading branch information
sgraband committed Sep 25, 2024
1 parent 642504c commit e48ddf3
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@ public Session create(String correlationId, SessionSpec spec) {
metadata.setName(spec.getName());
session.setMetadata(metadata);

updateStatus(correlationId, session, status -> status.setLastActivity(Instant.now().toEpochMilli()));

info(correlationId, "Create Session " + session.getSpec());
return operation().resource(session).create();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ default T edit(String correlationId, String name, Consumer<T> consumer) {
info(correlationId, "Edit " + name);
Resource<T> resource = resource(name);
if (resource.get() == null) {
warn(correlationId, "Resource not found! Could not edit " + name
+ ". Was this called before the resource has been created?");
return null;
}
return resource.edit(JavaUtil.toUnary(consumer));
Expand All @@ -67,6 +69,8 @@ default T editStatus(String correlationId, String name, Consumer<T> consumer) {
trace(correlationId, "Edit status of " + name);
Resource<T> resource = resource(name);
if (resource.get() == null) {
warn(correlationId, "Resource " + name
+ " not found. Could not update the status. Note that the status of a resource cannot be changed before it is created on the cluster.");
return null;
}
return resource.editStatus(JavaUtil.toUnary(consumer));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import java.io.IOException;
import java.net.URISyntaxException;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -191,6 +192,7 @@ protected boolean doSessionAdded(Session session, String correlationId) {
client.sessions().updateStatus(correlationId, session, s -> {
s.setOperatorStatus(OperatorStatus.HANDLED);
s.setOperatorMessage("Service already exists.");
s.setLastActivity(Instant.now().toEpochMilli());
});
// TODO do not return true if the sessions was in handling state at the start of
// this handler
Expand Down Expand Up @@ -218,6 +220,7 @@ protected boolean doSessionAdded(Session session, String correlationId) {
client.sessions().updateStatus(correlationId, session, s -> {
s.setOperatorStatus(OperatorStatus.HANDLED);
s.setOperatorMessage("Configmaps already exist.");
s.setLastActivity(Instant.now().toEpochMilli());
});
// TODO do not return true if the sessions was in handling state at the start of
// this handler
Expand All @@ -237,6 +240,7 @@ protected boolean doSessionAdded(Session session, String correlationId) {
client.sessions().updateStatus(correlationId, session, s -> {
s.setOperatorStatus(OperatorStatus.HANDLED);
s.setOperatorMessage("Deployment already exists.");
s.setLastActivity(Instant.now().toEpochMilli());
});
return true;
}
Expand Down Expand Up @@ -275,6 +279,7 @@ protected boolean doSessionAdded(Session session, String correlationId) {

client.sessions().updateStatus(correlationId, session, s -> {
s.setOperatorStatus(OperatorStatus.HANDLED);
s.setLastActivity(Instant.now().toEpochMilli());
});
return true;
}
Expand Down Expand Up @@ -351,9 +356,8 @@ protected Optional<String> getStorageName(Session session, String correlationId)
if (!session.getSpec().getUser().equals(workspace.get().getSpec().getUser())) {
// the workspace is owned by a different user. do not mount and go ephemeral
// should get prevented by service, but we need to be sure to not expose data
LOGGER.error(formatLogMessage(correlationId,
"Workspace is owned by " + workspace.get().getSpec().getUser() + ", but requesting user is "
+ session.getSpec().getUser()));
LOGGER.error(formatLogMessage(correlationId, "Workspace is owned by " + workspace.get().getSpec().getUser()
+ ", but requesting user is " + session.getSpec().getUser()));
return Optional.empty();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,12 @@
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.eclipse.theia.cloud.common.k8s.client.TheiaCloudClient;
import org.eclipse.theia.cloud.common.k8s.resource.OperatorStatus;
import org.eclipse.theia.cloud.common.k8s.resource.appdefinition.AppDefinition;
import org.eclipse.theia.cloud.common.k8s.resource.session.Session;
import org.eclipse.theia.cloud.operator.TheiaCloudOperatorArguments;
Expand Down Expand Up @@ -78,7 +80,10 @@ public void start() {
}

protected void pingAllSessions() {
List<Session> sessions = resourceClient.sessions().list();
// Only look at handled sessions (handled sessions have a lastActivity)
List<Session> sessions = resourceClient.sessions().list().stream()
.filter(session -> OperatorStatus.HANDLED.equals(session.getStatus().getOperatorStatus()))
.collect(Collectors.toList());
String correlationId = generateCorrelationId();

LOGGER.debug("Pinging sessions: " + sessions);
Expand Down

0 comments on commit e48ddf3

Please sign in to comment.