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

Theia remote plug-in runtime in sidecar container improvement #13387

Closed
evidolob opened this issue May 22, 2019 · 34 comments
Closed

Theia remote plug-in runtime in sidecar container improvement #13387

evidolob opened this issue May 22, 2019 · 34 comments
Assignees
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.
Milestone

Comments

@evidolob
Copy link
Contributor

Description

Currently we have base image for remote plugin. So all remote plugin images should inherit it, which is not convenient by it self, and heavily increase remote plugin image build time. Also we have some problems when Thiea IDE and remote plugin runtime has different version(when some one doesn't update remote plugin image) which leads to very strange and difficult to detect bugs.

We can improve that with two options:

  1. Add che-theia remote plugin runtime binary at the end of plugin image build
  2. Mount and run che-theia remote plugin runtime binary on che workspace start
@evidolob evidolob added kind/enhancement A feature request - must adhere to the feature request template. team/ide2 labels May 22, 2019
@evidolob
Copy link
Contributor Author

@benoitf WDYT?

@benoitf
Copy link
Contributor

benoitf commented May 22, 2019

@evidolob

yes I think steps are:

  1. Move theia-endpoint outside of dockerfiles folder
  2. Produce all-in-one binary of the package in another foler
  3. @davidfestal is proposing as well to copy binary in a volume so basically we should try to have image without the runtime inside as well. (let's pick up one of the current images) and to provide binary inside a docker image (to allow to grab this binary from a init container)
  4. move the insert of the binary to the end of images instead of inheriting parent endpoint image (so layers are re-used at their best) . (and if David is able to run binary outside we could avoid this step and just prune endpoint from all images)

@AndrienkoAleksandr AndrienkoAleksandr self-assigned this May 28, 2019
@AndrienkoAleksandr AndrienkoAleksandr added the status/in-progress This issue has been taken by an engineer and is under active development. label May 28, 2019
@AndrienkoAleksandr
Copy link
Contributor

AndrienkoAleksandr commented Jun 3, 2019

  1. Move theia-endpoint outside of dockerfiles folder

Seems, done by @tolusha eclipse-che/che-theia#254

  1. Produce all-in-one binary of the package in another foler

I complete investigation.
node.js doesn't has embedded mechanism to build executable image from the box(it's not golang...). So to build executable binary we need to use third party libs. But all of them has own pluses and minuses. To build remote-plugin-endpoint I've got not bad result only with nexe tool, but this tool has own limitation too: doesn't support dynamic require and native node libs(native node libs - it's code written on non node, f.e on the c or c++, but wrapped with help node). So for remote-plugin-endpoint we should not use these features to prevent issues with nexe.
So, To package all-in-one binary I experemented with 4 tools:

pkg - doesn't work for our remote-plugin code, because our code uses getMac dependency(like transitive dependency). pkg unable to resolve such kind of libs, see more vercel/pkg#519

node-packer - remote-plugin-endpoint binary compiled with help this tool doesn't work for alpine images at all (pmq20/node-packer#127), and node-packer executable binary doesn't work under alpine too. On the github repo we can see that source code contains node source code with version 8.3.0, but we need node 10.

appImage - main idea - package remote-plugin with help electron and generate with help electron-builder lib binary in the universal linux binary format - AppImage. But such binaries doesn't work in the docker containers, because appImage uses fuse filesystem, which works for docker containers only with privilege mode. Also fuse filesystem need fuse filesystem libs - I could not find any base linux images where are these dependencies are pre-installed.

nexe - I generated binary with platform flag alpine, but this binary works for all another containers which I tested. I tested this binary with images:

  • registry.access.redhat.com/ubi8/ubi-minimal
  • ubuntu:18.04
  • debian:9
  • fedora:30
  • centos:7
  • busybox
  • node:10.15-alpine
  • opensuse/leap
  • cirros
  • clefos
  • mageia:7
  • photon:latest
  • proton
  • alt
  • amazonlinux
  • open-liberty
  • php
  • oraclelinux
  • golang:stretch
  • alpine

@AndrienkoAleksandr
Copy link
Contributor

All-in-one binary branch https://github.com/eclipse/che-theia/compare/remoteBinary

@AndrienkoAleksandr
Copy link
Contributor

Binary size 136.8Mb

@AndrienkoAleksandr
Copy link
Contributor

AndrienkoAleksandr commented Jun 4, 2019

@davidfestal is proposing as well to copy binary in a volume so basically we should try to have image without the runtime inside as well. (let's pick up one of the current images) and to provide binary inside a docker image (to allow to grab this binary from a init container)

Hello, @davidfestal Did you mean, that we can use plugin-broker to serve remote-plugin-endpoint binary, and Eclipse CHE master could create volume for remote plugin containers and copy binary inside volume?

@davidfestal
Copy link
Contributor

Well, initially my general idea was that we could provide such binaries (or any additional standalone files) in very lightweight containers that would contain:

  • the files to inject,
  • a lightweight cp utility (such as the busybox one)
  • An env variable that indicates where to copy the provided files to inject.

Then it would be added as an initContainer in the workspace POD with a volume and the right env variable.

Such lightweight container images could be either created at build time (for the remote-plugin-endpoint) or even created on-the-fly by the plugin broker (for plugin archives for example).

@AndrienkoAleksandr
Copy link
Contributor

@davidfestal Do you have a plan to implement your initial idea or this part of task should be done by ide2 team?

@davidfestal
Copy link
Contributor

In fact it seems to me that it would be sufficient to simply:

  • add a single optional boolean initContainer field (false by default) on the CheContainer model class,
  • and add all the containers with a true value as init containers in the workspace POD.

All the rest of the work would be done in the plugin broker.

However, if I find some time I might be able to prepare the plugin-broker part of this work and test it with the Che Workspace CRD Operator POC.

@davidfestal
Copy link
Contributor

@l0rd Should I open a separate issue for this ?

@l0rd
Copy link
Contributor

l0rd commented Jun 5, 2019

@davidfestal ok to create a separate issue. But I have some comments:

About the build of the init-container

👍 to have an init container that copies an all-in-one binary in a volume but 👎 to build the init container image during the workspace bootstrap

About the initContainer field

If that means that we should add these containers in the meta.yaml of every vs-code extension or theia plugin I am 👎. If I am a user and want to add a VS Code extension in my workspace I should only provide 2 information:

  • the reference or id to the VS Code Extension
  • the sidecar image that will host my VS Code Extension

I should not care about how the Theia runtime is injected.

I am 👍 if instead there would be no mention of such init containers in the meta.yaml of the plugins: the plugin-broker automatically adds a theia-remote-runtime-copier initContainer for every theia-plugin component that needs to run in a sidecar.

But the specification of the theia-remote-runtime-copier image (what name and version) should be specified as an attribute in the che-theia meta.yaml to guarantee consistency between che-theia and the all-in-one runtime.

And I would call the field isInitContainer rather than initContainer.

About the plugin sidecar

Take into consideration that the sidecar container image that will run the all-in-one binary should match the image used in our default stacks. For example the maven image in the maven stack should be used for both the build/runtime container and the jdt LS sidecar containers: 2 containers based on the same image guarantee consistency and take less time to be pulled.

@davidfestal
Copy link
Contributor

@davidfestal ok to create a separate issue.

Great.

But I have some comments:

About the build of the init-container

👍 to have an init container that copies an all-in-one binary in a volume but 👎 to build the init container image during the workspace bootstrap

OK. As soon as we have the initContainer mechanism in place in the Che server, we are free to decide how and when we build this containers.

About the initContainer field

If that means that we should add these containers in the meta.yaml of every vs-code extension or theia plugin I am 👎. If I am a user and want to add a VS Code extension in my workspace I should only provide 2 information:

  • the reference or id to the VS Code Extension
  • the sidecar image that will host my VS Code Extension

I should not care about how the Theia runtime is injected.

I am 👍 if instead there would be no mention of such init containers in the meta.yaml of the plugins: the plugin-broker automatically adds a theia-remote-runtime-copier initContainer for every theia-plugin component that needs to run in a sidecar.

That was my idea.

But the specification of the theia-remote-runtime-copier image (what name and version) should be specified as an attribute in the che-theia meta.yaml to guarantee consistency between che-theia and the all-in-one runtime.

So will the user have to ensure consistency manually ? or couldn't we use, by default, an image name based on the name of the docker image of the che-theia editor plugin ?

And I would call the field isInitContainer rather than initContainer.

Sure.

About the plugin sidecar

Take into consideration that the sidecar container image that will run the all-in-one binary should match the image used in our default stacks. For example the maven image in the maven stack should be used for both the build/runtime container and the jdt LS sidecar containers: 2 containers based on the same image guarantee consistency and take less time to be pulled.

I don't see how this is related to my proposal. This is already the case and totally depends on what the meta.yaml will contain. The good thing brought by this proposal is that the sidecar container that will run the all-in-one binary doesn't have to have it inside the docker image. It will have it available in a shared volume (having been copied by the new init container). And we will override its entrypoint to run the all-in-one binary at start. So, I assume that, in the Maven case mentioned above, it would even be the base maven image.
Or am I missing some point ?

@AndrienkoAleksandr
Copy link
Contributor

Current implementation state
List pull requests:
eclipse-che/che-theia#410
eclipse-che/che-plugin-registry#210
#14333
eclipse-che/che-plugin-broker#71

To launch remote plugins we have node application https://github.com/eclipse/che-theia/tree/master/extensions/eclipse-che-theia-plugin-remote. For this application we had separated image eclipse/che-theia-endpoint-runtime. And all remote plugins extended this image, but it’s an issue for us.

Proposed enhancements:
In the pulll request eclipse-che/che-theia#410 was introduced changes:
we compile application https://github.com/eclipse/che-theia/tree/master/extensions/eclipse-che-theia-plugin-remote to the binary with help of nexe tool. Compiled binary contains node runtime, that’s why this binary doesn’t need to have pre-installed node in the image.
Also we need pick up this binary to the remote plugin containers and start this binary, let’s call it “remote plugin runtime injection”.
To inject remote plugin binary we need to change plugin-broker model and che-server.

I proposed to define in the plugin meta.yaml PluginPatcher object:

type PluginPatcher struct {
// List init containers
    InitContainers []Container `json:"initContainers" yaml:"initContainers"`

     // List plugin types matched to PluginPatcher
    PluginTypeMatcher []string `json:"pluginTypeMatcher" yaml:"pluginTypeMatcher"`

     // Command to patch original plugin container command
    PluginContainerCommand []string `json:"pluginContainerCommand" yaml:"pluginContainerCommand"`
    // Argument to patch original plugin container arguments
    PluginContainerArgs []string `json:"pluginContainerArgs" yaml:"pluginContainerArgs"`
}

PluginPatcher has initContainers. eclipse/che-theia-endpoint-runtime image with remote binary we are using like init container. Main goal of this init container => copy remote binary to the volume plugins. I proposed to apply PluginPatcher to the ChePlugin:

type ChePlugin struct {
    … 
   PluginPatcher PluginPatcher 
}

Main idea: we are using PluginPatcher with editor ChePlugin. Che server take a look on the list workspace plugins, find editor plugin, get PluginPatcher. Than che-server get list of the init containers from plugin patcher and start them on workspace start. After init container we have copied remote binary in the volume plugins. Than che-server override root container command for vscode and theia plugin containers to start remote binary. When remote plugin containers started, each remote binary in the container with help of env variables detect were is (vscode Extension)/(che-theia plugin), start it and connect to the che-theia editor. remote binary and editor are always compatible by the same code version, because we defined in the meta.yaml of the eclipse/che-theia in the plugin registry compatible che-theia image and remote binary image:

#che-theia meta.yaml definitiion
apiVersion: v3
publisher: eclipse
name: che-theia
...
spec:
  endpoints:
  ....
  pluginPatcher:
    pluginTypeMatcher: ["vs code extension", "theia plugin"]
    pluginContainerCommand: ["sh", "-c"]
    pluginContainerArgs: ["/plugins/remote-launcher/entrypoint.sh"]
    initContainers:
      - name: theia-remote-plugin-launcher
        image: eclipse/che-theia-endpoint-runtime:next
        initContainer: true
        command: ['cp']
        args: ['-rf', '/remote-plugin-launcher', '/plugins/remote-launcher']
        volumes:
          - mountPath: "/plugins"
            name: plugins
  containers:
   - name: theia-ide
     image: "docker.io/eclipse/che-theia:next"
     env:
       ....
     volumes:
       ...
     ports:
       ...
     ...

@sleshchenko @mario @evidolob @davidfestal @benoitf @amisevsk what do you think?

@AndrienkoAleksandr
Copy link
Contributor

AndrienkoAleksandr commented Sep 13, 2019

Third iteration(Add an ability to forcibly mark a volume as ephemeral persistVolume:false):
PR's

Test devfile:
https://gist.github.com/AndrienkoAleksandr/c5bdf9e40db4d30edad4cb07e36f4ec9

@nickboldt
Copy link
Contributor

clearly still ongoing so slip to 7.3.0

@AndrienkoAleksandr
Copy link
Contributor

Detected regression: #14833 Working on fix.

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

10 participants