CLI - Separate debugging apart from local repo #3581

TylerJewell · 2016-12-30T23:51:54Z

What does this PR do?

This PR creates a separation of concern between the concept of using a local repository :/repo apart from activating development mode + debugging --debug. In the existing CLI, these two concepts are mixed together, so that anytime you mount a local repository, debugging of the che server is activated. By separating these two concerns, a user can now activate debugging for images within the shipped Docker image. Or, a user can also use local binaries in production mode.

Activates debugging for any command with the option --debug
Removes automatic setting of dev mode if :/repo is mounted.
If :/repo is not mounted AND --debug is activated, then debugs binaries shipping within eclipse/che-server
Fixes bug where CHE_DEBUG_PORT was not being used in the docker-compose.yml template.
If --debug displays the Che server streaming output to the foreground, and then kills it after server is booted.
Enables usage of local binaries in production mode.
Fixes long term bug on windows where some directories are not deleted unless command rerun.

What issues does this PR fix or reference?

#3542
#3546

Previous behavior

If :/repo was mounted, then dev mode was automatically activated. But there was no way to debug internal binaries. Now the only way to activate debug mode is with --debug added to any command. The :/repo is now optional, only if you want to debug your source repo.

Testing

To test this, there are modifications to three images - you need to rebuild dockerfiles/base, dockerfiles/cli, and dockerfiles/init. Or you can run:

docker run --rm -it -v /var/run/docker.sock:/var/run/docker.sock \
        -v <PATH>:/data \
        -v <PATH-TO-CHE>/dockerfiles/base/scripts/base:/scripts/base \
        eclipse/che-cli:nightly start --debug

Codenvy Updates

The Codenvy and ARTIK CLIs now inherit from the Che CLI. This could be breaking behavior for those inherited CLIs. Things to update:

CHE_DEVELOPMENT_MODE is no longer on or off. There is now a CHE_DEBUG which is true or false. Rather it's development or production. There is now a debug_server() method which will tell you if debugging is on.
You can now call local_repo() to find out if a local repository has been activated or not. In the puppet configuration of cmd_config(), special attention must be taken as we now have different syntax for four configurations - debug or no-debug and local repo or no-local-repo.
In the puppet module for Docker compose, the debug port should be set if CHE_DEBUG=true. However, the local assembly should only be mounted to the container if local_repo() = true. Before, the variable was meaning double - that it was for both dev mode and for using a repo. Now we have a hard separation.

When this is merged, also merge:
eclipse-che/che-docs#19
codenvy/codenvy#1458

TylerJewell · 2016-12-30T23:54:19Z

Do not merge this until:

Codenvy has been tested with the modifications.
Docs have a matching pull request

codenvy-ci · 2016-12-31T00:59:22Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1497/

codenvy-ci · 2016-12-31T17:37:58Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1498/

garagatyi · 2017-01-04T09:43:02Z

dockerfiles/base/scripts/base/commands/cmd_config.sh

