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

Generate remote binary #410

Merged
merged 4 commits into from
Oct 8, 2019
Merged

Generate remote binary #410

merged 4 commits into from
Oct 8, 2019

Conversation

AndrienkoAleksandr
Copy link
Contributor

What does this PR do?

Add image with remote-plugin-endpoint binary

What issues does this PR fix or reference?

eclipse-che/che#13387

@dmytro-ndp
Copy link
Contributor

crw-ci-test

fi \
fi
# Light image without node. We include remote binary to this image.
FROM alpine
Copy link
Contributor

Choose a reason for hiding this comment

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

hello, please add the version for the image (to be sure to not get unwanted) updates

#invalidate cashe
ADD https://${GITHUB_TOKEN}:x-oauth-basic@api.github.com/repos/theia-ide/theia/git/refs/head /tmp/branch_info.json
ADD https://${GITHUB_TOKEN}:x-oauth-basic@api.github.com/repos/eclipse/che-theia/git/refs/head /tmp/branch_info.json
RUN mkdir /home/theia
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this docker image if we do injection of the binary ?

or it's to have the binary for injection that we're making this image ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This image will be init container for Che theia editor plugin.

trap 'responsible_shutdown' SIGHUP SIGTERM SIGINT

cd ${HOME}
trap 'responsible_shutdown' HUP TERM INT
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason to change the traps ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, of course, SIGHUP SIGTERM SIGINT doesn't work on fedora.

@@ -22,7 +21,7 @@ export class ChePluginApiProvider implements ExtPluginApiProvider {
initFunction: 'initializeApi',
initVariable: 'che_api_provider'
},
backendInitPath: path.join(__dirname, '../plugin/node/che-api-node-provider.js')
backendInitPath: '@eclipse-che/theia-plugin-ext/lib/plugin/node/che-api-node-provider.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this change and the next one could be merged in another PR

@dmytro-ndp
Copy link
Contributor

crw-ci-test

@che-bot
Copy link
Contributor

che-bot commented Sep 4, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

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

che-bot commented Sep 29, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@che-bot
Copy link
Contributor

che-bot commented Sep 29, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@che-bot
Copy link
Contributor

che-bot commented Oct 3, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

@AndrienkoAleksandr
Copy link
Contributor Author

ci-build-check

@AndrienkoAleksandr
Copy link
Contributor Author

ci-build

@AndrienkoAleksandr
Copy link
Contributor Author

@benoitf @evidolob @mmorhun Could you review, please?

nexe node_modules/@eclipse-che/theia-remote/lib/node/plugin-remote.js -t alpine-x64-10.16.0 -o /plugin-remote-endpoint

# Light image without node. We include remote binary to this image.
FROM alpine:3.10.2
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the difference of using FROM ALPINE:3.10.2 and using FROM scratch if we don't install anything and that the binary is self-contained ? it's due to musl linking I suppose ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, @benoitf, Seems, scratch has only latest tag. And this image has not cp and has not shell(even without sh). Dockerfile RUN section doesn't work without sh, so I can not install cp. And I think scratch has not any package manager, because it's empty image, so I don't know how to install cp or so on.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok I see.

I might see another problem.
We generate nexe for alpine but if the runtime is not alpine, what is the behavior ?
like using debian, centos, fedora, etc images ?

as we'll copy the bootstrap to another container ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benoitf I tested with centos, fedora debian and it is works. List tested images was pretty big. You can see this list 99ae354#diff-f7eaf7e082319e42b2dfd26a86ca0736R80 search by from, I commented code, but each image was tested.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, still strange but if it works.

. "${base_dir}"/../build.include

init --name:theia-endpoint-runtime-binary "$@"
build
Copy link
Contributor

Choose a reason for hiding this comment

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

very minor: missing EOF


USER root

RUN cd /home/theia && \
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems usually linters want that we use WORKDIR /home/theia before but I haven't run linter there so maybe it's fine


