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

Move plugin loading to dispatcher startup #15738

Draft
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

AlanCoding
Copy link
Member

SUMMARY

If you read some of the comments of the methods moved here, it was very clear even when implemented that you're not supposed to be interacting with the database in the .ready method.

To summarize some discussion:

  • the dispatcher startup is the "middle-ground" solution, not too often, not too infrequently
    • the migration hook would be much more infrequent, but awx-operator does not reliably call it every startup
    • the ready method calls every time ever service or any other script using Django starts
  • The remaining concern is that you could have a race condition, expecting a CredentialType for configuration-as-code, which is technically possible
    • This is not actually realistic in my opinion
    • If there is a problem, scripts can poll until the capacity of instances reaches a non-zero value, which happens after these methods in the startup method
  • I am very opinionated to move away from standard Django practices, as I have another patch up at Move PG version check to awx-manage check_db & migrate commands #15463 to remove another case where another query-on-import was performed. Why?
    • Log maintenance becomes a nightmare because these categorically front-run any other database errors, which I spend a lot of time debugging
    • People will find it more and more difficult to work with AWX because it doesn't use Django out-of-the-box. If there were a really strong reason to break standard practice, I won't resist, we may have to do. This does not appear to be a strong reason.
ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • API

@AlanCoding
Copy link
Member Author

Running the development environment seems to go fine with this. Additionally, making sure methods are well-behaved:

In [1]: from awx.main.tasks.system import *

In [2]: load_inventory_plugins()

In [3]: load_credential_types_feature()

The 2nd one takes longer (maybe 0.1 second) but not worried in any case.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant