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

Optimize che-launcher for perf #2105

Merged
merged 4 commits into from
Aug 12, 2016
Merged

Optimize che-launcher for perf #2105

merged 4 commits into from
Aug 12, 2016

Conversation

TylerJewell
Copy link

@TylerJewell TylerJewell commented Aug 12, 2016

What does this PR do?

This pull request implements optimizations to minimize the number of times that docker run is called within the che-launcher utility. Previously, there were numerous helper functions that have embedded docker run commands. If these functions were repeatedly called, we were launching unnecessary containers when one invocation will do.

Also fixes a small error where one of the docker run commands did not have a --rm leaving a lot of lingering exited containers on the user's system.

Also adds in a build.sh into the directory to perform a build of the image.

@TylerJewell TylerJewell added this to the 4.7.0 milestone Aug 12, 2016
@TylerJewell TylerJewell self-assigned this Aug 12, 2016
@TylerJewell
Copy link
Author

@benoitf @l0rd @riuvshin - FYI - one of you +1. The speed of a che info command is now dramatically faster.

@codenvy-ci
Copy link

Build # 71 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/71/ to view the results.

@l0rd
Copy link
Contributor

l0rd commented Aug 12, 2016

@TylerJewell 👏 👏 the launcher is really faster now.

I have one comment:

You are calling 3 times docker run at the beginning of the launcher to set ETH0_ADDRESS, ETH1_ADDRESS and DOCKER0_ADDRESS but you really just need one of them. You can check if DEFAULT_DOCKER_HOST_IP has already been set at the very beginning ofget_docker_host_ip()` instead:

if [ -n "${DEFAULT_DOCKER_HOST_IP}" ]; then
    echo "${DEFAULT_DOCKER_HOST_IP}"
fi

You could do the same in has_docker_for_windows_ip() and get_che_launcher_version(): if the variables are already set don't run the command, otherwise run it.

As a result you will be able to call the functions directly as many times as you need and you should not need to worry to run all the docker run command at the beginning (but just when you need them).

@TylerJewell
Copy link
Author

@l0rd - so I studied this for a bit this morning and concluded that the system would not profit. In order to implement variable check within get_docker_host_ip, I'd have to reintroduce additional docker run command back into is_docker_for_windows to avoid an infinite loop, thus impacting performance again.

@TylerJewell
Copy link
Author

Added a fix for #2102 as well.

@l0rd
Copy link
Contributor

l0rd commented Aug 12, 2016

LGTM

@TylerJewell TylerJewell merged commit 54f7432 into master Aug 12, 2016
@TylerJewell TylerJewell deleted the che-launcher-optimize branch August 12, 2016 15:59
@codenvy-ci
Copy link

rnavagamuwa pushed a commit to rnavagamuwa/che that referenced this pull request Aug 15, 2016
* Optimize che-launcher for perf

* Changed test for addresses

* fixup! ignore missing interface

* fix port view
rnavagamuwa pushed a commit to rnavagamuwa/che that referenced this pull request Aug 15, 2016
* Optimize che-launcher for perf

* Changed test for addresses

* fixup! ignore missing interface

* fix port view
@bmicklea bmicklea mentioned this pull request Aug 18, 2016
89 tasks
JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017
* Optimize che-launcher for perf

* Changed test for addresses

* fixup! ignore missing interface

* fix port view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants