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

wildfly component gets broken after storage is added #445

Closed
kadel opened this issue May 10, 2018 · 23 comments · Fixed by #1041
Closed

wildfly component gets broken after storage is added #445

kadel opened this issue May 10, 2018 · 23 comments · Fixed by #1041
Assignees
Labels
estimated-size/L (20-40) Rough sizing for Epics. About 2 sprints of work for a person. kind/bug Categorizes issue or PR as related to a bug. priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)).
Projects

Comments

@kadel
Copy link
Member

kadel commented May 10, 2018

Steps to reproduce

git clone https://github.com/marekjelen/katacoda-odo-backend.git
cd /katacoda-odo-backend.
odo create wildfly
odo push
odo url create

# go to `<url>/counter`
# counter works

odo storage create data --path /data --size 100M

# wait a bit
# go to `<url>/counter`
# 404 - Not Found

odo push
# go to `<url>/counter`
# counter works

Reason why component gets broken is because widlfly assemble script moves compiled files to /wildfly/standalone/deployments

Moving all war artifacts from /opt/app-root/src/target directory into /wildfly/standalone/deployments for later deployment...
'/opt/app-root/src/target/ROOT.war' -> '/wildfly/standalone/deployments/ROOT.war'
removed '/opt/app-root/src/target/ROOT.war'

Path /wildfly/standalone/deployments is not backed by persistent volume, so when deploymentConfig is modified (new mount is added) and new deployment is triggered we lose all data that were not in persistent volume.

This is wildfly specific issue. When I tested this with nodejs or php images everything was ok, as they leave and serve all data from /opt/app-root/ and we create persistent volume for this path by default.

@kadel kadel added kind/bug Categorizes issue or PR as related to a bug. priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)). size/M labels May 10, 2018
@kadel kadel added this to To do in Sprint 149 via automation May 10, 2018
@kadel kadel self-assigned this May 10, 2018
@kadel kadel moved this from To do to In progress in Sprint 149 May 22, 2018
@kadel
Copy link
Member Author

kadel commented May 22, 2018

Possible solutions:

  • a) detect if the builder image is wildfly and add volume for /wildfly/standalone/deployments
  • b) create special s2i builder image for widlfly and is that
  • c) run assemble script when the container starts

a) and b) are fixes that are specific for widlfly image, there might be other builder images with similar issue, and we will have to create a separate fix for each one. (component based on httpd image gets also broken after storge is added)

c) is a generic solution and should work for every assemble script that is copying compiled or processed artifacts away from /opt/app-root/.
The only downside of this that I can think of right now is that It shows down container startup time (component creation time)

@kadel
Copy link
Member Author

kadel commented May 22, 2018

What do you think @jorgemoralespou ?

@kadel
Copy link
Member Author

kadel commented May 22, 2018

c) is a generic solution and should work for every assemble script that is copying compiled or processed artifacts away from /opt/app-root/.
The only downside of this that I can think of right now is that It shows down container startup time (component creation time)

I forgot that wildfly script moves files from its original location. So to this approach won't work :-(

@jorgemoralespou
Copy link
Contributor

@kadel I think we should look into having a pre-backed image with a supervisor in go (this could be a good start: https://github.com/ochinchina/supervisord) and have all the required logic scripts in that init-container image.
Then any preparation for any s2i image would be as simple as run this initContainer (or possibly 2 initContainers)

@GrahamDumpleton, it would be awesome if you get to help us make this task as lean as possible

@kadel
Copy link
Member Author

kadel commented Oct 15, 2018

@cdrage can you please verify if this is still an issue with new supevisord?
I expect that nothing changed here. We should start looking into the solution as this will be part of #807

@kadel kadel mentioned this issue Oct 15, 2018
6 tasks
@cdrage
Copy link
Member

cdrage commented Oct 15, 2018

@kadel

I can confirm that this still does not work..

github.com/redhat-developer/odo  master ✔                                                                                                                                                                                                                                                                                                                            1h20m
▶ ./odo push
Pushing changes to component: wildfly
Please wait, building component....
+ set -eo pipefail
+ '[' -f /opt/app-root/src/.s2i/bin/assemble ']'
+ '[' -f /usr/local/s2i/assemble ']'
+ /usr/libexec/s2i/assemble
cp: cannot stat '/opt/s2i/destination/src/.': No such file or directory
Found pom.xml... attempting to build with 'mvn package -Popenshift -DskipTests -B -s /opt/app-root/src/.m2/settings.xml'
Apache Maven 3.5.4 (1edded0938998edf8bf061f1ceb3cfdeccf443fe; 2018-06-17T18:33:14Z)
Maven home: /usr/local/apache-maven-3.5.4
Java version: 1.8.0_181, vendor: Oracle Corporation, runtime: /usr/lib/jvm/java-1.8.0-openjdk-1.8.0.181-3.b13.el7_5.x86_64/jre
Default locale: en_US, platform encoding: ANSI_X3.4-1968
OS name: "linux", version: "3.10.0-862.6.3.el7.x86_64", arch: "amd64", family: "unix"
[INFO] Scanning for projects...
[INFO] 
[INFO] -------------------< eu.mjelen.katacoda.odo:backend >-------------------
[INFO] Building PersistentStorageCounterServlet 1
[INFO] --------------------------------[ war ]---------------------------------
[INFO] 
[INFO] --- maven-resources-plugin:2.6:resources (default-resources) @ backend ---
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] skip non existing resourceDirectory /opt/app-root/src/src/main/resources
[INFO] 
[INFO] --- maven-compiler-plugin:3.1:compile (default-compile) @ backend ---
[INFO] Nothing to compile - all classes are up to date
[INFO] 
[INFO] --- maven-resources-plugin:2.6:testResources (default-testResources) @ backend ---
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] skip non existing resourceDirectory /opt/app-root/src/src/test/resources
[INFO] 
[INFO] --- maven-compiler-plugin:3.1:testCompile (default-testCompile) @ backend ---
[INFO] No sources to compile
[INFO] 
[INFO] --- maven-surefire-plugin:2.12.4:test (default-test) @ backend ---
[INFO] Tests are skipped.
[INFO] 
[INFO] --- maven-war-plugin:2.3:war (default-war) @ backend ---
failed to push component: wildfly

