Extending DevWorkspace Status fields

[amisevsk]

Is your feature request related to a problem? Please describe.

The fields available in a DevWorkspace's status are currently

status:
  conditions: <conditions list>
  ideUrl: <string>
  message: <string>
  phase: <string>
  workspaceId: <string>

However, a devfile will generally contain multiple endpoints, which is unsupported by the singular ideUrl field. This poses a problem for consumers of devfiles such as Theia, as there's no straightforward way to match an endpoint in the devfile with a URL from a route/ingress created on the cluster. Additionally, the ideUrl field is specific to a few use cases, and should be removed/renamed to reflect a more generic usage. We should:

• Deprecate/remove/rename .status.ideUrl (perhaps to .status.mainUrl?)
• Define a way to mark a specific endpoint in the devfile as our "main" entrypoint
  • Currently, the DWO looks for an endpoint with the attribute type: ide, which is clearly not useful for everyone.
• Represent all URLs created for a devfile in the status, preferably linked to endpoints/components, so that we can take an endpoint in the devfile and resolve the URL that points to it.

Describe the solution you'd like

Two options come to mind for me:

Option 1: Represent just URLs in status

This is a straightforward addition of endpoint URLs to the status. This option is cleaner and simpler to read.

status:
  # other fields...
  mainUrl: <url-of-endpoint-with-specific-attribute?>
  endpoints:
    - name: <endpoint-name>
      component: <component-name>
      url: <url>

Option 2: Represent component statuses in status, with subfield for endpoints

As opposed to option 1, this approach would be more easily extensible if we want to represent additional information about components in the devfile in the future (e.g. if we decide to also propagate container statuses for each container component?)

status:
  # other fields...
  mainUrl: <url-of-endpoint-with-specific-attribute?>
  componentStatuses:
    - name: <component-name>
      endpoints:
        - name: <endpoint-name>
          url: <url>

[amisevsk]

cc: @benoitf @davidfestal @l0rd @sleshchenko

[benoitf]

from consumer perspective within che-theia I prefer option 2 because it's a direct mapping of components ( component statuses) ( less loops in client's code)
would be good to have type, targetPort and exposure in the endpoint status

And in component status it would be nice to know if component was defined in 'original' devfile or being added by 3rd party ( boolean attribute telling if it's a user defined or 3rd party component by filtering components by some labels)

About main Url I'm even ok if it's removed ( because for example Che dashboard or Che theia know which terminal type to look) but I agree it's helpful when using 'kubectl get dw' command for example
So if we could provide to devworkspace operator the default type to look for ( as you suggested) I would say it's good
In case of Che : 'ide' type

[amisevsk]

would be good to have type, targetPort and exposure in the endpoint status

I'm generally opposed to duplicating that information from the spec -- at a certain point we risk printing large pieces of the devfile twice. The information we add to the status should be
a) fields that are defined at runtime (e.g. URLs)
b) minimal in the sense that there's no non-crucial information there

About main Url I'm even ok if it's removed ( because for example Che dashboard or Che theia know which terminal type to look) but I agree it's helpful when using 'kubectl get dw' command for example
So if we could provide to devworkspace operator the default type to look for ( as you suggested) I would say it's good
In case of Che : 'ide' type

