Skip to content

Support syncing Amazon Linux blobstore style repos (e.g. AL2, AL2022) #457

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.

Sign up for GitHub

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

Open
stewartsmith wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Support syncing Amazon Linux blobstore style repos (e.g. AL2, AL2022) #457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa700f6
Support syncing Amazon Linux blobstore style repositories
stewartsmith May 30, 2022
f9f5a07
Add Stewart Smith to contributors
stewartsmith May 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,4 @@ DNF-PLUGINS-CORE CONTRIBUTORS
Vladan Kudlac <vladankudlac@gmail.com>
Wieland Hoffmann <themineo@gmail.com>
Otto Urpelainen <oturpe@iki.fi>
Stewart Smith <trawets@amazon.com>
79 changes: 74 additions & 5 deletions plugins/reposync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@

import hawkey
import os
import tempfile
import shutil
import types
import urllib.parse

from dnfpluginscore import _, logger
from dnf.cli.option_parser import OptionParser
Expand Down Expand Up @@ -80,6 +82,21 @@ def set_argparser(parser):
help=_("Don't add the reponame to the download path."))
parser.add_argument('-p', '--download-path', default='./',
help=_('where to store downloaded repositories'))
parser.add_argument('-B', '--blobstore', default=False, action='store_true',
help=_('Enable compatibility with blobstore style repos '
'(e.g. Amazon Linux repositories)'))
parser.add_argument('-M', '--save-relative-mirrorlist', default=False,
action='store_true',
help=_('Save a modified copy of the repo mirror list '
'pointing to the repo data. pointing to the '
'repo downloaded. Useful with --blobstore '
Copy link
Copy Markdown
Member

@m-blaha m-blaha Jun 15, 2022

Choose a reason for hiding this comment

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

pointing to the repo data. pointing to the repo downloaded. - probably remnant of previous version of the sentence.

Copy link
Copy Markdown
Author

@stewartsmith stewartsmith Jun 15, 2022

Choose a reason for hiding this comment

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

Ahhh whoops, will fix.

'and needs --save-relative-mirrorlist-prefix '
'to produce a usable mirrorlist'))
parser.add_argument('--save-relative-mirrorlist-prefix',
help=_('Used with --save-releative-mirrorlist so the '
'resulting mirror list is usable. If hosting '
'this synced repo on https://example.com/ then '
'put that URL here.'))
parser.add_argument('--remote-time', default=False, action='store_true',
help=_('try to set local timestamps of local files by '
'the one on the server'))
Expand Down Expand Up @@ -169,12 +186,21 @@ def run(self):
os.path.basename(local_path), error))
os.unlink(local_path)
gpgcheck_ok = False
if self.opts.save_relative_mirrorlist:
self.save_relative_mirrorlist(repo)
if self.opts.delete:
self.delete_old_local_packages(repo, pkglist)
if not gpgcheck_ok:
raise dnf.exceptions.Error(_("GPG signature check failed."))

def repo_target(self, repo):
if self.opts.blobstore:
mirrors = repo._repo.getMirrors()
dest_path = urllib.parse.urlparse(mirrors[0]).path[1:]
Copy link
Copy Markdown
Member

@m-blaha m-blaha Jun 15, 2022

Choose a reason for hiding this comment

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

Repositories are not required to have mirrorlist. They can have metalink or baseurl set. In such case mirrors is an empty tuple and this line raises an IndexError exception.

Copy link
Copy Markdown
Author

@stewartsmith stewartsmith Jun 15, 2022

Choose a reason for hiding this comment

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

We don't (currently at least, and don't have immediate plans to) have a metalink pointer for any of our repos... although I could sync one somewhere, create a metalink and test it that way. I'll give it a go and see if I can suitably test, or at least more gracefully bail out until such a repo exists.

Good point on the baseurl though, as that should probably work here as well, even if not the common path for Amazon Linux repos.

dest_path = os.path.join(repo.id if not self.opts.norepopath else '',
dest_path, '')
return _pkgdir(self.opts.destdir or self.opts.download_path,
dest_path)
return _pkgdir(self.opts.destdir or self.opts.download_path,
repo.id if not self.opts.norepopath else '')

Expand All @@ -186,15 +212,31 @@ def metadata_target(self, repo):

def pkg_download_path(self, pkg):
repo_target = self.repo_target(pkg.repo)

pkg_location = pkg.location
pkg_download_path = os.path.realpath(
os.path.join(repo_target, pkg.location))
os.path.join(repo_target, pkg_location))