@jorgemoralespou
Copy link
Contributor

@kadel @cdrage This problem can be easily bypassed by providing along the sources a modified .s2i/bin/assemble that instead of move does a copy. We should probably also do a PR to the wildfly image so that every version copies those files instead of moving, but we need to think this can be a problem and we should look into proper solutions.

IMHO we will need to provide a dedicated supervisor process that will have intelligence, and that will also be able to be configured to alter this intelligent behavior in some cases.

Also, I found while doing some tests in online a couple of things, which might need to go to a different issue, but wanted to mention here.

  • Deployment should need to be changed from rolling to recreate. This is probably something that should go to the config, but IMHO for inner loop development, by default recreate should be preferred anyways.
  • While building with maven in a container in online (500 MB by default), the -Xmx parameter was configured too low. I needed to alter the memory configured to the container. There's currently no way to alter the deployment. We need to work on these CLI flags to modify behavior, memory/cpu being the most critical one.

@GrahamDumpleton
Copy link

FWIW. Many S2I images originally used cp in assemble. I said I disagreed with them when they changed to using mv and one of the reasons was that assemble would break when there were no files. They may not want to change it back to cp.

@mik-dass
Copy link
Contributor

@GrahamDumpleton Is there any way to prevent this behaviour? Any env variable or any suggestion to prevent this?

@kadel kadel added this to To do in Sprint 157 via automation Oct 24, 2018
@kadel kadel added size/L and removed size/M labels Oct 24, 2018
@geoand
Copy link
Contributor

geoand commented Nov 2, 2018

I am pretty sure that if we go down the go binary route, we will be able to make supervisord work seamlessly with s2i in a maintainable manner (much more than the bash scripts that test for the presence directories :P )

@surajnarwade
Copy link
Contributor

surajnarwade commented Nov 2, 2018

@kadel , we already discussed about this label: com.redhat.deployments-dir
as per our discussion we tried shell script hack and @mik-dass , can you please replace shell script with this com.redhat.deployments-dir and try out ?

also this might be the issue only in case of language runtimes which generates artifacts, and we should also mention that if someone is using custom imagestream, please use this labels there

@kadel
Copy link
Member Author

kadel commented Nov 2, 2018

also this might be the issue only in case of language runtimes which generates artifacts, and we should also mention that if someone is using custom imagestream, please use this labels there

this label is not label in imagestream but container (docker) image label

@kadel kadel added this to To do in Sprint 158 via automation Nov 14, 2018
@kadel kadel removed this from To do in Sprint 157 Nov 14, 2018
@kadel
Copy link
Member Author

kadel commented Nov 14, 2018

@mik-dass - will work on odo/client side
@surajnarwade - initContainer / cluster side

@geoand
Copy link
Contributor

geoand commented Nov 14, 2018

I'll be happy to assist with this one should anyone need it

@kadel kadel moved this from To do to In progress in Sprint 158 Nov 14, 2018
surajnarwade added a commit to surajnarwade/odo-supervisord-image that referenced this issue Nov 20, 2018
surajnarwade added a commit to surajnarwade/odo-supervisord-image that referenced this issue Nov 20, 2018
surajnarwade added a commit to surajnarwade/odo-supervisord-image that referenced this issue Nov 21, 2018
surajnarwade added a commit to surajnarwade/odo-supervisord-image that referenced this issue Nov 21, 2018
surajnarwade added a commit to surajnarwade/odo-supervisord-image that referenced this issue Nov 21, 2018
surajnarwade added a commit to surajnarwade/odo-supervisord-image that referenced this issue Nov 21, 2018
surajnarwade added a commit to surajnarwade/odo-supervisord-image that referenced this issue Nov 21, 2018
surajnarwade added a commit to surajnarwade/odo-supervisord-image that referenced this issue Nov 21, 2018
Sprint 158 automation moved this from In progress to Done Nov 26, 2018
@kadel
Copy link
Member Author

kadel commented Nov 26, 2018

reopening as odo main repository has not been updated with the new image yet

@kadel kadel reopened this Nov 26, 2018
Sprint 158 automation moved this from Done to In progress Nov 26, 2018
Sprint 158 automation moved this from In progress to Done Nov 26, 2018
@kadel
Copy link
Member Author

kadel commented Nov 26, 2018

🎉

@rm3l rm3l added the estimated-size/L (20-40) Rough sizing for Epics. About 2 sprints of work for a person. label Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
estimated-size/L (20-40) Rough sizing for Epics. About 2 sprints of work for a person. kind/bug Categorizes issue or PR as related to a bug. priority/High Important issue; should be worked on before any other issues (except priority/Critical issue(s)).
Projects
Archived in project
Sprint 158
  
Done
Development

Successfully merging a pull request may close this issue.

8 participants