-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
wait for database connection #10583
wait for database connection #10583
Conversation
Build succeeded.
|
Build succeeded.
|
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @coolbry95 - thank you very much for tracking this down and proposing a patch. This isn't quite the way I'd like to handle this, but it helps a lot.
Could you please try adding this line: https://github.com/ansible/awx/blob/devel/tools/ansible/roles/dockerfile/files/launch_awx_task.sh#L16
To somewhere around here: https://github.com/ansible/awx/blob/devel/tools/ansible/roles/dockerfile/files/launch_awx.sh#L15
I believe this should also resolve your issue.
That did not work.
|
It looks like this command maybe isn't waiting until it can connect the the database? There are more exceptions that get thrown besides the first one I have in my previous comment. |
Oh maybe the wait for migrations is failing because of the same init code. I am trying to debug what is happening here.
|
Ok the wait for migrations doesn't work because the logic in it is wrong.
The exit code is always 0 for this thus it always immediately returns. If you change the return to an echo it works properly. Any thoughts on how you want to fix this? We could catch the exception still and then exit with a different code. I will update the PR to reflect that. |
After more testing I know what is happening. wait-for-migrations returns because grep does not find any "[ ]" because awx-manage showmigrations is throwing an exception because it can't reach the database. Either there needs to be a different validation check that all migrations ran or we need to wait and try for a database connection which my PR is currently doing. I am not sure how to change the logic for showmigrations. Any thoughts @shanemcd? I apologize for the spam I was just trying to test using an unmanaged database with the awx-operator and started testing some more. |
Build succeeded.
|
With the latest commit both awx-task and awx-web work correctly when deployed with a fresh unmnaged database in the awx-operator. |
Hi @coolbry95 - I'd like to fix up the |
Ok how do you propose fixing wait-for-migrations? If it can't connect then it throws an exception and there is not any migration output that it is currently grepping for. We could save the output to a variable then check for the word exception or the like and try again if we see that? Would we also add a check in the launch script to check for a proper exit code from wait-for-migrations then fail if the exit code is bad? If you are on irc we can chat there if possible. |
I took a stab at this. Not sure on how else to do it. |
Build succeeded.
|
b24576c
to
38d2e93
Compare
@shanemcd much smaller change. The logic is correct now. I have tested this in the same cluster the other issues arose in. I am going to do a few more tests. Hopefully you will have time to review and merge before the next release. |
Signed-off-by: coolbry95 <coolbry95@gmail.com>
…check-postgres-version-4940 introduced a pre-flight check for postgres 12
SUMMARY
Wait for the database connection because in some environments the connection cannot be made right away.
#10527
ISSUE TYPE
COMPONENT NAME
AWX VERSION
ADDITIONAL INFORMATION
We could also stop after so many tries. Not sure what the point in continuing is if you can't use the database though.
I also tried just skipping past this check if the database was not available but it would fail later on just running collectstatic so the database is needed even for collectstatic.
Before
After