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

Jump to bottom

Kickstart syncing #1427

Merged
merged 1 commit into from Sep 4, 2019
Merged

Kickstart syncing #1427

merged 1 commit into from Sep 4, 2019

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Aug 27, 2019

depends on: #1418

closes #5202
https://pulp.plan.io/issues/5202
Required PR: #1418

@fao89 fao89 requested a review from a team August 27, 2019 19:06
@fao89 fao89 mentioned this pull request Aug 27, 2019
Merged
@fao89 fao89 force-pushed the 5202-kickstart-sync branch 3 times, most recently from e9be1ca to 8cb7e01 Compare August 28, 2019 14:27
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Aug 28, 2019
@fao89
publishing kickstarts
c269a30 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
@fao89 fao89 mentioned this pull request Aug 28, 2019
Merged
@fao89 fao89 force-pushed the 5202-kickstart-sync branch 3 times, most recently from 820f865 to abc00cf Compare August 28, 2019 22:11
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Aug 28, 2019
@fao89
publishing kickstarts
f7056ae 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
@fao89 fao89 force-pushed the 5202-kickstart-sync branch 2 times, most recently from c43191c to 8b5fbab Compare August 29, 2019 13:20
nixocio
nixocio approved these changes Aug 29, 2019
View reviewed changes
Copy link

@nixocio nixocio left a comment

Choose a reason for hiding this comment

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

@fabricio-aguiar, thanks for adding this test.

@fao89 fao89 force-pushed the 5202-kickstart-sync branch 8 times, most recently from 6770f96 to 87ddb1c Compare September 2, 2019 18:36
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Sep 2, 2019
@fao89
publishing kickstarts
c1794d2 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Sep 2, 2019
@fao89
publishing kickstarts
835ee9b 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
fao89
fao89 commented Sep 2, 2019
View reviewed changes
pulp_rpm/tests/functional/api/test_sync.py Outdated
@@ -100,6 +102,70 @@ def test_rpm(self):
)
self.assertDictEqual(get_added_content_summary(repo), {})

@unittest.skip("Kickstart fixture with wrong checksums")
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to put this because the fixture seems to be with the wrong checksums:
https://repos.fedorapeople.org/pulp/pulp/fixtures/rpm-kickstart/.treeinfo

Copy link
Member Author

Choose a reason for hiding this comment

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

LiveOS/squashfs.img

  • current = 158657d4ff270db50204ae70c2256ca1523ee4f3c23eeedbabc3f723b69163a2
  • calculated = 30e14955ebf1352266dc2ff8067e68104607e750abb9d3b36582b8af909fcb58

images/pxeboot/initrd.img

  • current = 094631a67f1766688dfcf4ec1f07ab0921100878736afc3cc1b31f69e7b517a1
  • calculated = 30e14955ebf1352266dc2ff8067e68104607e750abb9d3b36582b8af909fcb58

images/pxeboot/upgrade.img

  • current = bc884c1b4b520e57f81f3bf43156365d9ef651a47266b21e2c3d00b09e0da46b
  • calculated = 30e14955ebf1352266dc2ff8067e68104607e750abb9d3b36582b8af909fcb58

images/pxeboot/vmlinuz

  • current = 91052b444e73f3eebdb93d1fb1506597e96c92d8de9c1e3c3f36b07a57d0a18f
  • calculated = 91052b444e73f3eebdb93d1fb1506597e96c92d8de9c1e3c3f36b07a57d0a18f

Copy link
Member

Choose a reason for hiding this comment

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

I agree the checksums are incorrect. In my own testing I found this also:

[bmbouter@localhost pulp_ansible]$ sha256sum ~/Downloads/squashfs.img 
30e14955ebf1352266dc2ff8067e68104607e750abb9d3b36582b8af909fcb58  /home/bmbouter/Downloads/squashfs.img

