search

eclipse-che / che

Kubernetes based Cloud Development Environments for Enterprise Teams
http://eclipse.org/che
Eclipse Public License 2.0
6.99k stars 1.19k forks source link

Support running multiple VS Code extensions in the same sidecar #12395

Closed l0rd closed 5 years ago

l0rd commented 5 years ago

Description

This is something needed to properly support JDT LS extensions. We should be able to define a list of VS Code extensions to run in the same container as an original VS Code extension (e.g. JDT ls).

tsmaeder commented 5 years ago

In my discussion with @benoitf a couple of months ago, the idea was to collect plugins that reference the same docker image and run them on the same container per workspace. As far as the plugins are concerned this should be completely transparent and automatic.

l0rd commented 5 years ago

@tsmaeder +1 Anyway do we need it for Che 7 beta?

tsmaeder commented 5 years ago

Anyway do we need it for Che 7 beta?

Yes, as it seems we do not get the extensions from other sidecars through the plugin system. VSCode-Java scans them in order to find stuff from their package.json. My assumption was that we would get those, if they are visible on the file system.

l0rd commented 5 years ago

Ok thanks @tsmaeder

@ibuziuk @garagatyi we miss that during prioritization. Can you guys help to add it to next sprint planning?

ibuziuk commented 5 years ago

@l0rd we could try to add it. @tsmaeder will you be able to create an issue ?

garagatyi commented 5 years ago

@l0rd @benoitf I recall we were discussing it before the EclipseCon but didn't proceed to have an agreement on how it should be designed. Have you had a chance to agree on that after?

l0rd commented 5 years ago

@ibuziuk this is the issue, why should we open a new one?

@garagatyi we should now have all the missing pieces now and it should be just a matter of deciding how a plugin developer can specify (in meta.yaml or che_plugin.yaml) that its plugins will run in the same container. A simple rule would be that all the plugins that use the same sidecar image should run in the same container.

garagatyi commented 5 years ago

@l0rd got it, thanks

ibuziuk commented 5 years ago

@ibuziuk this is the issue, why should we open a new one?

added to the next sprint prio doc

tsmaeder commented 5 years ago

While I think reusing containers is a good thing, would it be a start to just have multiple theia or vs code plugins in a single Che 7 plugins?

garagatyi commented 5 years ago

@tsmaeder do you mean specifying in meta.yaml:

attributes:
    extensions: "vscode:extension/SonarSource.sonarlint-vscode,vscode:extension/ms-vscode.node-debug"

or 

attributes:
    urls: https://marketplace.visualstudio.com/_apis/public/gallery/publishers/redhat/vsextensions/java/0.38.0/vspackage,https://marketplace.visualstudio.com/_apis/public/gallery/publishers/redhat/vsextensions/vscode-xml/0.3.0/vspackage

? Like bundles of extensions.

garagatyi commented 5 years ago

Format of attribute is ugly, but would be easier to implement without changing structure. On the other hand we could try to implement:

id: blah
version: blah
attributes:
  blah
urls:  
 - https://marketplace.visualstudio.com/_apis/public/gallery/publishers/redhat/vsextensions/java/0.38.0/vspackage
 - https://marketplace.visualstudio.com/_apis/public/gallery/publishers/redhat/vsextensions/vscode-xml/0.3.0/vspackage

or even

id: blah
version: blah
attributes:
  blah
extensions:  
 - vscode:extension/SonarSource.sonarlint-vscode
 - https://marketplace.visualstudio.com/_apis/public/gallery/publishers/redhat/vsextensions/java/0.38.0/vspackage
 - https://marketplace.visualstudio.com/_apis/public/gallery/publishers/redhat/vsextensions/vscode-xml/0.3.0/vspackage
l0rd commented 5 years ago

@garagatyi that's nice

benoitf commented 5 years ago

I will bring also support of plug-ins/theia endpoint in a Docker Image/container. It means that we could avoid to use plug-in broker in that case, vscode extension being directly inside the sidecar image. And of course, we could add several extensions in the same container.

garagatyi commented 5 years ago

@benoitf at the moment we take remote plugin publisher and name from package.json and use it to set env var to Theia container to discover this remote plugin. What should we do in a case of several plugins? Should we inject an env var for each of them?

garagatyi commented 5 years ago

I'm planning to implement next case:

id: blah
version: blah
attributes:
  blah
extensions:  
 - vscode:extension/SonarSource.sonarlint-vscode
 - https://marketplace.visualstudio.com/_apis/public/gallery/publishers/redhat/vsextensions/java/0.38.0/vspackage
 - https://marketplace.visualstudio.com/_apis/public/gallery/publishers/redhat/vsextensions/vscode-xml/0.3.0/vspackage

If you have any objections or would want to go to another direction, please, comment here

benoitf commented 5 years ago

@garagatyi yes

garagatyi commented 5 years ago

using latest new way introduced this week: there is now automatic multicast discovery and auto-port assignment for sidecar so you should just copy plug-ins into /my-plug-ins folder and set THEIA_PLUGINS=local-dir:///my-plug-ins in each sidecar. Main instance of theia will detect endpoint and endpoint will take some random ports automatically

This is not clear for me. If I not expose any ports in the sidecar container how Theia connects to the sidecar? Is there any docs/explanation on how this works(maybe detailed PR description or at lest code)?

benoitf commented 5 years ago

@garagatyi it's working as all containers are in the same pod and then share the same network. (and multicast discovery is possible) https://github.com/eclipse/che-theia/pull/91

garagatyi commented 5 years ago

Oh, I see. I thought we were thinking about splitting workspace to several deployments at some point. Is the 3rd way of sidecars discovery is supposed to be the approach we should implement instead of what we have now with endpoints?

benoitf commented 5 years ago

