Improve session pod labels #362
Conversation
org.eclipse.theia.cloud.common.util.LabelsUtil is added to encapsulate creation of labels. To indicate that a pod is a user session, its 'app.kubernetes.io/component' label is set to 'session'. For custom labels, we use the prefix 'theia-cloud.io'. This is currently used for: - Identifying a session pods associated with a user: 'theia-cloud.io/user' = '$username' - Identifying a session pod by appdefinition name: 'theia-cloud.io/app-definition' = $appdefinitionname Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
There was a problem hiding this comment.
Hi @xai ,
Thanks for this improvement :) Generally the changes look pretty good to me already. I have a few small remarks inline. Please have a look :)
| 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]; | ||
| labels.put(LABEL_KEY_USER, userName); | ||
| labels.put(LABEL_KEY_APPDEF, appDefinitionSpec.getName()); | ||
| return labels; | ||
| } |
There was a problem hiding this comment.
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?
| 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]; |
There was a problem hiding this comment.
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.
| try { | ||
| client.services().inNamespace(client.namespace()).withName(serviceToUse.get().getMetadata().getName()) | ||
| .edit(service -> { | ||
| LOGGER.info("Setting pod labels"); |
There was a problem hiding this comment.
I think this should only be a debug level log as nothing exiting or too relevant happens here functionality wise.
| LOGGER.info("Setting pod labels"); | |
| LOGGER.debug("Setting service labels"); |
| @@ -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"); | |||
There was a problem hiding this comment.
I think this should only be a debug level log as nothing exiting or too relevant happens here functionality wise.
| LOGGER.info("Setting pod labels"); | |
| LOGGER.debug("Setting pod labels"); |
|
I tested the current state and the labels are getting added as expected 👍 At the moment, in the In the long run, I think we should consider adding the same labels to all deployments, services, and configmaps that are created. For the
We could pass label maps to these methods now, and they would forward them to:
I think we may also ignore the |
I agree: Eventually we should label all components created by Theia Cloud, either by the operator or the Helm charts.
I think we should leave it in but make very clear in the documentation on the website, the helm charts and potentially here that the implementation is not up to par. My reason for leaving it in is that it might encourage contributions to improve this and shows our intent on bringing this up to par eventually. Furthermore, it allows basic prototyping of eager start. WDYT? |
Sounds good |
org.eclipse.theia.cloud.common.util.LabelsUtil is added to encapsulate creation of labels.
To indicate that a pod is a user session, its
'app.kubernetes.io/component' label is set to 'session'.
For custom labels, we use the prefix 'theia-cloud.io'.
This is currently used for:
#366