Fix the failed to mv error on python s2i #10

anmolbabu · 2018-12-12T21:57:39Z

The error happens when the local component source contains
a directory by name s2i. This fix clears the $APP_ROOT of
all files and folders before the assemble script copies
over sources from $destination dir to $app-root so that
mv from assemble script does not compile that the folder
already exists

fixes redhat-developer/odo#1054
Signed-off-by: anmolbabu anmolbudugutta@gmail.com

anmolbabu · 2018-12-12T22:06:49Z

This breaks watch :(

cdrage · 2018-12-13T20:39:06Z

Hey @anmolbabu can you please rebase to master? Tests had broken before.. See #11

anmolbabu · 2018-12-14T18:07:00Z

Working approach for source deployments :

Use backup-dir(/opt/app-root-src-backup) to backup source in $ODO_S2I_SRC_BIN_PATH..
rm -fr contents of $APP_ROOT/src so that assemble does not complain error in Python component push consistently fails to update source after the first successful push odo#1054
Trigger assemble script
Copy back source from /opt/app-root-src-backup to $APP_ROOT

This way, the sources from $ODO_S2I_SRC_BIN_PATH are backed up before being moved to $APP_ROOT from which the sources will be deleted to void error in redhat-developer/odo#1054 before the same being movede over from $ODO_S2I_SRC_BIN_PATH(which empties it for the next iteration of watch)

ToDo: Verification of the method being fool proof for every runtime and different combinations of watch and push triggered for binary and local source

cdrage · 2018-12-14T18:11:35Z

@anmolbabu

It looks like the tests are working as intended (I see that it failed in two sections..)

Make sure you look at @kadel 's PR too as he restores from backup so it may conflict with regards to your PR.

assemble-and-restart

anmolbabu · 2018-12-30T17:30:49Z

@anmolbabu

It looks like the tests are working as intended (I see that it failed in two sections..)

Make sure you look at @kadel 's PR too as he restores from backup so it may conflict with regards to your PR.

@cdrage Ack
Fortunately those lines remained untouched in my PR ;) So hopefully they shouldn't conflict

geoand

Tested and works!

geoand · 2019-01-11T09:36:15Z

assemble-and-restart

+        fi
+    done
+    # In case of java clear off the target directory
+    rm -fr ${ODO_S2I_SRC_BIN_PATH}/src/target


This is needed because java s2i does not delete the previous compilation output. If this weren't added, then deleted source files would not be reflected in the compilation output

This is java specific, I would like to avoid working with paths that are just for valid just for one type of builder images for now. We need a better way to handle that.

What if my project source that is in a different language contains src/target path for some reason?

Unfortunately, if we don't delete this directory, watch + delete is totally broken for Java.
Perhaps we should wrap the delete call in an if statement that checks if the image is a Java image (by testing for the existence of the mvn binary)?

Yeah I added the conditional check now
@kadel @geoand can you have a look now if it is ok?

@anmolbabu I don't think that using "$ODO_S2I_BUILDER_IMG" == "redhat-openjdk-18/openjdk18-openshift" is correct, because the image builder could have a different name, right?
I personally think that testing for the image capabilities (meaning testing if it has the mvn binary) is the correct approach.
An alternative could be perhaps to check for the existence of pom.xml in the pushed source code

The only other Java builder we use at the moment is the widlfly one and I haven't checked to see if it uses mvn clean.
My issue is basically with the name - are you sure the builder image name that is injected as an env var will always be the same no matter how I name it in my cluster?

Yeah this is the name of docker image(not the name it is imported as for ex: I have imported it as openjdk18 like in tests) corresponding to s2i image...
So it is likely not to change

In that case, I'm fine with the change as long as we check and see if widlfly needs the same kind of handling :)

What is exactly the issue that this fixes? I have tried a few Java examples and with this commented out and haven't any issues. Except hitting redhat-developer/odo#1117 again.

@kadel To see the problem this fixes you need to do the following:

Use the updated source example from this PR.

odo create ... odo push odo watch

Then delete one of the Java classes. Once the watch picks up the delete, the build should fail (since of the required sources no longer exists).
If you remove this line, then the build does not fail because it retains the previous compiled file

