Improve app state checking and fix deployment issues #21
Conversation
|
I've used |
Add logging to deployment/undeployment to help debug occasional deployment issues.
Fixes OpenLiberty#18 where the container very briefly sees the app in INSTALLED state before liberty begins starting it and assumes that it has failed to start. Also did some refactoring and cleanup of the application state checking code.
Fixes OpenLiberty#16 by ensuring the apps and dropins directories are created if they're needed. Fixes OpenLiberty#18 by exporting the app to a temporary location and then moving it into the dropins directory.
Azquelt
force-pushed
the
improve-app-state-checking
branch
from
ac4186b
to
4776c49
Compare
Azquelt
changed the title
|
Did a few more liberty builds with these changes and I'm pretty convinced that it fixes the issues we've been seeing. It's ready to be reviewed and merged. |
| @@ -398,6 +387,9 @@ public ProtocolMetaData deploy(final Archive<?> archive) throws DeploymentExcept | |||
| try { | |||
| // If the deployment is to server.xml, then update server.xml with the application information | |||
| if (containerConfiguration.isDeployTypeXML()) { | |||
| if (log.isLoggable(FINER)) { | |||
| log.finer("Deploying using server.xml"); | |||
There was a problem hiding this comment.
This is OK but I might have included the archiveName in the log msg.
There was a problem hiding this comment.
I don't think having the archive name here is necessary as it'll be included with the archive details (logged at the same level on line 376)
| appMessageStatus.append("Timeout while waiting for \"") | ||
| .append(appStateChecker.getAppName()) | ||
| .append("\" ApplicationMBean to reach ") | ||
| .append(desiredState); |
There was a problem hiding this comment.
I can't see where it logs the actual state (what used to be "status") at the error level or in then exception. It is logged as currentState in line 951 but at a logging level that is less likely to be operative when an unexpected problem occurs?? [Unless one thinks of Arquillian being used for test only and thus mostly at high log levels???)
Short description of what this resolves:
Avoid the case where the container sees the app in
INSTALLEDstate and assumes that it has failed to start when actually liberty hasn't attempted to start it yet.Avoid the case where liberty tries to start an application deployed to
dropinsbefore the container has finished writing it.Create the
appsanddropinsdirectories if they do not exist and are required.Changes proposed in this pull request:
INSTALLEDstate for 1 second (this should be plenty of time, usually it moves fromINSTALLEDtoSTARTINGso quickly that the container doesn't even see it.appsordropinsdirectories, create them if they don't exist.Fixes: #18 #16