Improve session pod labels #362
Conversation
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 :)
...eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/util/LabelsUtil.java
Outdated
Show resolved
Hide resolved
...eclipse.theia.cloud.common/src/main/java/org/eclipse/theia/cloud/common/util/LabelsUtil.java
Outdated
Show resolved
Hide resolved
| 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: - 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 <[email protected]>
Signed-off-by: Olaf Lessenich <[email protected]>
There was a problem hiding this comment.
Code changes LGTM me. I have one very minor, non-blocking suggestion inline.
I did not retest the second iteration though. @jfaltermeier Did you test it? Otherwise, I can still do it.
...clipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/util/K8sUtil.java
Outdated
Show resolved
Hide resolved
I did a short test and saw the labels on pods, deployments, services, and config maps |
…rg/eclipse/theia/cloud/operator/util/K8sUtil.java Co-authored-by: Lucas Koehler <[email protected]>
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