Implement remote plugin runtime injection

[AndrienkoAleksandr]

What does this PR do?

Implement remote plugin runtime injection with help of init container…, plugin container command and arguments

What issues does this PR fix or reference?

redhat-developer/rh-che#1449
Needed for eclipse-che/che#13387

Signed-off-by: Oleksandr Andriienko oandriie@redhat.com

[codecov]

Codecov Report

Merging #71 into master will increase coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
+ Coverage   67.06%   67.22%   +0.16%     
==========================================
  Files           6        6              
  Lines         589      592       +3     
==========================================
+ Hits          395      398       +3     
  Misses        168      168              
  Partials       26       26

Impacted Files	Coverage Δ
brokers/unified/broker.go	83.33% <100%> (+0.18%)	⬆️
brokers/unified/vscode/broker.go	74.07% <100%> (+0.13%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c864a10...272c376. Read the comment docs.

[amisevsk]

LGTM

[davidfestal]

It seems to me that we might need to have a bit more discussion on the underlying meta.yaml and plugin model changes before merging this.

[davidfestal]

So I assume adding the type here is only a way to forward the type (defined in the meta.yaml) to the Che server in order to use it in the Che server Kubernetes infrastructure ?

[davidfestal]

What is the goal of this addition and how is it managed by the plugin broker ?

[AndrienkoAleksandr]

Command and args defines root process entrypoint container command. We could declare here command, for example, to copy remote binary from container to volume. Currently plugin broker doesn't manage it, this logic was implemented in the che-server...

[davidfestal]

Why isn't the plugin patcher managed by the plugin broker directly ?
In such a case, only the Container object would be modified (possibly to add a isInitContainer boolean flag), no?

[sleshchenko]

possibly to add a isInitContainer boolean flag

I believe we try to be as close to K8s API as possible. So, accordingly to K8s API it would be better to introduce new field initContainers instead of just adding a boolean flag.

Why isn't the plugin patcher managed by the plugin broker directly ?

I believe that the current implementation brings a consistent picture(I'm not talking about fields naming but about idea in general). Let me explain my thought:
In the current implementation one place - theia.meta.yaml has declarative configuration for plugin patching:

  pluginPatcher:
    pluginTypeMatcher: ["vs code extension", "theia plugin"]
    pluginContainerCommand: ["sh", "-c"]
    pluginContainerArgs: ["/plugins/remote-launcher/entrypoint.sh"]
    initContainers:
      - name: theia-remote-plugin-laucher
        image: aandrienko/che-theia-endpoint-runtime:next
        initContainer: true
        command: ['cp']
        args: ['-rf', '/remote-plugin-launcher', '/plugins/remote-launcher']
        volumes:
          - mountPath: "/plugins"
            name: plugins

More see https://github.com/eclipse/che-plugin-registry/pull/210/files

In case of adding only just initContainers to meta.yaml - we would have such configuration in different places:
like theia.meta.yaml

    - name: theia-remote-plugin-laucher
      image: aandrienko/che-theia-endpoint-runtime:next
      initContainer: true
      volumes:
      - mountPath: "/plugins"
        name: plugins

Then Dockerfile of che-theia-endpoint-runtime:next would have cp rf /remote-plugin-launcher /plugins/remote-launcher.
And plugin-broker should (via configuration or hard-coded value) would have

    pluginTypeMatcher: ["vs code extension", "theia plugin"]
    pluginContainerCommand: ["sh", "-c"]
    pluginContainerArgs: ["/plugins/remote-launcher/entrypoint.sh"]

And we can easily imagine a situation when vscode and remote plugin need different contianers entrypoints, or even more difficult situation - different entrypoint according to che-theia-endpoint-runtime container version, like che-theia-endpoint-runtime: next -> /plugins/remote-launcher/entrypoint.sh and che-theia-endpoint-runtime: 7.0.0 -> /plugins/remote-launcher/launch ${THEIA_PLUGIN_FILE_PATH}

Another case, what if a workspace does not have any vscode or remote plugin, in the current solution init container won't be propagated because of a matcher.
In case of moving this logic to plugin broker - it should delete this information from theia.meta.yaml what is like hidden logic, or run initContainer always even if it is redundant.

The current solution allows solving these issue quite nicely.

P.S. Hope I explained my thought clearly and feel free to reask if something is unclear or missing
P.S.S. I'm quite OK with introducing only initContainers in meta.yaml but it does not look as clear solution and it is needed to understand why we accept this solution: like keep meta.yaml as simple as possible (but it would not make things clearer in general), or because we don't have much time to implement something else...

[AndrienkoAleksandr]

+1 To have pluginPatcher configuration in one place.

[davidfestal]

I'm a bit worried with this approach, since it changes the schema of the plugin meta.yaml defintion to manage a very specific case (provide the ability for a given plugin to patch other plugins). It seems to me that we push logic that is implementation-dependent into a format that should be declarative.

Adding the concept of initConainers in the definition of a plugin is a very good thing IMO because it's a very general, and kubernetes standard, and easy to understand to a user.

BUT

The concept of some logic that should be triggered by the presence of 1 plugin for other plugins seems not straightforward to grasp for the meta.yaml reader IMO, and makes the schema (== spec ?) of the meta.yaml very implementation-dependent.

Let's not forget that plugin definitions are in fact a part of a Devfile (definition of the workspace structure). IMO we should keep the Devfile format (as well as the format of the plugin defintions it references) as much declarative and simple as possible.

[sleshchenko]

Adding the concept of initConainers in the definition of a plugin is a very good thing IMO because it's a very general, and kubernetes standard, and easy to understand to a user.

Totally agree with that. And here

I believe we try to be as close to K8s API as possible. So, accordingly to K8s API it would be better to introduce new field initContainers instead of just adding a boolean flag.

I tried to explain that I prefer initContainer field instead of adding initContainer flag into a container entity model.

I think it makes sense to separate and merge changes we all are agreed:

1. Add an ability to add init containers in meta.yaml
2. Add an ability to override command and args
3. Add an ability to forcibly mark a volume as ephemeral (emptyDir, persistVolume:false).

So, I propose to do the following changes independently from pluginMatcher discussions: change meta.yaml or put all logic into che-plugin-broker.

apiVersion: v2
publisher: eclipse
...
spec:
  endpoints:
   -  name: "theia"
      ...
  initContainers:
    - name: theia-remote-plugin-launcher
      image: eclipse/che-theia-endpoint-runtime:next
      command: ['cp']
      args: ['-rf', '/remote-plugin-launcher', '/plugins/remote-launcher']
      volumes:
        - mountPath: "/plugin-laucher"
          name: plugin-laucher
          attributes:
            persistVolume: false
  containers:
   - name: theia-ide
     image: "docker.io/eclipse/che-theia:next"
     env:
       - name: THEIA_PLUGINS
         value: local-dir:///plugins
     ...
     volumes:
       - mountPath: "/plugins"
         name: plugins
     mountSources: true
     ...

@davidfestal @amisevsk @l0rd WDYT?

[AndrienkoAleksandr]

@sleshchenko I think it makes sense to separate and merge changes we all are agreed:
Add an ability to add init containers in meta.yaml
Add an ability to override command and args
Add an ability to forcibly mark a volume as ephemeral (emptyDir, persistVolume:false).

Cool idea. +1 To split work part which was agreed like positive changes and move incremental forward. Che server then will be ready for any our solution with plugin-broker. And then rework plugin-broker.

@sleshchenko I prefer initContainer field instead of adding initContainer flag into a container entity model.

+1 for initContainer field

[davidfestal]

Is the whole PluginPatcher structure really required to manage only the Che Theia use-case ?
Wasn't it simpler to just implement the change on the plugin-broker side for the Theia/Vscode plugins (since the broken already makes the difference between the plugin types) ?

Is the PluginPatcher type (that now leaks into the plugin registry meta.yaml json schema) sufficiently general to be really useful for other type of Che plugins ? I'm especially thinking of the matcher: maybe matching on the type only is a bit limited if you want the plugin patcher to be a general and generic mechanism...

[nickboldt]

What's the status here? PR closed but not merged? Is this still in progress in another PR, or have we given up on this plan?

Meanwhile linked issue eclipse-che/che#13387 is set for milestone 7.2.0 which was released 3 days ago. Is it moving to 7.3?

[AndrienkoAleksandr]

Closed on favour: #76 or concurrent solution #75