Skip to content
This repository has been archived by the owner on Jan 20, 2023. It is now read-only.

Start remote binary using env and appling ephemeral volume #76

Conversation

AndrienkoAleksandr
Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr commented Sep 30, 2019

What does this PR do?

At this pr I changed a bit model: applied Type field to the model.ChePlugin

Inject remote endpoint binary using env variable with remote binary path and volume where is binary located.
For this purpose we are using init container from che-theia editor-plugin:

initContainers:
  - name: remote-runtime-injector
    image: aandrienko/che-theia-endpoint-runtime-binary:next
    volumes:
      - mountPath: "/remote-endpoint"
        name: remote-endpoint
        ephemeral: true
    env:
      - name: PLUGIN_REMOTE_ENDPOINT_EXECUTABLE
        value: /remote-endpoint/plugin-remote-endpoint
      - name: REMOTE_ENDPOINT_VOLUME_NAME
        value: remote-endpoint

Plugin broker should find this init container by name remote-runtime-injector and only for che-theia editor plugin. Then plugin-broker should analize env variables and volume from init container.

REMOTE_ENDPOINT_VOLUME_NAME env we are using to find one volume which should be shared with other sidecar containers. In this volume init container on start should copy remote binary

PLUGIN_REMOTE_ENDPOINT_EXECUTABLE env will be shared with all sidecar containers.

To start remote binary in the sidecar container, plugin writer should define in the sidecar dockerfile ENTRYPOINT:

FROM some-image 
....
ENTRYPOINT ${PLUGIN_REMOTE_ENDPOINT_EXECUTABLE} 

Or apply starting remote binary in the entrypoint.sh script, if plugin image uses such one.

What issues does this PR fix or reference?

eclipse-che/che#13387

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@codecov
Copy link

codecov bot commented Sep 30, 2019

Codecov Report

Merging #76 into master will increase coverage by 0.97%.
The diff coverage is 81.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
+ Coverage   67.17%   68.15%   +0.97%     
==========================================
  Files           6        7       +1     
  Lines         591      650      +59     
==========================================
+ Hits          397      443      +46     
- Misses        168      176       +8     
- Partials       26       31       +5
Impacted Files Coverage Δ
brokers/unified/broker.go 83.51% <100%> (+0.26%) ⬆️
brokers/unified/vscode/broker.go 74.21% <100%> (+0.13%) ⬆️
brokers/unified/vscode/sidecar.go 100% <100%> (ø) ⬆️
brokers/unified/remote_runtime_injector.go 76.78% <76.78%> (ø)

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 8b3c7a9...af5340e. Read the comment docs.

@benoitf
Copy link

benoitf commented Sep 30, 2019

IMHO description of the PR should tell the name of the env variable, what to add, etc.
Why it's like that, what we can do, with an example, etc.

@nickboldt
Copy link
Contributor

nickboldt commented Sep 30, 2019

What is the impact on docs for this change? #definitionOfDone


InjectorContainerName = "remote-runtime-injector"

RemoteEndPontExecutableEnvVar = "PLUGIN_REMOTE_ENDPOINT_EXECUTABLE"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: RemoteEndPointExecutableEnvVar

func InjectRemoteRuntime(plugins []model.ChePlugin) {
editorPlugin, err := findDefaultEditorPlugin(plugins)
if err != nil {
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't fail silently here.


runtimeBinaryPathEnv, err := findEnv(RemoteEndPontExecutableEnvVar, containerInjector.Env)
if err != nil || runtimeBinaryPathEnv.Value == "" {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If err == nil and runtimeBinaryPathEnv.Value == "", this will return nil, nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@AndrienkoAleksandr
Copy link
Contributor Author

IMHO description of the PR should tell the name of the env variable, what to add, etc.
Why it's like that, what we can do, with an example, etc.

Fixed

Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
Signed-off-by: Oleksandr Andriienko <oandriie@redhat.com>
@AndrienkoAleksandr
Copy link
Contributor Author

Closed on favor full solution #75
@amisevsk all your comments I handled using cherry-pick commits to #75

@nickboldt nickboldt deleted the startRemoteBinaryUsingEnvAndApplingEphemeralVolume branch June 29, 2021 19:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants