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

Merge all che_plugin.yml fields into meta.yaml #12908

Closed
l0rd opened this issue Mar 18, 2019 · 13 comments · Fixed by #13311
Closed

Merge all che_plugin.yml fields into meta.yaml #12908

l0rd opened this issue Mar 18, 2019 · 13 comments · Fixed by #13311
Labels
kind/enhancement A feature request - must adhere to the feature request template.

Comments

@l0rd
Copy link
Contributor

l0rd commented Mar 18, 2019

Description

Currently the informations to load plugins are splitted in two distinct files: meta.yml and che_plugin.yml. See for example che-theia meta.yml and che-plugin.yml.

We have experienced that managing 2 distinct files is problematic:

  • some information are duplicated
  • some kind of plugins can't have a che_plugin.yaml (i.e. vs code extensions)
  • clients may need access to the informations included in the che_plugin.yml (i.e. the user dashboard and the memory limits of a plugin)

The proposed solution is to extend the meta.yaml model to include all the fields of a che_plugin.yml and to remove the need for che_plugin.yml file at all.

@l0rd l0rd added team/osio kind/enhancement A feature request - must adhere to the feature request template. labels Mar 18, 2019
@l0rd
Copy link
Contributor Author

l0rd commented Mar 18, 2019

cc @ibuziuk @garagatyi

@skabashnyuk
Copy link
Contributor

Does this mean that che-plugin.yaml brokering process would be moved to workspace master side? In other words how it would be implemented?

@l0rd
Copy link
Contributor Author

l0rd commented Mar 18, 2019

Does this mean that che-plugin.yaml brokering process would be moved to workspace master side?

That's a good question. I will let @garagatyi provide a proposal for the implementation but let me say that the good thing about brokers is that they can be reused as golang library by the CRD (i.e. avoid code duplication). Hence I would take this in consideration when choosing if or not we should integrate the brokering mechanism in wsmaster.

@ibuziuk
Copy link
Member

ibuziuk commented Mar 18, 2019

let's also consider updating che_plugin_brokers [1] doc in order to reflect changes that will be done as part of this issue.

https://github.com/ibuziuk/docs/blob/master/che_plugin_brokers.adoc

@garagatyi
Copy link

