From 01bd5c88f7aad0e54bf81f1f1d4dd534439ed4ff Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Sep 05 2025 18:00:02 +0000 Subject: [PATCH 1/3] unit test that shows the problem --- diff --git a/tests/test_hub/test_promote_build.py b/tests/test_hub/test_promote_build.py index 26d69d0..eb2f8a9 100644 --- a/tests/test_hub/test_promote_build.py +++ b/tests/test_hub/test_promote_build.py @@ -272,5 +272,33 @@ class TestPromoteBuildFiles(unittest.TestCase): with open(orig_bdir + '/sentinel.txt', 'rt') as fp: assert fp.read() == sentinel + def test_promote_build_default_volume_link(self): + # promote a draft build on the default volume + self.draft_build['volume_id'] = 0 + self.draft_build['volume_name'] = 'DEFAULT' + self.new_build['volume_name'] = 'DEFAULT' + self.get_build.side_effect = [ + self.draft_build, # the build to promote + None, # checking to see if promoted build already exists + self.new_build # updated build info after promotion + ] + + # add test content to build dir + orig_bdir = self.pathinfo.build(self.draft_build) + koji.ensuredir(orig_bdir) + sentinel = 'HELLO 873\n' + with open(orig_bdir + '/sentinel.txt', 'wt') as fp: + fp.write(sentinel) + + # promote + ret = self.exports.promoteBuild('a-draft-build') + + self.assertEqual(ret, self.new_build) + # orig_bdir should be a symlink + assert os.path.islink(orig_bdir) + # should be accessible via original path + with open(orig_bdir + '/sentinel.txt', 'rt') as fp: + assert fp.read() == sentinel + # the end From 55238524b86e841ba4973642e2ccc557a0223c78 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Sep 05 2025 18:00:02 +0000 Subject: [PATCH 2/3] fix --- diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index c848bc9..1a4ccb9 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -6294,13 +6294,14 @@ def ensure_volume_symlink(binfo): os.symlink(relpath, basedir) -def ensure_volume_backlink(old_binfo, new_binfo=None): - """Ensure we have a link for a build on given non-default volume +def ensure_draft_backlink(old_binfo, new_binfo=None): + """Ensure we have a link for a promoted build We point the symlink at the default volume location, because this path should - always be either valid symlink or the actual build dir. + always be either a valid symlink or the actual build dir. This way, the link + will continue to work if the volume later changes. - Note: this is tricky! + Note: this is tricky in the non-default volume case We primarily use relative symlinks under /mnt/koji, however this can break when crossing volumes. In general, /mnt/koji/vol/foo/.. != /mnt/koji/vol However, relpath() assumes this is the case because it is "a path computation" @@ -6308,37 +6309,41 @@ def ensure_volume_backlink(old_binfo, new_binfo=None): # basic checks volname = old_binfo['volume_name'] - if volname == 'DEFAULT': - # nothing to do - # default volume symlinks are handled in ensure_volume_symlink() - return - voldir = koji.pathinfo.volumedir(volname) - if not os.path.isdir(voldir): - raise koji.GenericError(f'Missing volume dir: {voldir}') - toplink = joinpath(voldir, 'toplink') - if not os.path.exists(toplink): - raise koji.GenericError(f'Missing volume toplink: {toplink}') + if volname != 'DEFAULT': + voldir = koji.pathinfo.volumedir(volname) + if not os.path.isdir(voldir): + raise koji.GenericError(f'Missing volume dir: {voldir}') + toplink = joinpath(voldir, 'toplink') + if not os.path.exists(toplink): + raise koji.GenericError(f'Missing volume toplink: {toplink}') # get the old build path (where we will place the symlink) olddir = koji.pathinfo.build(old_binfo) - # construct the relative path in parts - # - relpath to voldir - # - voldir/toplink is a symlink to topdir - # - relpath from topdir to olddir - path1 = os.path.relpath(voldir, os.path.dirname(olddir)) # should be ../../.. - assert path1 == '../../..' # XXX - relpathinfo = koji.PathInfo(topdir='toplink') + # construct the relative path if new_binfo is not None: base_binfo = new_binfo.copy() else: # call can pass just old_binfo if NVR is not changing base_binfo = old_binfo.copy() + # symlink target is always the default volume base_binfo['volume_name'] = 'DEFAULT' - path2 = relpathinfo.build(base_binfo) # toplink/packages/N/V/R + if volname == 'DEFAULT': + # simpler case, both paths on default + dest = koji.pathinfo.build(base_binfo) + relpath = os.path.relpath(dest, os.path.dirname(olddir)) + else: + # for the cross volume case, we construct the link in parts: + # - relpath to voldir + # - voldir/toplink is a symlink to topdir + # - relpath from topdir to olddir + path1 = os.path.relpath(voldir, os.path.dirname(olddir)) # should be ../../.. + assert path1 == '../../..' # XXX + relpathinfo = koji.PathInfo(topdir='toplink') + path2 = relpathinfo.build(base_binfo) # toplink/packages/N/V/R + relpath = joinpath(path1, path2) # check/make the symlink - relpath = joinpath(path1, path2) if os.path.islink(olddir): if os.readlink(olddir) == relpath: # already correct @@ -10825,7 +10830,7 @@ def _promote_build(build, force=False): # provide a symlink at original draft location # we point to the default volume in case the build moves in the future - ensure_volume_backlink(binfo, new_binfo) + ensure_draft_backlink(binfo, new_binfo) # apply volume policy in case it's changed by release update. apply_volume_policy(new_binfo, strict=False) diff --git a/tests/test_hub/test_draft_backlink.py b/tests/test_hub/test_draft_backlink.py new file mode 100644 index 0000000..fb43d54 --- /dev/null +++ b/tests/test_hub/test_draft_backlink.py @@ -0,0 +1,160 @@ +from unittest import mock +import os +import os.path +import shutil +import tempfile +import unittest +import koji +import kojihub + +import pytest + +class TestEnsureDraftBacklink(unittest.TestCase): + + def setUp(self): + self.tempdir = tempfile.mkdtemp() + self.topdir = self.tempdir + '/koji' + self.pathinfo = koji.PathInfo(self.topdir) + mock.patch('koji.pathinfo', new=self.pathinfo).start() + + # set up other volume + vol = self.tempdir + '/vol_other' + self.volmount = vol + toplink = vol + '/toplink' + koji.ensuredir(vol) + voldir = self.pathinfo.volumedir('OTHER') + koji.ensuredir(os.path.dirname(voldir)) # koji/vol + os.symlink(vol, voldir) + os.symlink(self.topdir, toplink) + + # mock.patch('kojihub.kojihub.lookup_name', new=self.my_lookup_name).start() + self.buildinfo = { + 'id': 137, + 'task_id': 'TASK_ID', + 'name': 'some-image', + 'version': '1.2.3.4', + 'release': '3', + 'epoch': None, + 'source': None, + 'state': koji.BUILD_STATES['BUILDING'], + # 'volume_id': 1, + 'volume_name': 'OTHER', + } + + def tearDown(self): + mock.patch.stopall() + shutil.rmtree(self.tempdir) + + def my_lookup_name(self, table, info, **kw): + if table != 'volume': + raise Exception("Cannot fake call") + return { + 'id': 'VOLUMEID:%s' % info, + 'name': '%s' % info, + } + + def test_draft_symlink_create(self): + # verify that backlink is created correctly + basedir = self.pathinfo.build(self.buildinfo) # OTHER volume + + kojihub.ensure_draft_backlink(self.buildinfo) + + files = list(find_files(self.volmount)) + expected = [ + 'packages', + 'toplink', + 'packages/some-image', + 'packages/some-image/1.2.3.4', + 'packages/some-image/1.2.3.4/3', + ] + self.assertEqual(files, expected) + relpath = ('../../../toplink/packages/' + '%(name)s/%(version)s/%(release)s' % self.buildinfo) + self.assertEqual(os.readlink(basedir), relpath) + + def test_draft_symlink_exists(self): + # if an incorrect link is present, it should be replaced + basedir = self.pathinfo.build(self.buildinfo) # OTHER volume + oldpath = 'some/other/link' + os.makedirs(os.path.dirname(basedir)) + os.symlink(oldpath, basedir) + + kojihub.ensure_draft_backlink(self.buildinfo) + + relpath = ('../../../toplink/packages/' + '%(name)s/%(version)s/%(release)s' % self.buildinfo) + self.assertEqual(os.readlink(basedir), relpath) + + def test_draft_symlink_exists_same(self): + # if link is already correct, it should be left alone + basedir = self.pathinfo.build(self.buildinfo) # OTHER volume + relpath = ('../../../toplink/packages/' + '%(name)s/%(version)s/%(release)s' % self.buildinfo) + os.makedirs(os.path.dirname(basedir)) + os.symlink(relpath, basedir) + + with mock.patch('os.unlink') as unlink: + kojihub.ensure_draft_backlink(self.buildinfo) + unlink.assert_not_called() + + files = list(find_files(self.volmount)) + expected = [ + 'packages', + 'toplink', + 'packages/some-image', + 'packages/some-image/1.2.3.4', + 'packages/some-image/1.2.3.4/3', + ] + self.assertEqual(files, expected) + + def test_draft_symlink_exists_error(self): + # if the path exists and is not a link, we should error + basedir = self.pathinfo.build(self.buildinfo) # OTHER volume + os.makedirs(basedir) + files1 = list(find_files(self.tempdir)) + + with self.assertRaises(koji.GenericError): + kojihub.ensure_draft_backlink(self.buildinfo) + + files2 = list(find_files(self.tempdir)) + self.assertEqual(files1, files2) + + def test_draft_symlink_exists_error(self): + # if the volume dir is bad, we should error + basedir = self.pathinfo.build(self.buildinfo) # OTHER volume + + os.unlink(self.volmount + '/toplink') + with self.assertRaises(koji.GenericError): + kojihub.ensure_draft_backlink(self.buildinfo) + + os.rmdir(self.volmount) + with self.assertRaises(koji.GenericError): + kojihub.ensure_draft_backlink(self.buildinfo) + + def test_draft_symlink_default(self): + # the call should handle the default volume case + binfo = self.buildinfo.copy() + binfo['volume_name'] = 'DEFAULT' + + kojihub.ensure_draft_backlink(binfo) + + files = list(find_files(self.topdir)) + expected = [ + 'packages', + 'vol', + 'packages/some-image', + 'packages/some-image/1.2.3.4', + 'packages/some-image/1.2.3.4/3', + 'vol/OTHER', + ] + self.assertEqual(files, expected) + + +def find_files(dirpath): + '''Find all files under dir, report relative paths''' + for path, dirs, files in os.walk(dirpath): + for fn in sorted(dirs + files): + yield os.path.relpath(os.path.join(path, fn), dirpath) + + +# the end diff --git a/tests/test_hub/test_ensure_volume_backlink.py b/tests/test_hub/test_ensure_volume_backlink.py deleted file mode 100644 index 2d1f9e3..0000000 --- a/tests/test_hub/test_ensure_volume_backlink.py +++ /dev/null @@ -1,150 +0,0 @@ -from unittest import mock -import os -import os.path -import shutil -import tempfile -import unittest -import koji -import kojihub - -import pytest - -class TestEnsureVolumeBacklink(unittest.TestCase): - - def setUp(self): - self.tempdir = tempfile.mkdtemp() - self.topdir = self.tempdir + '/koji' - self.pathinfo = koji.PathInfo(self.topdir) - mock.patch('koji.pathinfo', new=self.pathinfo).start() - - # set up other volume - vol = self.tempdir + '/vol_other' - self.volmount = vol - toplink = vol + '/toplink' - koji.ensuredir(vol) - voldir = self.pathinfo.volumedir('OTHER') - koji.ensuredir(os.path.dirname(voldir)) # koji/vol - os.symlink(vol, voldir) - os.symlink(self.topdir, toplink) - - # mock.patch('kojihub.kojihub.lookup_name', new=self.my_lookup_name).start() - self.buildinfo = { - 'id': 137, - 'task_id': 'TASK_ID', - 'name': 'some-image', - 'version': '1.2.3.4', - 'release': '3', - 'epoch': None, - 'source': None, - 'state': koji.BUILD_STATES['BUILDING'], - # 'volume_id': 1, - 'volume_name': 'OTHER', - } - - def tearDown(self): - mock.patch.stopall() - shutil.rmtree(self.tempdir) - - def my_lookup_name(self, table, info, **kw): - if table != 'volume': - raise Exception("Cannot fake call") - return { - 'id': 'VOLUMEID:%s' % info, - 'name': '%s' % info, - } - - def test_volume_symlink_no_action(self): - # the call should do nothing if the volume is DEFAULT - binfo = self.buildinfo.copy() - binfo['volume_name'] = 'DEFAULT' - files1 = list(find_files(self.topdir)) - - kojihub.ensure_volume_backlink(binfo) - - files2 = list(find_files(self.topdir)) - self.assertEqual(files1, files2) - - def test_volume_symlink_create(self): - # verify that backlink is created correctly - basedir = self.pathinfo.build(self.buildinfo) # OTHER volume - - kojihub.ensure_volume_backlink(self.buildinfo) - - files = list(find_files(self.volmount)) - expected = [ - 'packages', - 'toplink', - 'packages/some-image', - 'packages/some-image/1.2.3.4', - 'packages/some-image/1.2.3.4/3', - ] - self.assertEqual(files, expected) - relpath = ('../../../toplink/packages/' - '%(name)s/%(version)s/%(release)s' % self.buildinfo) - self.assertEqual(os.readlink(basedir), relpath) - - def test_volume_symlink_exists(self): - # if an incorrect link is present, it should be replaced - basedir = self.pathinfo.build(self.buildinfo) # OTHER volume - oldpath = 'some/other/link' - os.makedirs(os.path.dirname(basedir)) - os.symlink(oldpath, basedir) - - kojihub.ensure_volume_backlink(self.buildinfo) - - relpath = ('../../../toplink/packages/' - '%(name)s/%(version)s/%(release)s' % self.buildinfo) - self.assertEqual(os.readlink(basedir), relpath) - - def test_volume_symlink_exists_same(self): - # if link is already correct, it should be left alone - basedir = self.pathinfo.build(self.buildinfo) # OTHER volume - relpath = ('../../../toplink/packages/' - '%(name)s/%(version)s/%(release)s' % self.buildinfo) - os.makedirs(os.path.dirname(basedir)) - os.symlink(relpath, basedir) - - with mock.patch('os.unlink') as unlink: - kojihub.ensure_volume_backlink(self.buildinfo) - unlink.assert_not_called() - - files = list(find_files(self.volmount)) - expected = [ - 'packages', - 'toplink', - 'packages/some-image', - 'packages/some-image/1.2.3.4', - 'packages/some-image/1.2.3.4/3', - ] - self.assertEqual(files, expected) - - def test_volume_symlink_exists_error(self): - # if the path exists and is not a link, we should error - basedir = self.pathinfo.build(self.buildinfo) # OTHER volume - os.makedirs(basedir) - files1 = list(find_files(self.tempdir)) - - with self.assertRaises(koji.GenericError): - kojihub.ensure_volume_backlink(self.buildinfo) - - files2 = list(find_files(self.tempdir)) - self.assertEqual(files1, files2) - - def test_volume_symlink_exists_error(self): - # if the volume dir is bad, we should error - basedir = self.pathinfo.build(self.buildinfo) # OTHER volume - - os.unlink(self.volmount + '/toplink') - with self.assertRaises(koji.GenericError): - kojihub.ensure_volume_backlink(self.buildinfo) - - os.rmdir(self.volmount) - with self.assertRaises(koji.GenericError): - kojihub.ensure_volume_backlink(self.buildinfo) - - -def find_files(dirpath): - '''Find all files under dir, report relative paths''' - for path, dirs, files in os.walk(dirpath): - for fn in sorted(dirs + files): - yield os.path.relpath(os.path.join(path, fn), dirpath) diff --git a/tests/test_hub/test_promote_build.py b/tests/test_hub/test_promote_build.py index eb2f8a9..2405de8 100644 --- a/tests/test_hub/test_promote_build.py +++ b/tests/test_hub/test_promote_build.py @@ -35,7 +35,7 @@ class TestPromoteBuild(unittest.TestCase): return_value=None).start() self.safer_move = mock.patch('kojihub.kojihub.safer_move').start() self.ensure_volume_symlink = mock.patch('kojihub.kojihub.ensure_volume_symlink').start() - self.ensure_volume_backlink = mock.patch('kojihub.kojihub.ensure_volume_backlink').start() + self.ensure_draft_backlink = mock.patch('kojihub.kojihub.ensure_draft_backlink').start() self.lookup_name = mock.patch('kojihub.kojihub.lookup_name', return_value={'id': 1, 'name': 'DEFAULT'}).start() self.os_symlink = mock.patch('os.symlink').start() From aa4db84aa102d742e2bcac972d490981a90ed089 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Sep 05 2025 18:00:02 +0000 Subject: [PATCH 3/3] flake8 --- diff --git a/tests/test_hub/test_draft_backlink.py b/tests/test_hub/test_draft_backlink.py index fb43d54..36eb9d2 100644 --- a/tests/test_hub/test_draft_backlink.py +++ b/tests/test_hub/test_draft_backlink.py @@ -7,7 +7,6 @@ import unittest import koji import kojihub -import pytest class TestEnsureDraftBacklink(unittest.TestCase): @@ -29,17 +28,17 @@ class TestEnsureDraftBacklink(unittest.TestCase): # mock.patch('kojihub.kojihub.lookup_name', new=self.my_lookup_name).start() self.buildinfo = { - 'id': 137, - 'task_id': 'TASK_ID', - 'name': 'some-image', - 'version': '1.2.3.4', - 'release': '3', - 'epoch': None, - 'source': None, - 'state': koji.BUILD_STATES['BUILDING'], - # 'volume_id': 1, - 'volume_name': 'OTHER', - } + 'id': 137, + 'task_id': 'TASK_ID', + 'name': 'some-image', + 'version': '1.2.3.4', + 'release': '3', + 'epoch': None, + 'source': None, + 'state': koji.BUILD_STATES['BUILDING'], + # 'volume_id': 1, + 'volume_name': 'OTHER', + } def tearDown(self): mock.patch.stopall() @@ -49,9 +48,9 @@ class TestEnsureDraftBacklink(unittest.TestCase): if table != 'volume': raise Exception("Cannot fake call") return { - 'id': 'VOLUMEID:%s' % info, - 'name': '%s' % info, - } + 'id': 'VOLUMEID:%s' % info, + 'name': '%s' % info, + } def test_draft_symlink_create(self): # verify that backlink is created correctly @@ -61,12 +60,12 @@ class TestEnsureDraftBacklink(unittest.TestCase): files = list(find_files(self.volmount)) expected = [ - 'packages', - 'toplink', - 'packages/some-image', - 'packages/some-image/1.2.3.4', - 'packages/some-image/1.2.3.4/3', - ] + 'packages', + 'toplink', + 'packages/some-image', + 'packages/some-image/1.2.3.4', + 'packages/some-image/1.2.3.4/3', + ] self.assertEqual(files, expected) relpath = ('../../../toplink/packages/' '%(name)s/%(version)s/%(release)s' % self.buildinfo) @@ -99,12 +98,12 @@ class TestEnsureDraftBacklink(unittest.TestCase): files = list(find_files(self.volmount)) expected = [ - 'packages', - 'toplink', - 'packages/some-image', - 'packages/some-image/1.2.3.4', - 'packages/some-image/1.2.3.4/3', - ] + 'packages', + 'toplink', + 'packages/some-image', + 'packages/some-image/1.2.3.4', + 'packages/some-image/1.2.3.4/3', + ] self.assertEqual(files, expected) def test_draft_symlink_exists_error(self): @@ -119,10 +118,8 @@ class TestEnsureDraftBacklink(unittest.TestCase): files2 = list(find_files(self.tempdir)) self.assertEqual(files1, files2) - def test_draft_symlink_exists_error(self): + def test_draft_symlink_exists_error2(self): # if the volume dir is bad, we should error - basedir = self.pathinfo.build(self.buildinfo) # OTHER volume - os.unlink(self.volmount + '/toplink') with self.assertRaises(koji.GenericError): kojihub.ensure_draft_backlink(self.buildinfo) @@ -140,13 +137,13 @@ class TestEnsureDraftBacklink(unittest.TestCase): files = list(find_files(self.topdir)) expected = [ - 'packages', - 'vol', - 'packages/some-image', - 'packages/some-image/1.2.3.4', - 'packages/some-image/1.2.3.4/3', - 'vol/OTHER', - ] + 'packages', + 'vol', + 'packages/some-image', + 'packages/some-image/1.2.3.4', + 'packages/some-image/1.2.3.4/3', + 'vol/OTHER', + ] self.assertEqual(files, expected)