@@ -30,8 +30,8 @@ cmd_config() {
  fi

  # Development mode


Does this comment still relevant?

Good catch. Updated.

garagatyi · 2017-01-04T09:46:49Z

dockerfiles/base/scripts/base/commands/cmd_config.sh

+
+        # Super weird bug - sometimes, the RM command doesn't wipe everything, so we have to repeat it a couple times
+        until directory_is_clean; do
+          docker_run -v "${CHE_HOST_INSTANCE}/dev":/root/dev alpine:3.4 sh -c "rm -rf /root/dev/${CHE_MINI_PRODUCT_NAME}-tomcat" > /dev/null 2>&1  || true


Why do you change path from /root/dev/* to /root/dev/${CHE_MINI_PRODUCT_NAME}-tomcat" ?

I didn't make this path change. I believe that @riuvshin had made this path change in another issue that was already reviewed and merged. I believe it has to do with being clearer in the in the instance folder on what parts are Tomcat vs. which parts are not.

Then can you rebase your branch or merge master into it

garagatyi · 2017-01-04T09:49:49Z

dockerfiles/base/scripts/base/commands/cmd_config.sh

@@ -76,7 +82,21 @@ generate_configuration_with_puppet() {
    CHE_ENV_FILE="${CHE_HOST_INSTANCE}/config/$CHE_MINI_PRODUCT_NAME.env"
  fi

-  if [ "${CHE_DEVELOPMENT_MODE}" = "on" ]; then
+  if debug_server; then
+    DEV_MODE="development"


CHE_ENVIRONMENT looks better than DEV_MODE

garagatyi · 2017-01-04T09:52:22Z

dockerfiles/base/scripts/base/commands/cmd_config.sh

                  -e \"CHE_ASSEMBLY=${CHE_ASSEMBLY}\" \
                  --entrypoint=/usr/bin/puppet \
                      $IMAGE_INIT \
                          apply --modulepath \
                                /etc/puppet/modules/ \
-                                /etc/puppet/manifests/${CHE_MINI_PRODUCT_NAME}.pp --show_diff"
+                                /etc/puppet/manifests/${CHE_MINI_PRODUCT_NAME}.pp --show_diff ${WRITE_LOGS}"
  else


It looks like both branches of if clause are equal now, WDYT?

They are different. In rows 85-97, we have a different logic for what this variable will contain dependening upon whether you have activated debugging or not. So with this logic, you can now have four combinations: using a repo (yes / no) or debugging (write to logs yes / no).

I have decided that the if() logic in this branch is no longer needed. So I have simplified this down to remove the if logic and generate a single command with variables.

garagatyi · 2017-01-04T09:52:57Z

dockerfiles/base/scripts/base/commands/cmd_config.sh

+  else
+    return 0
+  fi
+}


Please add trailing new line at the end of file

garagatyi · 2017-01-04T09:54:55Z

dockerfiles/base/scripts/base/commands/cmd_destroy.sh

-                alpine:3.4 sh -c "rm -rf /root${CHE_CONTAINER_ROOT}/docs \
-                               && rm -rf /root${CHE_CONTAINER_ROOT}/instance \
-                               && rm -rf /root${CHE_CONTAINER_ROOT}/${CHE_MINI_PRODUCT_NAME}.env" > /dev/null 2>&1  || true
-
  # Super weird bug.  For some reason on windows, this command has to be run 3x for everything


Does docker have an issue for that?

No, I haven't created an issue. I wasn't sure it was a docker issue for it. We discovered this a couple months ago when we first implemented the cmd_destroy function. We were seeing that we had to 3x remove stuff in that function and now it's coming up again. I can create that issue for them.

garagatyi · 2017-01-04T09:56:40Z

dockerfiles/base/scripts/base/commands/cmd_destroy.sh

+  until directory_is_empty; do
+    docker_run -v "${CHE_HOST_CONFIG}":/root${CHE_CONTAINER_ROOT} \
+                  alpine:3.4 sh -c "rm -rf /root${CHE_CONTAINER_ROOT}/docs \
+                                 && rm -rf /root${CHE_CONTAINER_ROOT}/instance \


Have you checked whether multiple runs of this command is still needed when && replaced with ; ?

When I put a semicolon there, then on windows it only requires a single loop to work. Nice catch!

garagatyi · 2017-01-04T09:58:32Z

dockerfiles/base/scripts/base/commands/cmd_destroy.sh

@@ -78,3 +66,12 @@ cmd_destroy() {
  fi
 }

+directory_is_empty() {


It is a bit confusing to have both directory_is_empty and directory_is_clean functions. Is it on purpose?

They had different syntax of what to check. So I keep both as directory_is_empty and then add a qualifier for the one in config now.

garagatyi · 2017-01-04T09:58:38Z

dockerfiles/base/scripts/base/commands/cmd_destroy.sh

+  else
+    return 0
+  fi
+}


Please add trailing new line at the end of file

riuvshin · 2017-01-04T10:00:26Z

afaik we wanted only possibility to enable debug for prod mode, if repo is mounted debug must be activated automatically.

garagatyi · 2017-01-04T10:06:30Z

@riuvshin Yes, It is what I requested originally. But @l0rd asked about complete separation of debug and repo mounting. So I believe Tyler is going to implement that.

riuvshin · 2017-01-04T10:15:43Z

dockerfiles/base/scripts/base/commands/cmd_config.sh

-  if [ "${CHE_DEVELOPMENT_MODE}" = "on" ]; then
+  if debug_server; then
+    DEV_MODE="development"
+    WRITE_LOGS=""


does that mean that puppet logs will be shown only in debug mode? debug mode should only activate debug and do not change any other behaviour

Yes, it means that puppet logs will be streamed if you have --debug only. If you mount a repository, but do not provide --debug then they would not be streamed.

garagatyi · 2017-01-04T10:20:11Z

dockerfiles/base/scripts/base/commands/cmd_start.sh

+      info "start" "Debug: ${DISPLAY_DEBUG_URL}"
+    fi
+  else
+    error "(${CHE_MINI_PRODUCT_NAME} start): Timeout waiting for server. Run \"docker logs ${CHE_SERVER_CONTAINER_NAME}\" to inspect the issue."


looks like kill $LOG_PID > /dev/null 2>&1 is needed here also

Moved the kill operation to be outside of hte initial loop so that it's killed first, displayed second.

garagatyi · 2017-01-04T10:22:55Z

dockerfiles/base/scripts/base/commands/cmd_config.sh

                  -e \"CHE_CONFIG=${CHE_HOST_INSTANCE}\" \
                  -e \"CHE_INSTANCE=${CHE_HOST_INSTANCE}\" \
-                  -e \"CHE_DEVELOPMENT_REPO=${CHE_HOST_DEVELOPMENT_REPO}\" \
+                  -e \"CHE_REPO=${CHE_REPO}\" \


Can you elaborate on why did you change CHE_HOST_DEVELOPMENT_REPO. As I can see it is still used in other places.

It was just come cleanup to keep things simpler. The puppet templates had logic where both the repo and debugging logic was using the same variable. And this was needed to separate the logic across two variables.

garagatyi · 2017-01-04T14:06:04Z

I tried it and here is what I found:

logs of che-server are not streamed when debug mode is enabled
--debug with no repo mounted doesn't allow to debug che-server

TylerJewell · 2017-01-04T14:07:18Z

I think you do not have the right images. You need to wipe everything and then rebuild /base, /init, /cli in that order.

garagatyi · 2017-01-04T14:13:01Z

I rebuilt everything with build-all script

TylerJewell · 2017-01-04T14:14:44Z

I have never used that script - and I think we should assume that it's not perfect. Because you need to make sure that /cli image inherits from the right /base image.

garagatyi · 2017-01-04T14:39:53Z

Sorry, I forgot to switch branch 😳
Now everything works like a charm in Che. I didn't try Codenvy yet.
BTW looks like final output was aligned on purpose, but now it is not:

INFO: (che start): Ver: nightly
INFO: (che start): Use: http://172.17.0.1:8080
INFO: (che start): API: http://172.17.0.1:8080/swagger
INFO: (che start): Debug: http://172.17.0.1:8000

TylerJewell · 2017-01-04T14:52:29Z

Yeah, I did the formatting on purpose. I figure in debugging mode, developers do not care about formatting. I'll respond to other comments later today.

TylerJewell · 2017-01-04T18:01:20Z

I believe this issue is ready for merging. Will update the matching Codenvy issue.

codenvy-ci · 2017-01-04T21:16:09Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/1515/

initial

c454276

TylerJewell added the kind/enhancement A feature request - must adhere to the feature request template. label Dec 30, 2016

TylerJewell self-assigned this Dec 30, 2016

Tyler Jewell added 2 commits December 30, 2016 15:56

fixup

6d15183

fixup

b068878

TylerJewell requested review from l0rd, garagatyi and riuvshin December 31, 2016 00:01

cleanup

d1c727c

TylerJewell mentioned this pull request Dec 31, 2016

Enhance CLI Dev Mode eclipse-che/che-docs#19

Merged

fixup

cd99f17

TylerJewell changed the title ~~Enhance CLI Dev Mode~~ CLI - Separate debugging apart from local repo Dec 31, 2016

TylerJewell mentioned this pull request Dec 31, 2016

Adapt CHE-3581 - Enhance CLI to separate debugging from local repo mount codenvy/codenvy#1458

Merged

benoitf approved these changes Jan 1, 2017

View reviewed changes

garagatyi reviewed Jan 4, 2017

View reviewed changes

riuvshin reviewed Jan 4, 2017

View reviewed changes

garagatyi reviewed Jan 4, 2017

View reviewed changes

fixup

18f5f10

l0rd approved these changes Jan 4, 2017

View reviewed changes

fixup

e844c99

TylerJewell mentioned this pull request Jan 4, 2017

cli dev mode consistency #3551

Closed

riuvshin approved these changes Jan 4, 2017

View reviewed changes

garagatyi approved these changes Jan 5, 2017

View reviewed changes

TylerJewell merged commit cfc39c2 into master Jan 5, 2017

TylerJewell deleted the cli-dev-mode branch January 5, 2017 13:25

TylerJewell added this to the 5.0.0 milestone Jan 5, 2017

JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017

CLI - Separate debugging apart from local repo (eclipse-che#3581)

5084a12

CLI - Separate debugging apart from local repo #3581

CLI - Separate debugging apart from local repo #3581

Conversation

TylerJewell commented Dec 30, 2016 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Previous behavior

Testing

Codenvy Updates

TylerJewell commented Dec 30, 2016

codenvy-ci commented Dec 31, 2016

codenvy-ci commented Dec 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riuvshin commented Jan 4, 2017

garagatyi commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garagatyi commented Jan 4, 2017

TylerJewell commented Jan 4, 2017

garagatyi commented Jan 4, 2017

TylerJewell commented Jan 4, 2017

garagatyi commented Jan 4, 2017

TylerJewell commented Jan 4, 2017

TylerJewell commented Jan 4, 2017

codenvy-ci commented Jan 4, 2017

TylerJewell commented Dec 30, 2016 •

edited

Loading