Here is what I suggest.

  1. Add model version into the meta.yaml model: apiVersion: 1, 2, 3. Current one is considered 1, if not specified is considered 1. This will be used for backward compatibility.
  2. Add meta.yaml of apiVersion: 2:
    1. Add spec field into the meta.yaml. All the configuration should go into spec filed now. Spec should represent fields that were previously presented in che-plugin.yaml except those that are already in meta.yaml. Example meta.yaml
    2. Remove URL from the meta.yaml - should be part of the spec field.
    3. Remove attributes from the meta.yaml - should be part of the spec field.
    4. Remove editors field from che-plugin.yaml model since it is unused ATM. We can add a functionality like this (compatibility config) later.
    5. In case VS Code plugin spec should also include extensions field that lists extensions IDs (from VS Code marketplace) and/or direct URLs to extensions archives. See simple variant which is similar to what we have ATM plugin-vscode.yaml. We can add RAM limit, see plugin-vscode-with-ram.yaml. Since it uses common syntax it can set all the other fields: env vars, memory limit, volumes, etc. See plugin-vscode-extended.yaml. Is it supposed that plugin broker will extend this definition with auto generated info before returning it to Che master.
    6. In the same way as VS Code plugin specifies its config Theia plugin can also specify it. We can use plugins field instead of extensions since Theia uses this term for runtime pluggable extensions.
    7. Remove files copying functionality in common Che plugin type. It was represented by che-dependencies.yaml files. At the moment we don’t use it, but we can add (initially or later) files field similar to extensions/plugins fields in VS code/Theia plugins config later.
  3. Conversion of meta.yaml with apiVersion: 1 to apiVersion: 2 should be transparent but will require downloading che-plugin.yaml in tar.gz archive in a case of common che plugin broker. We use che-plugin.yaml for Che 7 editors and exec plugin. It should be simple enough to implement backward compatibility for this case.
  4. We should move downloading of meta.yaml files from WS Master to plugin brokers itself. In this case, we would move all the useful code into golang codebase that can evolve faster than WS master, would be more holistic comparing to current splitting responsibilities between WS master and brokers. This will simplify other changes in brokering process, will allow to reuse changes in CRD POC without significant investment in writing code from scratch and will allow us to separate brokers into a long living service outside of workspace lifecircle which would allow speeding up workspace start.
  5. How we can iterate to deliver faster and split the work between different engineers/sprints (will reuse list counter to simplify referring in comments from others):
    1. Merge plugin brokers into a single program - unified plugin broker.
    2. Make WS master call unified broker instead of several plugin brokers.
    3. Add an ability to unified plugin broker to process plugins IDs instead of plugin metas. In the case when IDs are passed instead of metas broker should download metas from registry itself.
    4. Switch master to send plugin IDs instead of plugins metas to unified broker.
    5. Rework unified broker to support new fields in meta.yaml and provide backward compatibility with the apiVersion scheme.
  6. Future improvements:
    1. Instead of sending brokering results as it is sent now plugin-service-response-v1.yaml, send results in form which would allow implementing automatic plugins sidecar co-location plugin-service-response-v2.yaml. See explanation here. Automatic plugins co-location feature would also affect ability of UD to get RAM limits for the whole workspace - we will need to find solution for that.
    2. Separate sidecar config evaluation and binaries delivering to workspace:
      1. Do not support setting container image in Theia plugins using engine.cheRuntimeContainer in package.json. Container image has to go into meta.yaml starting from apiVersion: 2. This will help avoid confusion when image is set in both meta.yaml and package.json.
      2. Rework theia remote sidecar connectivity approach on brokers side to avoid downloading VS Code extension archives to evaluate plugin name and publisher from package.json. This would help splitting sidecar configuration and binaries downloading phases and will speed up workspace start. See Florent’s comment about available options here
      3. Create new golang based tiny program that can download binaries and put it in specified locations. Let’s call it download container.
      4. Rework broker to return download container config with plugin binaries location as part of sidecars definition. This should not involve any downloading on the sidecar definition evaluation stage.
      5. Make ws master use download container as init container(s) in workspace.
    3. Move unified broker to a separate long living rest service and make ws master use it instead of broker on each workspace start. We won’t need any additional deployments before workspace start which will speed up workspace start process, especially for ephemeral workspaces.
    4. Should we have json schema for meta.yaml?
    5. Make WS master not run binaries downloading when workspace doesn't change to speed up workspace start and decrease probability of VS code marketplace rate limit error for the workspace and decrease its influence on new/updated workspaces.

@garagatyi
Copy link

@garagatyi
Copy link

Do not get scared by the size of the suggestion. It includes detailed description of model changes, possible future improvements and how we can iteratively implement merge pf meta.yaml and che-plugin.yaml.
It seems that we can start work on this in the next sprint and split work in the team.

@ibuziuk
Copy link
Member

ibuziuk commented Mar 19, 2019

@garagatyi do we need subtasks? maybe just a bullet list + dedicated issues in rh-che for planning?

@garagatyi
Copy link

@ibuziuk I'll prepare a list of tasks in rhche repo before the planning session. Hopefully, Mario and others would be able to leave feedback whether the plan is OK for them before planning.

@garagatyi
Copy link

We discussed this with Mario and Ilya and agreed not to support backward compatibility yet since Che is in beta and not heavily used at the moment.

@garagatyi
Copy link

@benoitf Are you OK with the above described approach of deprecating setting plugin image in package.json? Copying here for the convenience:

Do not support setting container image in Theia plugins using engine.cheRuntimeContainer in package.json. Container image has to go into meta.yaml starting from apiVersion: 2. This will help avoid confusion when image is set in both meta.yaml and package.json.

@l0rd
Copy link
Contributor Author

l0rd commented Apr 29, 2019

@garagatyi @benoitf is on PTO this week

@benoitf
Copy link
Contributor

benoitf commented Apr 29, 2019

I am ok

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.
Projects
None yet
5 participants