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

Add repo sync option #1127

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Conversation

willtome
Copy link
Contributor

This PR adds the option sync to the repositories data structure to allow for an on-demand sync of a product when it is enabled. I believe this should be added to the current repositories role because it uses the same variable. The default behavior is not to sync. If sync: true the repos for that product will be synced. This is achieved using async operations so that multiple repos can be synced simultaneously.

closes #1119

@ehelms
Copy link
Member

ehelms commented Jan 12, 2021

There was a discussion, at length, in the original role PR (#1027) around this. In that PR, we avoided this as we landed on introducing a role for syncing rather than baking it into the repositories role. That would avoid duplicated logic (as a role for syncing is certainly useful) and would allow users to compose functionality to their desire. And the repositories role could potentially import the role if we went that route DRYing things up.

@willtome
Copy link
Contributor Author

@ehelms updated PR to introduce a new role rather than an extension to the existing role.

@evgeni
Copy link
Member

evgeni commented Jan 19, 2021

AI: @evgeni wants to link the old issue where the async style broke things

@evgeni
Copy link
Member

evgeni commented Jan 20, 2021

Hey @willtome,

we had the async: 9999, poll: 0 code as an example in the repository_sync module before, and have removed it (#838, #845) because we had reports that doing this with more than a few repositories would overload the system.

Is there a possibility to batch those syncs better? Like "only do X" at a time?

validate_certs: "{{ validate_certs | default(omit) }}"
organization: "{{ organization }}"
product: "{{ item.0.name }}"
repository: "{{ item.1.name }}"
Copy link
Member

Choose a reason for hiding this comment

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

If you omit repository the module will sync the whole product, which in most cases is what the user wants to do anyways.

Copy link
Member

Choose a reason for hiding this comment

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

If we go for the batching, we might want to iterate over the repositories to even out the load better.
If we do one product at a time, this might just be the easiest approach to batching.

Just my two thoughts here...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, agree, "by product" is probably the cheapest batching we can get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Will update to just sync by product and not async. Can an issue be opened to better queue requests in foreman? At the end of the day, it really shouldn't be the automation's responsibility to batch requests and manage load for the system.

product: "{{ item.0.name }}"
repository: "{{ item.1.name }}"
with_subelements:
- "{{ products | selectattr('repository_sets', 'defined') | list }}"
Copy link
Member

Choose a reason for hiding this comment

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

this means it won't sync any custom repositories that the repositories role can create.

@evgeni evgeni added this to the Content management workflow milestone Feb 3, 2021
Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

this needs tests -- do you know how to record those, or do you need help with them?

Comment on lines +9 to +13
product: "{{ item.0.name }}"
with_subelements:
- "{{ products | selectattr('repository_sets', 'defined') | list }}"
- repository_sets
when: item.0.sync | default(false) | bool
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
product: "{{ item.0.name }}"
with_subelements:
- "{{ products | selectattr('repository_sets', 'defined') | list }}"
- repository_sets
when: item.0.sync | default(false) | bool
product: "{{ item.name }}"
loop: "{{ products }}"
when: item.sync | default(false) | bool

There is no need to iterate over the repositories anymore, as we're syncing by-product now.
Also loop over all products, regardless of repository_sets (RH repos) or repositories (custom repos) attributes.

@@ -81,4 +81,4 @@
verify_ssl_on_sync: "{{ item.1.verify_ssl_on_sync | default(omit) }}"
with_subelements:
- "{{ products | selectattr('repositories', 'defined') | list }}"
- repositories
- repositories
Copy link
Member

Choose a reason for hiding this comment

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

this looks unrelated and unneeded?

Copy link
Contributor

@wbclark wbclark left a comment

Choose a reason for hiding this comment

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

Hi @willtome , thanks for the PR!

could you add some tests to this role? The documentation at https://theforeman.github.io/foreman-ansible-modules/develop/testing.html is really helpful for this

In short, what you need is a test playbook tests/test_playbooks/repositories_sync_role.yml and an apidoc fixture tests/fixtures/apidoc/repositories_sync_role.json which should just be a symlink to katello.json in the same dir

To record the test fixtures, there is a little first time setup: make test-setup and then edit tests/test_playbooks/vars/server.yml to point to your katello server. Once that setup is complete make record_repositories_sync_role would then record the fixtures.

You can look at the PRs for other roles to get an idea how to do this, or please feel free to reach out any time if you have any questions

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.

Repository Sync Role
5 participants