Skip to content

Commit

Permalink
Fix eager session delete handling for already removed owner references
Browse files Browse the repository at this point in the history
- Extend LabelsUtil to add the session name to session labels
- Find service from session via labels instead of owner references in eager session delete handler
  With this, the handling also works if the owner reference has been removed automatically
- Add minor clarifying comments
  • Loading branch information
lucas-koehler committed Jan 14, 2025
1 parent f41e4e8 commit 99dd2c8
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public class LabelsUtil {

public static final String LABEL_KEY_USER = LABEL_CUSTOM_PREFIX + "/user";
public static final String LABEL_KEY_APPDEF = LABEL_CUSTOM_PREFIX + "/app-definition";
public static final String LABEL_KEY_SESSION_NAME = LABEL_CUSTOM_PREFIX + "/session";

public static Map<String, String> createSessionLabels(SessionSpec sessionSpec,
AppDefinitionSpec appDefinitionSpec) {
Expand All @@ -27,15 +28,16 @@ public static Map<String, String> createSessionLabels(SessionSpec sessionSpec,
String sanitizedUser = sessionSpec.getUser().replaceAll("@", "_at_").replaceAll("[^a-zA-Z0-9]", "_");
labels.put(LABEL_KEY_USER, sanitizedUser);
labels.put(LABEL_KEY_APPDEF, appDefinitionSpec.getName());
labels.put(LABEL_KEY_SESSION_NAME, sessionSpec.getName());
return labels;
}

/**
* Returns the set of label keys that are specific to a specific session, i.e. the user key.
* Returns the set of label keys that are specific to a specific session, i.e. the user and session name keys.
*
* @return The session specific label keys.
*/
public static Set<String> getSessionSpecificLabelKeys() {
return Set.of(LABEL_KEY_USER);
return Set.of(LABEL_KEY_SESSION_NAME, LABEL_KEY_USER);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ public boolean sessionAdded(Session session, String correlationId) {
// This is the case because ConfigMap changes are not propagated to the pod immediately but during a
// periodic sync. See
// https://kubernetes.io/docs/concepts/configuration/configmap/#mounted-configmaps-are-updated-automatically
// NOTE that this is still not a one hundred percent guarantee that the pod is updated in time.
try {
LOGGER.info(formatLogMessage(correlationId, "Adding update annotation to pods..."));
client.kubernetes().pods().list().getItems().forEach(pod -> {
Expand Down Expand Up @@ -327,29 +328,32 @@ public boolean sessionDeleted(Session session, String correlationId) {
String sessionResourceName = session.getMetadata().getName();
String sessionResourceUID = session.getMetadata().getUid();
Map<String, String> sessionLabels = LabelsUtil.createSessionLabels(spec, appDefinitionSpec);
String namespace = session.getMetadata().getNamespace();
// Filtering by withLabels(sessionLabels) because the method requires an exact match of the labels.
// Additional labels on the service prevent a match and the service has an additional app label.
// Thus, filter by each session label separately.
// We rely on the fact that the session labels are unique for each session.
// We cannot rely on owner references because they might have been cleaned up automatically by Kubernetes.
// While this should not happen, it did on Minikube.
FilterWatchListDeletable<Service, ServiceList, ServiceResource<Service>> servicesFilter = client.services();
for (Entry<String, String> entry : sessionLabels.entrySet()) {
servicesFilter = servicesFilter.withLabel(entry.getKey(), entry.getValue());
}
List<Service> services = servicesFilter.list().getItems();
Optional<Service> ownedService = TheiaCloudServiceUtil.getServiceOwnedBySession(sessionResourceName,
sessionResourceUID, services);

if (ownedService.isEmpty()) {
LOGGER.error(
formatLogMessage(correlationId, "No Service owned by session " + sessionResourceName + " found."));
if (services.isEmpty()) {
LOGGER.error(formatLogMessage(correlationId, "No Service owned by session " + spec.getName() + " found."));
return false;
} else if (services.size() > 1) {
LOGGER.error(formatLogMessage(correlationId,
"Multiple Services owned by session " + spec.getName() + " found. This should never happen."));
return false;
}
String serviceName = ownedService.get().getMetadata().getName();
Service ownedService = services.get(0);
String serviceName = ownedService.getMetadata().getName();

// Remove owner reference and user specific labels from the service
Service cleanedService;
try {
cleanedService = client.services().inNamespace(namespace).withName(serviceName).edit(service -> {
cleanedService = client.services().withName(serviceName).edit(service -> {
TheiaCloudHandlerUtil.removeOwnerReferenceFromItem(correlationId, sessionResourceName,
sessionResourceUID, service);
service.getMetadata().getLabels().keySet().removeAll(LabelsUtil.getSessionSpecificLabelKeys());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ public static <T extends HasMetadata> T addOwnerReferenceToItem(String correlati
return item;
}

/**
* Removes the owner reference from the item if it is present. Does nothing otherwise.
*/
public static <T extends HasMetadata> T removeOwnerReferenceFromItem(String correlationId,
String sessionResourceName, String sessionResourceUID, T item) {
LOGGER.info(
Expand Down

0 comments on commit 99dd2c8

Please sign in to comment.