kadel · 2019-01-14T01:24:37Z

assemble-and-restart

+        fi
+    done
+    # In case of java clear off the target directory
+    rm -fr ${ODO_S2I_SRC_BIN_PATH}/src/target


This is java specific, I would like to avoid working with paths that are just for valid just for one type of builder images for now. We need a better way to handle that.

What if my project source that is in a different language contains src/target path for some reason?

cdrage

Hey @anmolbabu if you can organize the code a bit that'd be great. Add some newlines between code that's different! 👍

cdrage · 2019-01-14T21:17:27Z

assemble-and-restart

@@ -2,6 +2,34 @@
 set -x
 set -eo pipefail

+# If WorkingDir is injected as an env from odo and destination path is not equal to WorkingDir injected by odo


This sentence is confusing..

Ack I will fix it

cdrage · 2019-01-14T21:17:37Z

assemble-and-restart

+        if [ ! -d ${ODO_SRC_BACKUP_DIR} ]; then
+            mkdir -p ${ODO_SRC_BACKUP_DIR}/src
+        fi
+        # Backup the sources in DestinationDir to ODO_SRC_BACKUP_DIR bcoz the assemble script for some s2i images


bcoz = because

cdrage · 2019-01-14T21:18:39Z

assemble-and-restart

+            rm -fr $ODO_S2I_WORKING_DIR/$file
+        fi
+    done
+    # In case of java clear off the target directory


This is oddly specific... is there any way to make this more generic or do we actually have to hard-code in openjdk support?

This will be a future problem when newer versions of openjdk is released in the future of if the image name changes

Clearing off the output directory should not cause any problems, even if the builder image does this on it's own

See: #10 (comment)

anmolbabu · 2019-01-16T12:52:04Z

Don't know how to test, but want to make sure this would work:

When doing odo push and it's building, hit CTRL-C and break the execution of odo push. In current release, somehow backup dir is preserved (or saved, don't remember which) and further builds didn't work.

@jorgemoralespou I would say not in the scope of this PR
Do you mean hot Ctrl+c should break even an already executing assemble script?
odo doesn't intercept SIGINT at the moment I am not sure if there's a way to do it in Go(I have seen it in python though) so may be I can write a new PR for that bcoz I am really not sure how the SIGINT even if caught, can even be communicated to the component pod where the assemble is under execution

Cc: @kadel @cdrage @surajnarwade

kadel · 2019-01-17T13:24:11Z

assemble-and-restart

+    IFS=$b_IFS
+
+    # In case of java clear off the target directory
+    if [ -n "$ODO_S2I_BUILDER_IMG" ] && [ "$ODO_S2I_BUILDER_IMG" == "redhat-openjdk-18/openjdk18-openshift" ]; then


@anmolbabu please add TODO comment so we don't forget that this is a temporary hack and we need to find a proper solution for this.

kadel · 2019-01-17T13:31:32Z

@jorgemoralespou I would say not in the scope of this PR