@fao89 fao89 force-pushed the 5202-kickstart-sync branch 3 times, most recently from e04b270 to 3bd69bf Compare September 3, 2019 16:14
bmbouter
bmbouter reviewed Sep 3, 2019
View reviewed changes
pulp_rpm/app/models.py Outdated
content_artifact = ContentArtifact.objects.filter(
content=self.distribution_tree,
relative_path=self.path,
).get()
Copy link
Member

Choose a reason for hiding this comment

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

If this property is fetched while the DistributionTree was sync'd with policy=on_demand I think this will fail here.

What motivates having this here?

Copy link
Member Author

@fao89 fao89 Sep 4, 2019 •
edited

Choose a reason for hiding this comment

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

I did it for linking Image and Artifact:

"images": [
                {
                    "name": "initrd",
                    "path": "images/pxeboot/initrd.img",
                    "platforms": "x86_64, xen",
                    "artifact": {
                        "_href": "/pulp/api/v3/artifacts/d6ddfc54-fe44-485d-9c3a-834a35aa8675/",
                        "_created": "2019-09-03T16:06:16.263824Z",
                        "file": "artifact/56/47f05ec18958947d32874eeb788fa396a05d0bab7c1b71f112ceb7e9b31eee",
                        "size": 2097152,
                        "md5": "b2d1236c286a3c0704224fe4105eca49",
                        "sha1": "7d76d48d64d7ac5411d714a4bb83f37e3e5b8df6",
                        "sha224": "7841241795c51351bb1b7dc7d41ea855eddf4143e8925c227490c390",
                        "sha256": "5647f05ec18958947d32874eeb788fa396a05d0bab7c1b71f112ceb7e9b31eee",
                        "sha384": "6f71dee19ba3fbdc5c15e857c98727eb91c318321c1c8d5a716a6b5d1b0404acb2a62fd975562545701013ec7f99329f",
                        "sha512": "731859029215873fdac1c9f2f8bd25a334abf0f3a9e1b057cf2cacc2826d86b0c26a3fa920a936421401c0471f38857cb53ba905489ea46b185209fdff65b3b6"
                    }
                },

for policy=on_demand:

"images": [
                {
                    "name": "upgrade",
                    "path": "images/pxeboot/upgrade.img",
                    "platforms": "x86_64, xen",
                    "artifact": null
                },

bmbouter
bmbouter reviewed Sep 3, 2019
View reviewed changes
pulp_rpm/app/tasks/synchronizing.py Outdated
for path, checksum in self.kickstart["download"]["images"].items():
artifact = Artifact()

if checksum:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure, but can't this loop also be handled as L#279 being written as: artifact = Artifact(**checksum) and this loop could be dropped?

bmbouter
bmbouter reviewed Sep 3, 2019
View reviewed changes
pulp_rpm/app/tasks/synchronizing.py
)
# TODO: decide how to distinguish between a mirror list and a normal repo
result = await downloader.run()
metadata_pb.increment()

if self.kickstart:
d_artifacts = []
for path, checksum in self.kickstart["download"]["images"].items():
Copy link
Member

Choose a reason for hiding this comment

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

Will this part also sync the part of this .treeinfo file that has:

[stage2]
mainimage = images/install.img

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it does sync this part:
https://github.com/fabricio-aguiar/pulp_rpm/blob/5202-kickstart-sync/pulp_rpm/app/tasks/utils.py#L170-L174
but it will only stay in distribution_tree._artifacts because I don't create an Image form them if it does not appear on the checksums section.

bmbouter
bmbouter reviewed Sep 3, 2019
View reviewed changes
pulp_rpm/tests/functional/api/test_sync.py
RPM_KICKSTART_FIXTURE_SUMMARY
)

# Sync the repository again.
Copy link
Member

Choose a reason for hiding this comment

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

I don't see additional value in this re-sync aspect of this test given that it's in other tests already.

What is important for on_demand correctness assertion is that the content that is available "on_demand" can actually be fetched by Pulp smash. Can you assert that additionally via get_content() calls and validating the checksums of the content you are recieving. This would simulate a client actually fetching it which is key.

