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.

Sign up for GitHub

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

Jump to bottom

Support entrypoint definition in dockerimage recipe based workspaces on k8s and os #12716

Merged
merged 1 commit into from
Mar 5, 2019
Merged

Support entrypoint definition in dockerimage recipe based workspaces on k8s and os #12716

merged 1 commit into from
Mar 5, 2019

Conversation

metlos
Copy link
Contributor

@metlos metlos commented Feb 20, 2019

What does this PR do?

$TITLE.

More specifically, this makes sure that at most 1 machine is defined on dockerimage based workspace and if that machine contains containerCommand attribute, it is parsed as a YAML list and supplied as the entrypoint command of the container. Additionally if containerArgs is present, it is also parsed as a YAML list and supplied as custom arguments to the entrypoint command.

What issues does this PR fix or reference?

#12668

Release Notes

  • TODO

Docs PR

  • TODO

@che-bot che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. labels Feb 20, 2019
sleshchenko
sleshchenko approved these changes Feb 20, 2019
View reviewed changes
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

Draft LGTM.

Let's move on PR ready to review
It would be nice to see different commits for entrypoint and additional check for dockerimage
environment.

...e-api-model/src/main/java/org/eclipse/che/api/core/model/workspace/config/MachineConfig.java Outdated Show resolved Hide resolved
...rg/eclipse/che/workspace/infrastructure/docker/environment/dockerimage/EntryPointParser.java Outdated Show resolved Hide resolved
@sleshchenko
Copy link
Member

@garagatyi Please take a look too

@metlos metlos marked this pull request as ready for review February 20, 2019 14:15
@metlos metlos requested a review from skabashnyuk as a code owner February 20, 2019 14:15
@metlos metlos force-pushed the dockerimage-recipe-entrypoint branch from b02a910 to c19e664 Compare February 25, 2019 21:43
@metlos metlos mentioned this pull request Feb 25, 2019
Merged
2 tasks
garagatyi
garagatyi reviewed Feb 26, 2019
View reviewed changes
Copy link

@garagatyi garagatyi left a comment

Choose a reason for hiding this comment

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

To be honest I think that this functionality should be implemented with a different approach.
Current solution brings a lot of code that not really needed. What if we just convert Dokerimage environment to K8s environment using existing converter and add entrypoint, command arguments, etc?
This should eliminate a need for all this code.
@sleshchenko @metlos WDYT?

@sleshchenko
Copy link
Member

@garagatyi
A lot of code that contains this PR is related to converting workspace config attribute to yaml list (that may be a single string, strings list). And we can avoid having such code by declaring that this list MUST contain comma-separated strings.

I do not see how your proposal may reduce the amount of code. Can you elaborate more?

@metlos
Copy link
Contributor Author

metlos commented Feb 26, 2019

To be honest I think that this functionality should be implemented with a different approach.
Current solution brings a lot of code that not really needed. What if we just convert Dokerimage environment to K8s environment using existing converter and add entrypoint, command arguments, etc?
This should eliminate a need for all this code.
@sleshchenko @metlos WDYT?

Our dockerimage environment doesn't understand entrypoints, AFAIK, and enhancing docker support when we're about to drop it is IMHO not the way to go. That's why I only added the support for it to k8s env.

As @sleshchenko, I don't see where would save a lot of code here though if we went down the route of adding support for entrypoint to dockerimage and added support for the also to the converter.

@metlos
Copy link
Contributor Author

metlos commented Feb 26, 2019 •
edited
Loading

A lot of code that contains this PR is related to converting workspace config attribute to yaml list (that may be a single string, strings list). And we can avoid having such code by declaring that this list MUST contain comma-separated strings.

In this case, I think it would be counterproductive if we wanted to use 2 different formats for encoding entrypoints in workspace config and in the devfile (which is YAML). If we wanted to have a custom "comma separated list of commands", we'd need a parser (in workspace config) and serializer (in devfile) for that. I the approach I chose, we can use just the YAMLMapper for both (the serializer is not part of this PR but I added it in #12761).

@garagatyi
Copy link

@sleshchenko If we avoid direct implementation of entrypoint and command support in dockerimage workspace recipe (Che 6 stuff) we would do:

  1. In DockerimageToolToWorkspaceApplier convert devfile dockerimage tool into k8s environment directly. Using existing dockerimage to k8s env converter or even directly tool to k8s.
  2. Add entrypoint/args into k8s env

With this approach we would not need anything from this PR at all.