FROM ${BUILD_ORGANIZATION}/${BUILD_PREFIX}-theia:${BUILD_TAG} as builder

USER root
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to be root ?
AFAIK we can install stuff globally in theia images ?

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

che-bot commented Oct 3, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@benoitf
Copy link
Contributor

benoitf commented Oct 4, 2019

hello, how to test ?

@AndrienkoAleksandr
Copy link
Contributor Author

AndrienkoAleksandr commented Oct 4, 2019

@benoitf hello, how to test ?

All stuff I tested on the minishift.

Build che-theia and theia-endpoint-runtime-binary images

You need to build che-theia, and theia-endpoint-runtime-binary images, so from my branch:

./build.sh --pr

You can retag and push images to the dockerhub.

Use image with upgraded che-plugin-broker

For integration image theia-endpoint-runtime-binary like init-container for che-theia, you need to use upgraded che-plugin-broker: eclipse-che/che-plugin-broker#75 . You can use mine image: aandrienko/che-unified-plugin-broker:latest, or build own using script(but patched to use your dockerhub account): https://github.com/eclipse/che-plugin-broker/tree/startRemoteBinaryWithOverrideEntryPoint/.ci/build.sh

To activate this plugin-broker image in the che, you need to open deployment che-server and apply env variable:

CHE_WORKSPACE_PLUGIN__BROKER_UNIFIED_IMAGE aandrienko/che-unified-plugin-broker:latest

Redeploy che.

Test devfile

And you can use some test devfile with your che-theia and theia-endpoint-runtime-binary images. Example of mine devfile:

https://gist.github.com/AndrienkoAleksandr/ec1efffcbb4b25d52c22029bb7b6ac69

Interesting part of the test devfile.

We are using our builded image with che-theia:

https://github.com/AndrienkoAleksandr/che-plugin-registry/blob/master/v3/plugins/eclipse/che-theia/remote2/meta.yaml#L63

Init container section here with theia-endpoint-runtime-binary:

https://github.com/AndrienkoAleksandr/che-plugin-registry/blob/master/v3/plugins/eclipse/che-theia/remote2/meta.yaml#L51

Sidecar image for typescript lsp

And we need image for sidecar container, here is I used fedora:30:

DockerFile
Fedora with node image build Script(should be used for typescript lsp)
meta.yaml

In the devfile sidecar plugin here:
https://gist.github.com/AndrienkoAleksandr/ec1efffcbb4b25d52c22029bb7b6ac69#file-gistfile1-txt-L16

@AndrienkoAleksandr
Copy link
Contributor Author

@l0rd @benoitf Is it OK to merge it? It's planned for 7.3 and rework sidecar images depends on it.

@benoitf
Copy link
Contributor

benoitf commented Oct 7, 2019

yes
it's not harmful to merge it BTW

@AndrienkoAleksandr
Copy link
Contributor Author

ci-build

@AndrienkoAleksandr
Copy link
Contributor Author

crw-ci-test

1 similar comment
@musienko-maxim
Copy link

crw-ci-test

@che-bot
Copy link
Contributor

che-bot commented Oct 7, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

Copy link
Contributor

@l0rd l0rd left a comment

Choose a reason for hiding this comment

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

LGTM Tested on che.osio and worked fine. I kind of agree on @benoitf comments but these are things that can be addressed in the later.

@che-bot
Copy link
Contributor

che-bot commented Oct 7, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@che-bot
Copy link
Contributor

che-bot commented Oct 7, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has failed:

@musienko-maxim
Copy link

The PR was checked manually. All steps in the HappyPath test were passed.

@musienko-maxim musienko-maxim self-requested a review October 8, 2019 06:18
@AndrienkoAleksandr AndrienkoAleksandr merged commit 40a345b into master Oct 8, 2019
@AndrienkoAleksandr AndrienkoAleksandr deleted the GenerateRemoteBinary branch October 8, 2019 06:18
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.

8 participants