@bmbouter
Copy link
Member

bmbouter commented Sep 3, 2019

One special case we need to handle (and don't yet AIUI) is where the treeinfo file has a Variant that references the "main repository" with something like repository = . as in this example.

In that case the main sync machinery is already handling it and so the Variant needs to only preserve the data such as:

[variant-Server]
id = Server
name = Server
packages = Packages
repository = .
type = variant
uid = Server
variants = Server-HighAvailability,Server-ResilientStorage

It should not create a new repository to hold that content because the main sync machinery is going to handle syncing it into the existing the repository that the user thinks of as "their repository"

Does this make sense?

@fao89
Copy link
Member Author

fao89 commented Sep 4, 2019

One special case we need to handle (and don't yet AIUI) is where the treeinfo file has a Variant that references the "main repository" with something like repository = . as in this example.

In that case the main sync machinery is already handling it and so the Variant needs to only preserve the data such as:

[variant-Server]
id = Server
name = Server
packages = Packages
repository = .
type = variant
uid = Server
variants = Server-HighAvailability,Server-ResilientStorage

It should not create a new repository to hold that content because the main sync machinery is going to handle syncing it into the existing the repository that the user thinks of as "their repository"

Does this make sense?

it does not create a new repo:
https://github.com/fabricio-aguiar/pulp_rpm/blob/5202-kickstart-sync/pulp_rpm/app/tasks/synchronizing.py#L80-L82

@fao89 fao89 force-pushed the 5202-kickstart-sync branch 2 times, most recently from 519e1e4 to 21bcc10 Compare September 4, 2019 15:39
bmbouter
bmbouter reviewed Sep 4, 2019
View reviewed changes
pulp_rpm/app/tasks/synchronizing.py Outdated
@@ -110,7 +143,7 @@ class RpmFirstStage(Stage):
that should exist in the new :class:`~pulpcore.plugin.models.RepositoryVersion`.
"""

def __init__(self, remote, deferred_download):
def __init__(self, remote, deferred_download, new_url=None, kickstart={}):
Copy link
Member

Choose a reason for hiding this comment

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

Strange bugs can come from a kwarg whose default is a mutable type because if the first time you call it you mutate it, the second time you call it it shows up mutated which is usually unexpected. Generally a None as the default is considered a better practice.

bmbouter
bmbouter approved these changes Sep 4, 2019
View reviewed changes
Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

This looks all good to me. Thank you @fabricio-aguiar !

@bmbouter bmbouter merged commit bf48a5f into pulp:master Sep 4, 2019
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Sep 4, 2019
@fao89
publishing kickstarts
57614db 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Sep 4, 2019
@fao89
publishing kickstarts
ee6a358 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Sep 5, 2019
@fao89
publishing kickstarts
44a0bcd 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Sep 5, 2019
@fao89
publishing kickstarts
f7fcead 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Sep 10, 2019
@fao89
publishing kickstarts
15923a3 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Sep 12, 2019
@fao89
publishing kickstarts
54388f7 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
Required PR: pulp#1440
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Sep 13, 2019
@fao89
publishing kickstarts
24c1d95 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
Required PR: pulp#1440
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Sep 16, 2019
@fao89
publishing kickstarts
8a32f22 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
Required PR: pulp#1440
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Sep 16, 2019
@fao89
publishing kickstarts
273c4eb 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
Required PR: pulp#1440
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Sep 17, 2019
@fao89
publishing kickstarts
f4e5d2f 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
Required PR: pulp#1440
fao89 added a commit to fao89/pulp_rpm that referenced this pull request Sep 17, 2019
@fao89
publishing kickstarts
8a8048e 
closes #5206
https://pulp.plan.io/issues/5206
Required PR: pulp#1418
Required PR: pulp#1427
Required PR: pulp#1440
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmbouter bmbouter bmbouter approved these changes

@nixocio nixocio nixocio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fao89 @bmbouter @nixocio