Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve session pod labels #362

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improve session pod labels #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions ...ipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/util/LabelsUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package org.eclipse.theia.cloud.common.util;

import java.util.HashMap;
import java.util.Map;

import org.eclipse.theia.cloud.common.k8s.resource.appdefinition.AppDefinitionSpec;
import org.eclipse.theia.cloud.common.k8s.resource.session.SessionSpec;

public class LabelsUtil {
public static final String LABEL_CUSTOM_PREFIX = "theia-cloud.io";

public static final String LABEL_KEY_SESSION = "app.kubernetes.io/component";
public static final String LABEL_VALUE_SESSION = "session";

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 Map<String, String> createSessionLabels(SessionSpec sessionSpec, AppDefinitionSpec appDefinitionSpec) {
Map<String, String> labels = new HashMap<>();
labels.put(LABEL_KEY_SESSION, LABEL_VALUE_SESSION);
String userName = sessionSpec.getUser().split("@")[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we remove the second part of the email address (in case the user is an email address)? I would not do this because in deployments with a larger user base this can lead to collisions quite easily.

labels.put(LABEL_KEY_USER, userName);
labels.put(LABEL_KEY_APPDEF, appDefinitionSpec.getName());
return labels;
}
Comment on lines +18 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we already add the app.kubernetes.io/component annotation, I think we should also add the app.kubernetes.io/part-of annotation with value theia-cloud.
This is helpful if Theia Cloud is installed together with other services in the same namespace. When we eventually apply this to all of Theia Cloud's components, this provides a selector to get everything related to Theia Cloud.

What do you think?

}
22 changes: 22 additions & 0 deletions .../main/java/org/eclipse/theia/cloud/operator/handler/session/EagerStartSessionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import java.util.Collections;
import java.util.List;
import java.util.Map.Entry;
import java.util.Map;
import java.util.HashMap;
import java.util.Optional;

import org.apache.logging.log4j.LogManager;
Expand All @@ -30,6 +32,7 @@
import org.eclipse.theia.cloud.common.k8s.resource.session.Session;
import org.eclipse.theia.cloud.common.k8s.resource.session.SessionSpec;
import org.eclipse.theia.cloud.common.util.JavaUtil;
import org.eclipse.theia.cloud.common.util.LabelsUtil;
import org.eclipse.theia.cloud.operator.TheiaCloudOperatorArguments;
import org.eclipse.theia.cloud.operator.handler.AddedHandlerUtil;
import org.eclipse.theia.cloud.operator.ingress.IngressPathProvider;
Expand Down Expand Up @@ -115,6 +118,25 @@ public boolean sessionAdded(Session session, String correlationId) {
return false;
}

try {
client.services().inNamespace(client.namespace()).withName(serviceToUse.get().getMetadata().getName())
.edit(service -> {
LOGGER.info("Setting pod labels");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should only be a debug level log as nothing exiting or too relevant happens here functionality wise.

Suggested change
LOGGER.info("Setting pod labels");
LOGGER.debug("Setting service labels");

Map<String, String> labels = service.getMetadata().getLabels();
if (labels == null) {
labels = new HashMap<>();
service.getMetadata().setLabels(labels);
}
Map<String, String> newLabels = LabelsUtil.createSessionLabels(spec, appDefinition.get().getSpec());
labels.putAll(newLabels);
return service;
});
} catch (KubernetesClientException e) {
LOGGER.error(formatLogMessage(correlationId,
"Error while adding labels to service " + (serviceToUse.get().getMetadata().getName())), e);
return false;
}

/* get the deployment for the service and add as owner */
Integer instance = TheiaCloudServiceUtil.getId(correlationId, appDefinition.get(), serviceToUse.get());
if (instance == null) {
Expand Down
12 changes: 12 additions & 0 deletions ...or/src/main/java/org/eclipse/theia/cloud/operator/handler/session/LazySessionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.HashMap;
import java.util.Optional;

import org.apache.logging.log4j.LogManager;
Expand All @@ -39,6 +40,7 @@
import org.eclipse.theia.cloud.common.k8s.resource.session.SessionSpec;
import org.eclipse.theia.cloud.common.k8s.resource.session.SessionStatus;
import org.eclipse.theia.cloud.common.k8s.resource.workspace.Workspace;
import org.eclipse.theia.cloud.common.util.LabelsUtil;
import org.eclipse.theia.cloud.common.util.TheiaCloudError;
import org.eclipse.theia.cloud.common.util.WorkspaceUtil;
import org.eclipse.theia.cloud.operator.TheiaCloudOperatorArguments;
Expand Down Expand Up @@ -444,6 +446,16 @@ protected void createAndApplyDeployment(String correlationId, String sessionReso
}
K8sUtil.loadAndCreateDeploymentWithOwnerReference(client.kubernetes(), client.namespace(), correlationId,
deploymentYaml, Session.API, Session.KIND, sessionResourceName, sessionResourceUID, 0, deployment -> {

LOGGER.info("Setting pod labels");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should only be a debug level log as nothing exiting or too relevant happens here functionality wise.

Suggested change
LOGGER.info("Setting pod labels");
LOGGER.debug("Setting pod labels");

Map<String, String> labels = deployment.getSpec().getTemplate().getMetadata().getLabels();
if (labels == null) {
labels = new HashMap<>();
deployment.getSpec().getTemplate().getMetadata().setLabels(labels);
}
Map<String, String> newLabels = LabelsUtil.createSessionLabels(session.getSpec(), appDefinition.getSpec());
labels.putAll(newLabels);

pvName.ifPresent(name -> addVolumeClaim(deployment, name, appDefinition.getSpec()));
bandwidthLimiter.limit(deployment, appDefinition.getSpec().getDownlinkLimit(),
appDefinition.getSpec().getUplinkLimit(), correlationId);
Expand Down