This PR is already a significant change (because it is tight to redhat-developer/odo#1125) Let's fix this right after we are done with this.

Do you mean hot Ctrl+c should break even an already executing assemble script?
odo doesn't intercept SIGINT at the moment I am not sure if there's a way to do it in Go(I have seen it in python though) so may be I can write a new PR for that bcoz I am really not sure how the SIGINT even if caught, can even be communicated to the component pod where the assemble is under execution

We need to fix it, this was not an issue before we started doing all this backup and restore charades.
Odo should never leave component in a broken state where the only way to fix it is to delete the whole component.

kadel · 2019-01-17T13:56:13Z

@jorgemoralespou I just compiled odo with redhat-developer/odo#1125 that uses image from this PR to make it easy to test this anywhere.

You can download it here odo.zip ( It is compiled just for MacOS)

cdrage

Did some light testing, this LGTM 👍 code + comments + implementation.

good enough, don't block on me, but we need another review

anmolbabu · 2019-01-17T23:39:58Z

@kadel All comments incoporated please review

metacosm · 2019-01-18T15:19:05Z

I don't feel qualified to review this PR, sorry.

The error happens when the local component source contains a directory by name s2i. This fix clears the $APP_ROOT of all files and folders before the assemble script copies over sources from $destination dir to $app-root so that mv from assemble script does not compile that the folder already exists fixes redhat-developer/odo#1054 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

jorgemoralespou · 2019-01-21T12:05:13Z

run

-if [ -d /opt/app-root/backup ] && [ -n "$ODO_S2I_DEPLOYMENT_DIR" ]; then
-    cp -rf /opt/app-root/backup/. ${ODO_S2I_DEPLOYMENT_DIR}/
+if [ -d ${ODO_DEPLOYMENT_BACKUP_DIR} ] && [ -n "$ODO_S2I_DEPLOYMENT_DIR" ]; then
+    cp -rf ${ODO_DEPLOYMENT_BACKUP_DIR}/. ${ODO_S2I_DEPLOYMENT_DIR}/


Why everywhere in assemble-and-restart this has changed to rsync, but here it has remained as cp? same performance?

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

kadel · 2019-01-21T14:04:21Z

Tests are failing because the new image is not compatible with current odo. (It requires redhat-developer/odo#1125 to work properly)

kadel · 2019-01-21T14:07:40Z

run

-if [ -d /opt/app-root/backup ] && [ -n "$ODO_S2I_DEPLOYMENT_DIR" ]; then
-    cp -rf /opt/app-root/backup/. ${ODO_S2I_DEPLOYMENT_DIR}/
+if [ -d ${ODO_DEPLOYMENT_BACKUP_DIR} ] && [ -n "$ODO_S2I_DEPLOYMENT_DIR" ]; then
+    rsync -r ${ODO_DEPLOYMENT_BACKUP_DIR}/. ${ODO_S2I_DEPLOYMENT_DIR}/


it would be better to use rsync -a to make sure that it is really identical copy

kadel · 2019-01-21T14:07:43Z

assemble-and-restart

    fi
-   cp -rf ${ODO_S2I_DEPLOYMENT_DIR}/. /opt/app-root/backup/
+
+   rsync -r ${ODO_S2I_DEPLOYMENT_DIR}/. ${ODO_DEPLOYMENT_BACKUP_DIR}/


it would be better to use rsync -a to make sure that it is really identical copy

kadel · 2019-01-21T14:07:47Z

assemble-and-restart

+# After assemble script is run(which delets sources from destination dir by doing mv of them to the WorkingDir) and if only its component created from source,
+# copy back the sources from Source backup dir to DestinationDir for subsequent push of only updates by watch
+if [ ! -z "${ODO_S2I_WORKING_DIR}" ] && [ -n  "$ODO_SRC_BACKUP_DIR" ] && ([ "${ODO_S2I_SRC_BIN_PATH}" != "${ODO_S2I_WORKING_DIR}" ] || [ "${ODO_S2I_DEPLOYMENT_DIR}" != "${ODO_S2I_WORKING_DIR}" ]); then
+    rsync -r ${ODO_SRC_BACKUP_DIR}/src/. ${ODO_S2I_SRC_BIN_PATH}/src/


it would be better to use rsync -a to make sure that it is really identical copy

kadel · 2019-01-21T14:08:03Z

assemble-and-restart

+        # Backup the sources in DestinationDir to ODO_SRC_BACKUP_DIR because the assemble script for some s2i images
+        # moves sources from DestinationDir to WorkingDir thereby deleting it there(DestinationDir). So, back it up
+        # for partial push use cases like watch
+        rsync -r ${ODO_S2I_SRC_BIN_PATH}/src/. ${ODO_SRC_BACKUP_DIR}/src/


it would be better to use rsync -a to make sure that it is really identical copy

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

anmolbabu added the work in progress label Dec 12, 2018

anmolbabu mentioned this pull request Dec 17, 2018

propagate deletes during odo watch redhat-developer/odo#1125

Merged

anmolbabu force-pushed the fixes#1054 branch from 571d98b to 330e924 Compare December 19, 2018 09:44

anmolbabu commented Dec 19, 2018

View reviewed changes

assemble-and-restart Outdated Show resolved Hide resolved

anmolbabu commented Dec 19, 2018

View reviewed changes

assemble-and-restart Outdated Show resolved Hide resolved

anmolbabu force-pushed the fixes#1054 branch 3 times, most recently from 4f49cc6 to 643371e Compare December 21, 2018 16:35

anmolbabu removed the work in progress label Dec 22, 2018

anmolbabu requested review from cdrage, kadel, surajnarwade and mik-dass and removed request for cdrage December 22, 2018 13:23

anmolbabu force-pushed the fixes#1054 branch 3 times, most recently from d317677 to c6cd563 Compare December 23, 2018 06:57

anmolbabu force-pushed the fixes#1054 branch from c6cd563 to ce4ea2f Compare January 8, 2019 09:50

anmolbabu added the work in progress label Jan 8, 2019

anmolbabu force-pushed the fixes#1054 branch from ce4ea2f to cb9deec Compare January 8, 2019 11:15

geoand approved these changes Jan 8, 2019

View reviewed changes

anmolbabu removed the work in progress label Jan 8, 2019

geoand reviewed Jan 11, 2019

View reviewed changes

kadel requested changes Jan 14, 2019

View reviewed changes

cdrage requested changes Jan 14, 2019

View reviewed changes

anmolbabu force-pushed the fixes#1054 branch from 5aa4871 to d36b7c3 Compare January 17, 2019 09:07

kadel reviewed Jan 17, 2019

View reviewed changes

cdrage approved these changes Jan 17, 2019

View reviewed changes

anmolbabu force-pushed the fixes#1054 branch from d36b7c3 to deb827d Compare January 17, 2019 18:27

This was referenced Jan 18, 2019

watch on binary component does't work correctly redhat-developer/odo#1117

Closed

Integration test coverage for watch on binary component redhat-developer/odo#1121

Closed

anmolbabu added 2 commits January 18, 2019 20:55

Remove "target" folder allowing its fresh re-generation if java

6ecead9

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

anmolbabu force-pushed the fixes#1054 branch from deb827d to 3a1d3e0 Compare January 18, 2019 15:27

Incoporate @kadel and @cdrage comments

a9c010a

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

anmolbabu force-pushed the fixes#1054 branch from 3a1d3e0 to a9c010a Compare January 18, 2019 16:33

Enhance nodejs builds during re-used subsequent builds

174648c

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

anmolbabu added work in progress and removed work in progress labels Jan 19, 2019

jorgemoralespou reviewed Jan 21, 2019

View reviewed changes

Refactoring /opt/app-root/backup to ODO_DEPLOYMENT_BACKUP_DIR

3212197

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

anmolbabu force-pushed the fixes#1054 branch from 86b9218 to 3212197 Compare January 21, 2019 12:08

kadel requested changes Jan 21, 2019

View reviewed changes

Incoporate @kadel comments

ce90bea

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

kadel approved these changes Jan 21, 2019

View reviewed changes

kadel merged commit f4864e2 into redhat-developer:master Jan 21, 2019

anmolbabu mentioned this pull request Jan 25, 2019

Unable to push changes for source code for NodeJS redhat-developer/odo#1231

Closed

Fix the failed to mv error on python s2i #10

Fix the failed to mv error on python s2i #10

Conversation

anmolbabu commented Dec 12, 2018 • edited by kadel

anmolbabu commented Dec 12, 2018

cdrage commented Dec 13, 2018

anmolbabu commented Dec 14, 2018 • edited

cdrage commented Dec 14, 2018

anmolbabu commented Dec 30, 2018

geoand left a 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

geoand Jan 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolbabu Jan 14, 2019 • edited

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

cdrage left a 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

anmolbabu commented Jan 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel commented Jan 17, 2019

kadel commented Jan 17, 2019

cdrage left a comment

Choose a reason for hiding this comment

anmolbabu commented Jan 17, 2019

metacosm commented Jan 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel commented Jan 21, 2019

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

anmolbabu commented Dec 12, 2018 •

edited by kadel

anmolbabu commented Dec 14, 2018 •

edited

geoand Jan 14, 2019 •

edited

anmolbabu Jan 14, 2019 •

edited

anmolbabu commented Jan 16, 2019 •

edited