I would say it's kind of cumulative. If you do know that all sidecars are in the same network: no need to provide remote endpoint URL (and expose the port publicly). If there is a remote sidecar (not in a pod) then it needs to be announced with ENV variable to main instance of theia

benoitf commented 5 years ago

all the mechanisms are still there. But for example in development mode, with discovery, it avoids to declare/define some env variables as it can be automatically detected.

garagatyi commented 5 years ago

@benoitf I see. In a several extensions per sidecar case we have to declare env vars for each extension. Let's imaging we have 3 extensions in the Java Language support Che plugin - JDT.LS, Java debug, Gradle (let it be a separate extension for the example). Then to connect Theia to this endpoint we need to set next env vars:

benoitf commented 5 years ago

you can use also now

for main theia instance: (only one property per sidecar endpoint address)

THEIA_PLUGIN_REMOTE_ENDPOINT_1"="ws://sidecar12345:10234"

for sidecar (sidecar12345:10234)

THEIA_PLUGINS=local-dir:///plug-ins
+ drop com_redhat_jdtls, maven and java debug in that /plug-ins folder (only in that sidecar)
benoitf commented 5 years ago

Am I correct that in this case Theia will properly work with these extensions and will open just 1 websocket connection?

yes, only one websocket connection.

garagatyi commented 5 years ago

Thanks for the details

garagatyi commented 5 years ago

Implementation of automatic collapsing extensions to the same sidecar can be complicated by devfile -> workspace conversion algorithm.

Implementation details for context to help implementing this later:

Here is an example (for demonstration purposes - correct devfile syntax is not preserved): Devfile: ```yaml ... tools: - type: ChePlugin id: myplugin:0.0.1 commands: - name: my command for a specific plugin command: ["bash", "-c", "do cool stuff"] tool: myplugin:0.0.1 ... ``` Then Che 7 code receives plugin info from brokers: ```yaml id: myplugin version: 0.0.1 containers: - image ... ``` It generates machine representation of the container from a plugin and put machine name into command attributes. Now client can match command to machine and execute commands on a proper machine. If we want to put several extensions into the same sidecar then several plugin identifiers should match single sidecar. But brokers send information about sidecar in relation to a specific plugin. To overcome that we can do merge of sidecars on master side but it will split a lot of logic between brokers and master codebases. Another option is to send brokers info to master in a different format. For example: instead of list of plugins with sidecars: ```yaml // plugin - id: myplugin1 version:0.0.1 containers: - image: envVars: - name: value: workspaceEnv: - name: value: - id: anotherplugin version: 0.30.0 containers: - image: envVars: - name: value: workspaceEnv: - name: value: ``` use list of tooling configs that can contain 1..n plugins identifiers and sidecars: ```yaml // tooling entry - plugins: - id: myplugin version: 0.0.1 - id: anotherplugin version: 0.30.0 containers: - image: envVars: - name: value: workspaceEnv: - name: value: // tooling entry - plugins: - id: theia-editor version: 0.0.1 containers: - image: envVars: - name: value: workspaceEnv: - name: value: // tooling entry - plugins: - id: exec version: 0.0.1 containers: - image: envVars: - name: value: workspaceEnv: - name: value: ``` With such a format on master we can do commands matching in next way: 1. command -> plugin 1. plugin from tooling list -> sidecar

@sleshchenko since you implemented commands matching, please, read implementation details and let me know if you agree with the issue I described and possible solution.

At the moment I'm going to finish implementation of plugins that contains several extensions and postpone implementation automatic sidecars configs collapsing. If anyone think that we need to add this feature as part of the current work instead of postponing it for the next time, please, let comment. @ibuziuk @l0rd @benoitf FYI

garagatyi commented 5 years ago

PR https://github.com/eclipse/che/pull/12903 adds an ability to create Che plugin that provisions several VS Code extensions in a single Theia plugin runner sidecar. See description in the PR and here is the demo https://youtu.be/tu8LVNSLLA0 Tried to use it with Java and Java Debug extensions but Java tooling didn't work in workspace for me at all. Plugins are provisioned, Theia reports correct mapping of extensions to endpoints in logs, but something goes wrong. I've just heard that it is known issue that Theia doesn't discover sidecar but I have no details.

garagatyi commented 5 years ago

Here is an issue that explains the situation I'm referring to https://github.com/eclipse/che/issues/12904

garagatyi commented 5 years ago

@tsmaeder I've just merged PR https://github.com/eclipse/che/pull/12903, so you should be able to try it out on nightly tomorrow or today after manual rebuild of Che master image. To test it I used Che plugin entry https://raw.githubusercontent.com/eclipse/che-plugin-registry/shareSidecarWithDebug/plugins/org.eclipse.che.vscode-redhat.java-and-xml:0.38.0 which points to this plugin meta with the following extensions:

garagatyi commented 5 years ago

@benoitf FYI I used the old way of configuring sidecars since reworking it to a new way with extensions files isolated in sidecar container requires refactoring of brokering process and it wasn't the goal of the current scope of the task.

benoitf commented 5 years ago

@garagatyi sure old way is still working, but it was to inform you about future usage.

ibuziuk commented 5 years ago

@l0rd @tsmaeder are you ok to close this issue. For improvements of the current approach, there is already https://github.com/redhat-developer/rh-che/issues/1354

tsmaeder commented 5 years ago

@ibuziuk I don't understand how multicast discovery addresses running multiple plugins in the same container. If you don't think that is a goal to pursue anymore, that's something we can discuss.

ibuziuk commented 5 years ago

@tsmaeder multicast discovery is not related to this issue per se, but improves internal implementation. In Che 7 GA epic multicast discovery is not treated as a subtask of this issue.

ibuziuk commented 5 years ago

Closing as done