Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
l0rd opened this issue Jan 11, 2019 · 36 comments
Closed

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

l0rd opened this issue Jan 11, 2019 · 36 comments
Labels
kind/enhancement A feature request - must adhere to the feature request template. severity/P1 Has a major impact to usage or development of the system.

Comments

@l0rd
Copy link
Contributor

l0rd commented Jan 11, 2019

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
Copy link
Contributor

tsmaeder commented Feb 8, 2019

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
Copy link
Contributor Author

l0rd commented Feb 8, 2019

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

@tsmaeder
Copy link
Contributor

tsmaeder commented Feb 25, 2019

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
Copy link
Contributor Author

l0rd commented Feb 25, 2019

Ok thanks @tsmaeder

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

@ibuziuk
Copy link
Member

ibuziuk commented Feb 25, 2019

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

@garagatyi
Copy link

@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
Copy link
Contributor Author

l0rd commented Feb 25, 2019

@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
Copy link

@l0rd got it, thanks

@ibuziuk
Copy link
Member

ibuziuk commented Feb 26, 2019

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

added to the next sprint prio doc

@tsmaeder
Copy link
Contributor

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
Copy link

@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
Copy link

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
Copy link
Contributor Author

l0rd commented Mar 4, 2019

@garagatyi that's nice

@benoitf
Copy link
Contributor

benoitf commented Mar 4, 2019

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.

@skabashnyuk skabashnyuk added the severity/P1 Has a major impact to usage or development of the system. label Mar 6, 2019
@garagatyi
Copy link

@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
Copy link

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
Copy link
Contributor

benoitf commented Mar 7, 2019

@garagatyi yes

  • using old way:
    yes env var for each of them (the system will only use a single connection if there are duplicates)

  • using intermediate new way:
    it's now possible to select plug-ins per container by defining for each sidecar: (and then not have main theia instance seing the plug-ins directly) by only defining THEIA_PLUGINS=local-dir:///plug-ins
    you still define the property with endpoint for each plug-in
    and you drop plug-ins that are for the same sidecar in the same folder and you start only one container
    for example container java will have
    /plug-ins folder with foo.vsix and bar.vsix + THEIA_PLUGINS=local-dir:///plug-ins
    and container python will have
    /plug-ins folder with python.vsix

  • 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

@garagatyi
Copy link

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)?

@garagatyi
Copy link

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
Copy link
Contributor

benoitf commented Mar 12, 2019

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
Copy link
Contributor

benoitf commented Mar 12, 2019

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
Copy link

@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:

  • "THEIA_PLUGIN_REMOTE_ENDPOINT_com_redhat_jdt_ls"="ws://sidecar12345:10234"
  • "THEIA_PLUGIN_REMOTE_ENDPOINT_com_redhat_maven"="ws://sidecar12345:10234"
  • "THEIA_PLUGIN_REMOTE_ENDPOINT_com_redhat_java_debug"="ws://sidecar12345:10234"
    You can see that they are overlapping because values are the same.
    Also imaging that binaries of those plugins are not in Theia container.
    Am I correct that in this case Theia will properly work with these extensions and will open just 1 websocket connection?

@benoitf
Copy link
Contributor

benoitf commented Mar 12, 2019

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
Copy link
Contributor

benoitf commented Mar 12, 2019

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
Copy link

Thanks for the details

@garagatyi
Copy link

garagatyi commented Mar 13, 2019

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:

...
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:

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:

// 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:

// 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
  2. 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
Copy link

PR #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
Copy link

Here is an issue that explains the situation I'm referring to #12904

@garagatyi
Copy link

@tsmaeder I've just merged PR #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
Copy link

@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
Copy link
Contributor

benoitf commented Mar 18, 2019

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

@ibuziuk
Copy link
Member

ibuziuk commented May 13, 2019

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

@tsmaeder
Copy link
Contributor

@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
Copy link
Member

ibuziuk commented May 16, 2019

@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
Copy link
Member

ibuziuk commented May 16, 2019

Closing as done

@ibuziuk ibuziuk closed this as completed May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template. severity/P1 Has a major impact to usage or development of the system.
Projects
None yet
Development

No branches or pull requests

6 participants