eclipse-theia / theia-cloud

Eclipse Public License 2.0

61 stars 34 forks source link

Improve session pod labels #362

Open xai opened 2 weeks ago

xai commented 2 weeks ago

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

366

jfaltermeier commented 1 week ago

I tested the current state and the labels are getting added as expected :+1:

At the moment, in the EagerStartSessionHandler, we’re only adjusting the service labels, and in the LazySessionHandler, labels are only being added to deployments.

In the long run, I think we should consider adding the same labels to all deployments, services, and configmaps that are created. For the LazySessionHandler, this would mean updating the following:

createAndApplyService
createAndApplyEmailConfigMap
createAndApplyProxyConfigMap
createAndApplyDeployment

We could pass label maps to these methods now, and they would forward them to:

loadAndCreateServiceWithOwnerReference
loadAndCreateConfigMapWithOwnerReference
loadAndCreateDeploymentWithOwnerReference

I think we may also ignore the EagerStartSessionHandler for now. I am not sure if we should remove this handler for 1.0.0, since it does not cover all functionality anyway.

lucas-koehler commented 1 week ago

In the long run, I think we should consider adding the same labels to all deployments, services, and configmaps that are created.

I agree: Eventually we should label all components created by Theia Cloud, either by the operator or the Helm charts.

I think we may also ignore the EagerStartSessionHandler for now. I am not sure if we should remove this handler for 1.0.0, since it does not cover all functionality anyway.

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?

jfaltermeier commented 1 week ago

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