Yeah, the kubectl get dw output kind of requires something like ideUrl or mainUrl to work nicely (since it just reproduces a field from the status). My thinking was to either specify a non-attribute field on endpoint, e.g. mainEndpoint: true (and enforce a max of one such setting) or to define an attribute (whether it's type: main, or mainEndpoint: true) that signifies that an endpoint should be used as the mainUrl. I don't think there's an elegant way to use different values of a type argument for this purpose though (e.g. specifying you want the type: ide url to fill mainUrl.

[benoitf]

From che-theia perspective we'll have to merge all information so if status contains everything it's nice to me ( or if that kind of merged result is provided in a mounted file). If it's not then Che theia and maybe others client will have to merge data / iterate over two struct
Because for example for ports plugin you want to know what is the url for a given port number
Just knowing the name of the component or the name of the endpoint is not enough, you've to know for which port it was

And for private endpoint there is no URL but then it's listed in devfile spec but it means it won't show up in the status ( which is strange when you consume it from a client point of view)

so if status could avoid iterating/looping again and again in clients it would be helpful
If not then clients will process/iterate over structs while the could have all the information in one step

mainEndpoint: true looks the more flexible solution ( using ide type is meaningful and changing it to 'main' just to be displayed in a status looks odd.

[metlos]

What would be the behavior of mainEndpoint: true if it was defined on some component in the devfile and then, at deployment time, it was also defined by some 3rd party component injected into the workspace (like che-theia)?

[benoitf]

@metlos --> an error if more than one is defined ?

[amisevsk]

It's a hard problem to answer -- e.g. an odo devfile could have a mainEndpoint (or ideUrl in the current implementation) that's required when the application is deployed, but when we add an IDE to that devfile we need to define a new mainEndpoint. It's hard to say how to resolve this, but it is a problem in the current design as well.

[amisevsk]

I understand the goal of having the status hold bookkeeping info and how that makes it easier to use on the consumer-side, but I also think it clutters the status greatly, which is why I'm against it. It's not terribly costly to e.g. define functions getEndpointByPort, getEndpointByName, which realistically should be library functions for devfile/api.

Another option since (at least for our main use case currently) the main piece of data we're tying back to the devfile is the URL: we could also set the URL directly in the DevWorkspace. This is e.g. what k8s services do (.spec.clusterIP gets set after creation). Approaches to this could be

Using attributes

- name: theia-ide
  container:
    image: "quay.io/eclipse/che-theia:next"
    endpoints:
      - name: "theia"
        attributes:
          type: ide
          controller.devfile.io/url: <resolved URL for endpoint>

Using a new field on endpoint

- name: theia-ide
  container:
    image: "quay.io/eclipse/che-theia:next"
    endpoints:
      - name: "theia"
        url: <resolved URL for endpoint>
        attributes:
          type: ide

[sleshchenko]

And for private endpoint there is no URL but then it's listed in devfile spec but it means it won't show up in the status ( which is strange when you consume it from a client point of view)

Really?! no exposure endpoint should have localhost:port url, when internal exposure endpoint should have service-name.namespace.svc I think.

This is e.g. what k8s services do (.spec.clusterIP gets set after creation).

I like the idea but there is an issue that in K8s service it's fully mutable, on our case it can be changed after routingClass is changed.
Also, I must remove spec.ClusterIP when I export service into yaml file, the same here.
Also, do we want to see URL in the Devfile editor on dashboard? Or each user-friendly client should filter it before exporting?

[amisevsk]

Also, do we want to see URL in the Devfile editor on dashboard? Or each user-friendly client should filter it before exporting?

Depends on what the dashboard shows -- my assumption would be that it's the devfile (i.e. without plugins, etc), so it would take some work to propagate this info into the dashboard in the first place. If we're just showing DevWorkspaces, the editor would have to do quite a bit of work to remove meaningless (to the user) metadata fields, etc.

[l0rd]

The proposal we discussed today, at least to solve the "there's no straightforward way to match an endpoint in the devfile with a URL from a route/ingress created on the cluster" problem is to add a flattenedDevfileConfigMap.

Flattened Devfile ConfigMap reference

status:
  # other fields...
  flattenedDevfileConfigMap: <configmapname>

[amisevsk]

Worth noting that the configmap name is derived directly from workspaceID: it's named <workspaceID>-metadata. I agree that it would be useful to add this to the status, but wonder if that's not too implementation-specific for the status API.

[sleshchenko]

Also, flattened devfile is not really devfile but DevWorkspaceTemplateSpec

when devfile is DevWorkspaceTemplateSpecContent + Devfile Header https://github.com/devfile/api/blob/master/pkg/apis/workspaces/v1alpha2/devfile.go#L10