# If we haven't set the blobstore option (i.e. preserve full path)
# and we see references to a file outside our destination,
# then we should just store the package in the target location
Copy link
Copy Markdown
Member

@m-blaha m-blaha Jun 15, 2022

Choose a reason for hiding this comment

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

But this changes the behavior of the plugin in case --download-metadata switch is used. According to the documentation of the option: "Download all repository metadata. Downloaded copy is instantly usable as a repository, no need to run createrepo_c on it.".
Now (with changed package download location) this is not true any more and user is not even informed.

Copy link
Copy Markdown
Author

@stewartsmith stewartsmith Jun 15, 2022

Choose a reason for hiding this comment

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

Agree that in that case we should probably error out.

Logic should be something like:

  • if blobstore style repo
    • and --download-metadata but not --blobstore: error out as downloaded metadata will not be usable
    • if --blobstore and --download-metadata: download the metadata and things will work
  • if not blobstore style repo:
    • preserve existing behavior of --download-metadata or not.
      ?

if not self.opts.blobstore:
if not pkg_download_path.startswith(os.path.join(repo_target, '')):
pkg_location = os.path.basename(pkg_location)
pkg_download_path = os.path.realpath(
os.path.join(repo_target, pkg_location))

# join() ensures repo_target ends with a path separator (otherwise the
# check would pass if pkg_download_path was a "sibling" path component
# of repo_target that has the same prefix).
Copy link
Copy Markdown
Member

@m-blaha m-blaha Jun 15, 2022

Choose a reason for hiding this comment

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

This commend now lost it's meaning. It should be also moved (together with os.path.join(repo_target, '') move)

if not pkg_download_path.startswith(os.path.join(repo_target, '')):
raise dnf.exceptions.Error(
_("Download target '{}' is outside of download path '{}'.").format(
pkg_download_path, repo_target))
repo_target_check = repo_target
if self.opts.blobstore:
repo_target_check = _pkgdir(pkg.repo.id if not self.opts.norepopath else '', '')
Copy link
Copy Markdown
Member

@m-blaha m-blaha Jun 15, 2022

Choose a reason for hiding this comment

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

What's the reason for computing repo_target_check value here? Later in code there is no usage for it.
Also these two subsequent conditions if not self.opts.blobstore and if self.opts.blobstore look a bit strange.

else:
if not pkg_download_path.startswith(os.path.join(repo_target_check, '')):
raise dnf.exceptions.Error(
_("Download target '{}' is outside of download path '{}'.").format(
pkg_download_path, repo_target_check))
return pkg_download_path

def delete_old_local_packages(self, repo, pkglist):
Expand Down Expand Up @@ -226,6 +268,33 @@ def download_metadata(self, repo):
repo._repo.downloadMetadata(repo_target)
return True

def save_relative_mirrorlist(self, repo):
if repo.mirrorlist is None:
logger.error(_("repo doesn't have mirrorlist to rewrite"))
return False
target = _pkgdir(self.opts.destdir or self.opts.download_path,
repo.id if not self.opts.norepopath else '')
mirrorlist_path = urllib.parse.urlparse(repo.mirrorlist).path[1:]
mirrorlist_path = _pkgdir(target, mirrorlist_path)

if not mirrorlist_path.startswith(os.path.join(target, '')):
raise dnf.exceptions.Error(
_("Download target '{}' is outside of download path '{}'.").format(
mirrorlist_path, target))
os.makedirs(os.path.dirname(mirrorlist_path), exist_ok=True)

dirname, basename = os.path.split(mirrorlist_path)
temp = tempfile.NamedTemporaryFile(mode="w", prefix=basename,
dir=dirname, delete=False)

mirrors = repo._repo.getMirrors()
for m in mirrors:
temp.write(self.opts.save_relative_mirrorlist_prefix + urllib.parse.urlparse(m).path[1:])
temp.write("\n")

os.rename(temp.name, mirrorlist_path)
return True

def _get_latest(self, query):
"""
return union of these queries:
Expand Down
4 changes: 2 additions & 2 deletions tests/test_reposync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ def test_pkg_download_path(self):
self.assertEqual(pkgpath, '/become/legend/silver/foo-0-1.0-1.noarch.rpm')

pkg.location = "../pool/foo-0-1.0-1.noarch.rpm"
with self.assertRaises(dnf.exceptions.Error):
self.cmd.pkg_download_path(pkg)
self.assertEqual(self.cmd.pkg_download_path(pkg),
"/become/legend/silver/foo-0-1.0-1.noarch.rpm")

def test_metadata_target_default(self):
args = '-p /become/legend'.split()
Expand Down