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

Job fixes for git repositories #3840

Merged

Conversation

glennmatthews
Copy link
Contributor

@glennmatthews glennmatthews commented Jun 1, 2023

Closes: #3856

What's Changed

  • Replace import_tasks_from_jobs_root with import_jobs_as_celery_tasks, handling Celery loading of system jobs, local jobs, and git jobs all together.
    • Git jobs are imported as <repo_slug>.jobs.<module>.<ClassName>
    • This change allows removing settings.CELERY_IMPORTS as unnecessary
  • Job class_path structure has been changed from "git.repo_slug/submodule_name/ClassName to simply "module_name.ClassName" throughout the code. (TODO: document this)
  • Job.source and Job.git_repository have been removed as no longer needed, redundant
    • Job.unique_together is now just ["module_name", "job_class_name"]
  • Job.slug has been removed.
  • refresh_git_jobs() now relies on the registered Celery tasks instead of on jobs_in_directory(), which is no longer needed and has been removed
  • get_jobs() and its helper _get_job_source_paths are no longer needed and have been removed
  • Accessing JobModel.job_class now just pulls from the Celery registry instead of looking for the job on disk and potentially resyncing a Git repository
  • refresh_job_models() has been reworked to rely on the registered Celery tasks instead of get_jobs()
  • I found that the unittest runner recursively imports everything it finds in a /tests/ subdirectory (edit: or a test_*.py file), which was resulting in all of the nautobot/extras/tests/example_jobs being directly imported and registered with Celery when running tests. This is undesirable; I fixed it by moving this directory to nautobot/extras/test_jobs (edit: and renaming all of the job files to remove the test_ prefix from them) so that it no longer gets auto-imported by the runner.

TODO

  • Explanation of Change(s)
  • Added change log fragment(s) (for more information see the documentation)
  • Attached Screenshots, Payload Example
  • Unit, Integration Tests
  • Documentation Updates (when adding/changing features)
  • Example Plugin Updates (when adding/changing features)
  • Outline Remaining Work, Constraints from Design
  • Does not yet handle ensuring that all workers are up to date with their Git repository clones when receiving Job tasks. I'll be tacking that next.

- Replace `import_tasks_from_jobs_root` with `import_jobs_as_celery_tasks`,
  handling Celery loading of system jobs, local jobs, and git jobs all together.
  - Git jobs are imported as `<repo_slug>.jobs.<module>.<ClassName>`
  - This change allows removing `settings.CELERY_IMPORTS` as unnecessary
- Job class_path structure has been changed from "git.repo_slug/submodule_name/ClassName"
  to simply "module_name.ClassName" throughout the code. (TODO: document this, fix up API endpoint)
- Job.source and Job.git_repository have been removed as no longer needed, redundant
  - Job.unique_together is now just `["module_name", "job_class_name"]`
- Job.slug has been removed.
- `refresh_git_jobs()` now relies on the registered Celery tasks instead of on
  `jobs_in_directory()`, which is no longer needed and has been removed
- `get_jobs()` and its helper `_get_job_source_paths` are no longer needed and have been removed
- Accessing `JobModel.job_class` now just pulls from the Celery registry instead of looking for
  the job on disk and potentially resyncing a Git repository
- `refresh_job_models()` has been reworked to rely on the registered Celery tasks instead of `get_jobs()`
nautobot/extras/jobs.py Outdated Show resolved Hide resolved
* Ensure GitRepostory.slug is a valid Python package name

* Tell base test slugify function

* Web views too

* Update nautobot/extras/migrations/0072_job__unique_name_data_migration.py

Co-authored-by: Gary Snider <75227981+gsnider2195@users.noreply.github.com>

* Clean method enforcement

* Update enforcement, slugify JS function, tests

* Revert validation error change

* Fix view tests

---------

Co-authored-by: Gary Snider <75227981+gsnider2195@users.noreply.github.com>
nautobot/extras/datasources/git.py Outdated Show resolved Hide resolved
nautobot/extras/signals.py Outdated Show resolved Hide resolved
@glennmatthews glennmatthews marked this pull request as ready for review June 2, 2023 19:50
nautobot/extras/jobs.py Outdated Show resolved Hide resolved
nautobot/extras/filters/__init__.py Show resolved Hide resolved
Co-authored-by: Bryan Culver <31187+bryanculver@users.noreply.github.com>
@bryanculver bryanculver added the type: housekeeping Changes to the application which do not directly impact the end user label Jun 5, 2023
@bryanculver bryanculver added this to the v2.0.0 milestone Jun 5, 2023
@bryanculver bryanculver linked an issue Jun 5, 2023 that may be closed by this pull request
3 tasks
@@ -192,6 +192,12 @@ def empty_repo(self, path, url, *args, **kwargs):
os.remove(os.path.join(path, "jobs", "__init__.py"))
return mock.DEFAULT

def assert_repo_slug_valid_python_package_name(self):
git_repository = GitRepository.objects.create(
name="1 Very-Bad Git_____Repo Name (2)", remote_url="http://localhost/git.git"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the slugify function/clean handle dots in the slug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They get discarded:

>>> from nautobot.core.models.fields import slugify_dashes_to_underscores
>>> slugify_dashes_to_underscores("Alpha.Beta-Gamma")
'alphabeta_gamma'

@glennmatthews
Copy link
Contributor Author

glennmatthews commented Jun 5, 2023

TODO at this time: when syncing a Git repository, jobs from the repository are not executable correctly/without errors until the Celery workers are restarted. This appears to be due to the use of a prefork pool on the workers - i.e. while we're refreshing the task registry in the main worker process, the preforked child processes that actually execute tasks are not refreshed and contain stale or nonexistent registry entries for the refreshed tasks. I don't have a solution in mind at this time.

Resolved in b7e2b0a with some more mucking around in Celery guts.

nautobot/core/utils/git.py Show resolved Hide resolved
nautobot/extras/jobs.py Outdated Show resolved Hide resolved
nautobot/extras/jobs.py Outdated Show resolved Hide resolved
@glennmatthews glennmatthews merged commit 63c2264 into feature/765-jobs-overhaul Jun 6, 2023
21 checks passed
@glennmatthews glennmatthews deleted the u/glennmatthews-765-git-job-fixes branch June 6, 2023 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: housekeeping Changes to the application which do not directly impact the end user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jobs Re-registration, Import Path, Refinements to Job Model
3 participants