Our dockerimage environment doesn't understand entrypoints, AFAIK, and enhancing docker support when we're about to drop it is IMHO not the way to go. That's why I only added the support for it to k8s env.
As @sleshchenko, I don't see where would save a lot of code here though if we went down the route of adding support for entrypoint to dockerimage and added support for the also to the converter.

Well, I'm suggesting not to improve dockerimage recipe at all since it brings new attributes, parsing, etc. And this PR adds entrypoint/args case as machine config attribute which would not be needed when we replace workspace config with devfile usage, eventually.

@garagatyi
Copy link

@metlos I think improving outdated format with additional attributes is not productive.

@metlos
Copy link
Contributor Author

metlos commented Feb 26, 2019

@metlos I think improving outdated format with additional attributes is not productive.

ATM devfile "appliers" convert devfile into workspace config. If we were to take a different approach here, we would have to reimplement the whole of devfile conversion, which I think is not practical at this point in time. The amount of code added here is not that huge to justify rewriting everything devfile we have.

@garagatyi
Copy link

After discussion with @sleshchenko and @metlos we agreed that:

  • changing this solution to the one I suggested would simplify the code, but might lead to a very confusing UX for end users when they specify dockerimage tool and endup with kubernetes recipe in the workspace created from the dockerimage devfile tool. Presence of user facing workspace config here leads to this situation. We think that direct usage of devfile would be way better than continuing usage of workspace config
  • this solution is a hack, but because of significant limitations of workspace config we can't avoid it
  • we should not document new abilities for Che 6 style workspace config and only promote usage of devfile for setting entrypoint and args

@sleshchenko sleshchenko self-requested a review February 27, 2019 12:20
@sleshchenko sleshchenko self-requested a review February 27, 2019 12:29
@metlos metlos requested a review from amisevsk as a code owner February 28, 2019 07:36
@metlos
Copy link
Contributor Author

metlos commented Feb 28, 2019

ci-test

amisevsk
amisevsk reviewed Feb 28, 2019
View reviewed changes
Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

A few comments

...space/infrastructure/kubernetes/environment/convert/DockerImageEnvironmentConverterTest.java Outdated Show resolved Hide resolved
...workspace/infrastructure/kubernetes/environment/convert/DockerImageEnvironmentConverter.java Outdated Show resolved Hide resolved
@che-bot
Copy link
Contributor

che-bot commented Feb 28, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12716
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@sleshchenko sleshchenko self-requested a review March 1, 2019 10:00
sleshchenko
sleshchenko approved these changes Mar 1, 2019
View reviewed changes
Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

LGTM

Please take a look my inline comments

...in/java/org/eclipse/che/workspace/infrastructure/kubernetes/environment/util/EntryPoint.java Show resolved Hide resolved
@metlos
Copy link
Contributor Author

metlos commented Mar 1, 2019

ci-build

@metlos
Copy link
Contributor Author

metlos commented Mar 1, 2019

ci-test

@che-bot
Copy link
Contributor

che-bot commented Mar 1, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12716
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@metlos
Copy link
Contributor Author

metlos commented Mar 4, 2019

ci-test

@che-bot
Copy link
Contributor

che-bot commented Mar 4, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12716
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@metlos
Copy link
Contributor Author

metlos commented Mar 4, 2019 •
edited
Loading

ci-test, pretty please?

@che-bot
Copy link
Contributor

che-bot commented Mar 4, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12716
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@metlos
Copy link
Contributor Author

metlos commented Mar 4, 2019

ci-test

@che-bot
Copy link
Contributor

che-bot commented Mar 4, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12716
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@metlos
Handle the containerCommand and containerArgs attributes of the machine
b52ee40 
defined with the dockerimage recipe.

Note that this only works in kubernetes and openshift environments.

Signed-off-by: Lukas Krejci <[email protected]>
@metlos metlos force-pushed the dockerimage-recipe-entrypoint branch from 21edf1b to b52ee40 Compare March 5, 2019 14:04
@metlos
Copy link
Contributor Author

metlos commented Mar 5, 2019

rebased and squashed on top of latest master before merge

@metlos metlos merged commit 1dc5b8b into eclipse-che:master Mar 5, 2019
@metlos metlos mentioned this pull request Mar 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amisevsk amisevsk amisevsk approved these changes

@sleshchenko sleshchenko sleshchenko approved these changes

@garagatyi garagatyi garagatyi approved these changes

@skabashnyuk skabashnyuk Awaiting requested review from skabashnyuk

Assignees

@metlos metlos

Labels
kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@metlos @sleshchenko @garagatyi @che-bot @amisevsk