-
Notifications
You must be signed in to change notification settings - Fork 297
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
Fixes #35959 - Use structured APT for deb content #11058
Conversation
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.
This "review" (and the below comments) aims to explain the design approach for this PR.
Regarding the current state of this PR:
- I have tested the new "structured APT" functionality as follows:
- Created an APT repo, synced it, and added it to a CV version and LCENV. Registered a host and checked the APT repo components and distirbutions (structure information) make it all the way into the repo file on the registered host, as appropriate for the environment it is in. This works.
For deb content, the current state does not yet update the corresponding KatelloContent, whenever some candlepin content is updated. I have not found this to impact the functionality, but I will continue to work on this for the sake of consistency and to avoid problems down the road.
In my local dev environment I currently see two test failures relative to master branch:
ktest test/actions/pulp3/orchestration/export_test.rb
ktest test/lib/tasks/pulpcore/repository_vcr_test.rb
The failing VCR test does not turn green from re-recording the cassette. It complains about a call to POST https://localhost:23443/candlepin/environments/1/content
, but only records calls to Pulp.
The other test appears to attempt calling a pulp_deb API endpoint from a Yum API object, which does not make any sense. I was so far unable to figure out why this is, but I think it is likely an issue with the test rather than the code (not 100% sure though).
What I am hoping for at this point:
It would be great to receive some initial feedback if the design approach for this feature is sound. In particular, an initial review should focus on potential risks to non-deb content types. The idea is that from the point of view of yum and other non-deb types, this is a pure refactor, that does not change any outcomes! Thanks in advance for any and all input!
f5f1ae4
to
2e4b66a
Compare
The latest push adds the following fixes:
Remaining ToDo's:
|
This is the reference used for rerecording Candlepin VCR tests:
I noticed that test uses scenario_support.rb, which has its own record definition: def record(name, options = {})
VCR.insert_cassette("scenarios/#{name}", options)
User.current = user
yield
rescue
raise
ensure
VCR.eject_cassette
end This is kinda tricky because that test may need to do both candlepin and Pulp recordings at the same time. Another idea would be to stub out the live Candlepin bits. |
I will be on PTO from 2024-07-16 to 2024-07-28. During that time @m-bucher will be my replacement for questions regarding this PR. Before I go, I want to leave some pointers on how one can test/review this feature: For non-deb content types (or deb with structured APT disabled)All workflows should produce the same outcomes as before. Since some of the codepaths to get to those outcomes have changed slightly, carefull review and broad testing is necessary. How to test the new structured APT features
|
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.
Code structure is generally looking fine to me, just one small comment. I need to give it a test now.
Should this PR still be in draft form? |
I just came back from PTO/am continuing on this PR now, currently there are three things I am waiting on to undraft:
|
d34bc51
to
ed79dbd
Compare
Update: I rebased to latest master and re-recorded VCR cassettes (because of conflicts). For me all tests are now locally green. I have two remaining TODOs before un-drafting:
|
Update: "repository sets" query for deb content (with structured APT enabled) is partially fixed, but needs to be refined further. |
Update: To my best knowledge the feature itself is now ready for final/full review. I am currently reworking the enable/disable rake task. |
Update: Enable/disable rake task is 70% done. There are three TODOs two of which should be pretty trivial. Since I won't be working tomorrow, and Monday is full of meetings, I estimate I will be ready to undraft on Tuesday. |
I haven't been finding time lately to test this -- @quba42 if someone is able provide a testing report with steps taken from your team, it'd make it easier for us to get more eyes on it. |
Update: Unfortunately I found that handling of the Library environment is currently broken. (Non-Library environments work fine). I am working on fixing this. I will also add a testing report what has been tried already. |
Update: I have fixed the handling of Library content view environments. I have also fixed the enable/disable rake task to the point where it appears to be working in both directions, at least for my relatively simple initial test case (more testing is needed). This means I now have a state that is to the best of my knowledge both complete and working. I will now rebase to latest master branch and re-record vcr_cassettes. Then I will undraft the PR. |
504f4d4
to
8095086
Compare
Update: Pushed a minor cosmetic change. 😉 |
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.
Some minor nitpicks below.
@parthaa Can you take a look specifically at the changes to Dynflow actions and see if they look good to you?
aa375eb
to
295cf69
Compare
Update: The latest state incorporates the latest set of suggestions by @jeremylenz |
Incomplete list of things I have tested (split up into several parts): Testing the change with the feature disabledThe point of these tests is to show that this change is a pure refactor for content types other than deb (as well as for deb with the feature disabled). For some time now, all our internal builds have been using this change with the feature disabled. This includes large production like systems that were upgraded to this state. This means everything that was done on these systems has tested the "disabled" case. Some things that are explicitly worth mentioning:
For all of these reasons, I am very confident the refactor part is just that. Testing the enable/disable rake task
Testing of the feature itself (deb content with the feature enabled)
Detailed description of a front to back test case.
|
f916f53
to
d866593
Compare
Note that this feature must be enabled/disabled using: foreman-rake katello:enable_structured_content_for_deb
d866593
to
3a77258
Compare
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.
Had a meeting with @quba42 just now and looked through/discussed the latest changes again.
As far as I am concerned, this is good to go now 👍
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.
I did some testing for non-deb content and I was able to run the migrations and able to sync different non-deb repo types, have their content be setup correctly in candlepin and artifacts created in pulp..Pulp DB looked rightly setup and registration workflows went through fine and a subscribed host is able to consume the distributed content. Based on the code, most of the tasks updated look to be running successfully.
Note that this is not an exhaustive test but based on the code changes, ATIX testing, PRT testing and other reviews, this looks in a good state for merge.
ACK 🎉
Thanks a ton to everyone involved! |
RFC: https://community.theforeman.org/t/rfc-transitioning-katello-to-structured-apt-deb-content/34262
Issue: https://projects.theforeman.org/issues/35959
Replaces: #10420
Requires: candlepin/subscription-manager#3223
Note that the subscription-manager packages for Debian and Ubuntu published on http://oss.atix.de/ are already patched to support these changes.