From 4f6394b6b170f501666cf9b03a1fb1b1e3b2bd14 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: May 05 2025 19:07:17 +0000
Subject: [PATCH 1/11] test showing an issue with cross volume promotion links


---

diff --git a/tests/test_hub/test_promote_build.py b/tests/test_hub/test_promote_build.py
index 351a389..4ca6e7d 100644
--- a/tests/test_hub/test_promote_build.py
+++ b/tests/test_hub/test_promote_build.py
@@ -1,5 +1,6 @@
 import datetime
-import json
+import os.path
+import tempfile
 from unittest import mock
 import unittest
 
@@ -177,3 +178,97 @@ class TestPromoteBuild(unittest.TestCase):
             'version': 'bar',
             'release': 'tgtrel'
         }, strict=False)
+
+
+class TestPromoteBuildFiles(unittest.TestCase):
+    # these tests use a tempdir
+
+    def getUpdate(self, *args, **kwargs):
+        update = UP(*args, **kwargs)
+        update.execute = mock.MagicMock()
+        self.updates.append(update)
+        return update
+
+    def setUp(self):
+        # set up our dirs
+        self.tempdir = tempfile.mkdtemp()
+        self.topdir = self.tempdir + '/koji'
+        self.pathinfo = koji.PathInfo(self.topdir)
+        mock.patch('koji.pathinfo', new=self.pathinfo).start()
+        # separate dir for volume X
+        vol_x = self.tempdir + '/vol_X'
+        koji.ensuredir(vol_x)
+        voldir = self.pathinfo.volumedir('X')
+        koji.ensuredir(os.path.dirname(voldir))  # koji/vol
+        os.symlink(vol_x, voldir)
+
+        self.exports = kojihub.RootExports()
+        self.UpdateProcessor = mock.patch('kojihub.kojihub.UpdateProcessor',
+                                          side_effect=self.getUpdate).start()
+        self.updates = []
+        self.context = mock.patch('kojihub.kojihub.context').start()
+        self.context.session.assertLogin = mock.MagicMock()
+        self.user = {'id': 1, 'name': 'jdoe'}
+        self.get_user = mock.patch('kojihub.kojihub.get_user', return_value=self.user).start()
+        self.get_build = mock.patch('kojihub.kojihub.get_build').start()
+        self.assert_policy = mock.patch('kojihub.kojihub.assert_policy').start()
+        self.apply_volume_policy = mock.patch('kojihub.kojihub.apply_volume_policy',
+                                              return_value=None).start()
+        self.lookup_name = mock.patch('kojihub.kojihub.lookup_name',
+                                      return_value={'id': 1, 'name': 'DEFAULT'}).start()
+        self.list_tags = mock.patch('kojihub.kojihub.list_tags',
+                                    return_value=[{'id': 101}]).start()
+        self.set_tag_update = mock.patch('kojihub.kojihub.set_tag_update').start()
+        self._now = datetime.datetime.now()
+        self._datetime = mock.patch('kojihub.kojihub.datetime.datetime').start()
+        self.now = self._datetime.now = mock.MagicMock(return_value=self._now)
+
+        self.draft_build = {
+            'id': 1,
+            'name': 'foo',
+            'version': 'bar',
+            'release': 'tgtrel,draft_1',
+            'nvr': 'testnvr',
+            'state': 1,
+            'draft': True,
+            'volume_id': 99,
+            'volume_name': 'X',
+            'task_id': 222
+        }
+
+        self.new_build = {
+            # no check on the info
+            'id': 1,
+            'name': 'foo',
+            'version': 'bar',
+            'release': 'tgtrel',
+            'volume_name': 'X'
+        }
+
+    def tearDown(self):
+        mock.patch.stopall()
+
+    def test_promote_build_volume_link(self):
+        self.get_build.side_effect = [
+            self.draft_build,
+            None,
+            self.new_build
+        ]
+        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 c28dd81c71a96fc3125181c1f02539d2597cdaa6 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: May 05 2025 22:19:45 +0000
Subject: [PATCH 2/11] initial fix


---

diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py
index 24f8eda..b759003 100644
--- a/kojihub/kojihub.py
+++ b/kojihub/kojihub.py
@@ -6265,6 +6265,72 @@ def ensure_volume_symlink(binfo):
     os.symlink(relpath, basedir)
 
 
+def ensure_volume_backlink(new_binfo, old_binfo):
+    """Ensure we have a link for a build on given non-default volume
+
+    We point the symlink at the default volume location, because this path should
+    always be either valid symlink or the actual build dir.
+
+    Note: this is tricky!
+    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"
+    """
+
+    # 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('Missing volume dir: %s' % voldir)
+
+    # ensure we have the volume toplink
+    toplink = joinpath(voldir, 'toplink')
+    if os.path.islink(toplink):
+        if not os.path.exists(toplink):
+            raise koji.GenericError(f'Bad volume toplink: {toplink}')
+    elif os.path.exists(toplink):
+        # not a link
+        raise koji.GenericError(f'Not a symlink: {toplink}')
+    else:
+        # in the future, this should be part of volume setup, but for now
+        # we'll be nice and create it
+        target = koji.pathinfo.topdir
+        logger.warning('No toplink for volume. Creating {toplink} -> {target}')
+        os.symlink(target, 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')
+    base_binfo = new_binfo.copy()
+    base_binfo['volume_name'] = 'DEFAULT'
+    path2 = relpathinfo.build(base_binfo)  # toplink/packages/N/V/R
+
+    # check/make the symlink
+    relpath = joinpath(path1, path2)
+    if os.path.islink(olddir):
+        if os.readlink(olddir) == relpath:
+            # already correct
+            return
+        os.unlink(olddir)
+    elif os.path.exists(olddir):
+        raise koji.GenericError('Unexpected build content: %s' % olddir)
+    else:
+        # parent dir might not exist
+        koji.ensuredir(os.path.dirname(olddir))
+    os.symlink(relpath, olddir)
+
+
 def check_volume_policy(data, strict=False, default=None):
     """Check volume policy for the given data
 
@@ -10681,13 +10747,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
-    base_vol = lookup_name('volume', 'DEFAULT', strict=True)
-    base_binfo = new_binfo.copy()
-    base_binfo['volume_id'] = base_vol['id']
-    base_binfo['volume_name'] = base_vol['name']
-    basedir = koji.pathinfo.build(base_binfo)
-    relpath = os.path.relpath(basedir, os.path.dirname(oldpath))
-    os.symlink(relpath, oldpath)
+    ensure_volume_backlink(new_binfo, 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_promote_build.py b/tests/test_hub/test_promote_build.py
index 4ca6e7d..bc4e4e6 100644
--- a/tests/test_hub/test_promote_build.py
+++ b/tests/test_hub/test_promote_build.py
@@ -34,6 +34,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.lookup_name = mock.patch('kojihub.kojihub.lookup_name',
                                       return_value={'id': 1, 'name': 'DEFAULT'}).start()
         self.os_symlink = mock.patch('os.symlink').start()
@@ -94,10 +95,7 @@ class TestPromoteBuild(unittest.TestCase):
             '/mnt/koji/vol/X/packages/foo/bar/tgtrel,draft_1',
             '/mnt/koji/vol/X/packages/foo/bar/tgtrel'
         )
-        self.os_symlink.assert_called_once_with(
-            '../../../../../packages/foo/bar/tgtrel',
-            '/mnt/koji/vol/X/packages/foo/bar/tgtrel,draft_1'
-        )
+        self.os_symlink.assert_not_called()
 
     def test_promote_build_not_draft(self):
         self.get_build.return_value = {'draft': False, 'nvr': 'testnvr'}

From 4aad9579831b86160881d73e1e85f3db1feff9cd Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: May 05 2025 23:21:10 +0000
Subject: [PATCH 3/11] check/create volume toplink in add_volume


---

diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py
index b759003..c33d24e 100644
--- a/kojihub/kojihub.py
+++ b/kojihub/kojihub.py
@@ -6116,6 +6116,22 @@ def add_volume(name, strict=True):
     voldir = koji.pathinfo.volumedir(name)
     if not os.path.isdir(voldir):
         raise koji.GenericError('please create the volume directory first')
+
+    # volume directories should have a symlink to default volume, e.g. /mnt/koji
+    toplink = joinpath(voldir, 'toplink')
+    if os.path.islink(toplink):
+        if not os.path.exists(toplink):
+            raise koji.GenericError(f'Broken volume toplink: {toplink}')
+        if not os.path.samefile(toplink, koji.pathinfo.topdir):
+            raise koji.GenericError(f'Invalid volume toplink: {toplink}')
+    elif os.path.exists(toplink):
+        # not a link
+        raise koji.GenericError(f'Not a symlink: {toplink}')
+    else:
+        target = koji.pathinfo.topdir
+        logger.warning('No toplink for volume. Creating {toplink} -> {target}')
+        os.symlink(target, toplink)
+
     if strict:
         volinfo = lookup_name('volume', name, strict=False)
         if volinfo:

From d4767cbc2a61b01d718a94a5a327482400e8a895 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: May 05 2025 23:26:08 +0000
Subject: [PATCH 4/11] require volume toplink to exist


---

diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py
index c33d24e..9904db6 100644
--- a/kojihub/kojihub.py
+++ b/kojihub/kojihub.py
@@ -6301,22 +6301,10 @@ def ensure_volume_backlink(new_binfo, old_binfo):
         return
     voldir = koji.pathinfo.volumedir(volname)
     if not os.path.isdir(voldir):
-        raise koji.GenericError('Missing volume dir: %s' % voldir)
-
-    # ensure we have the volume toplink
+        raise koji.GenericError(f'Missing volume dir: {voldir}')
     toplink = joinpath(voldir, 'toplink')
-    if os.path.islink(toplink):
-        if not os.path.exists(toplink):
-            raise koji.GenericError(f'Bad volume toplink: {toplink}')
-    elif os.path.exists(toplink):
-        # not a link
-        raise koji.GenericError(f'Not a symlink: {toplink}')
-    else:
-        # in the future, this should be part of volume setup, but for now
-        # we'll be nice and create it
-        target = koji.pathinfo.topdir
-        logger.warning('No toplink for volume. Creating {toplink} -> {target}')
-        os.symlink(target, 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)
diff --git a/tests/test_hub/test_promote_build.py b/tests/test_hub/test_promote_build.py
index bc4e4e6..104e20b 100644
--- a/tests/test_hub/test_promote_build.py
+++ b/tests/test_hub/test_promote_build.py
@@ -195,10 +195,12 @@ class TestPromoteBuildFiles(unittest.TestCase):
         mock.patch('koji.pathinfo', new=self.pathinfo).start()
         # separate dir for volume X
         vol_x = self.tempdir + '/vol_X'
+        toplink = self.tempdir + '/vol_X/toplink'
         koji.ensuredir(vol_x)
         voldir = self.pathinfo.volumedir('X')
         koji.ensuredir(os.path.dirname(voldir))  # koji/vol
         os.symlink(vol_x, voldir)
+        os.symlink(self.topdir, toplink)
 
         self.exports = kojihub.RootExports()
         self.UpdateProcessor = mock.patch('kojihub.kojihub.UpdateProcessor',

From 5b2a63f469596ace4dc1486796f0282a5babad90 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: May 06 2025 19:33:15 +0000
Subject: [PATCH 5/11] don't require new_binfo


---

diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py
index 9904db6..b54483b 100644
--- a/kojihub/kojihub.py
+++ b/kojihub/kojihub.py
@@ -6281,7 +6281,7 @@ def ensure_volume_symlink(binfo):
     os.symlink(relpath, basedir)
 
 
-def ensure_volume_backlink(new_binfo, old_binfo):
+def ensure_volume_backlink(old_binfo, new_binfo=None):
     """Ensure we have a link for a build on given non-default volume
 
     We point the symlink at the default volume location, because this path should
@@ -6316,7 +6316,11 @@ def ensure_volume_backlink(new_binfo, old_binfo):
     path1 = os.path.relpath(voldir, os.path.dirname(olddir))  # should be ../../..
     assert path1 == '../../..'  # XXX
     relpathinfo = koji.PathInfo(topdir='toplink')
-    base_binfo = new_binfo.copy()
+    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()
     base_binfo['volume_name'] = 'DEFAULT'
     path2 = relpathinfo.build(base_binfo)  # toplink/packages/N/V/R
 
@@ -10751,7 +10755,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(new_binfo, binfo)
+    ensure_volume_backlink(binfo, new_binfo)
 
     # apply volume policy in case it's changed by release update.
     apply_volume_policy(new_binfo, strict=False)

From a6934df5310fc5a236db52d749fab17fa6e4b81c Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: May 06 2025 20:12:40 +0000
Subject: [PATCH 6/11] more unit tests


---

diff --git a/tests/test_hub/test_ensure_volume_backlink.py b/tests/test_hub/test_ensure_volume_backlink.py
new file mode 100644
index 0000000..2d1f9e3
--- /dev/null
+++ b/tests/test_hub/test_ensure_volume_backlink.py
@@ -0,0 +1,150 @@
+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)

From 6b4d5723b7bb378355868137b1612316e0904184 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: May 06 2025 20:42:24 +0000
Subject: [PATCH 7/11] update tests for add_volume


---

diff --git a/tests/test_hub/test_add_volume.py b/tests/test_hub/test_add_volume.py
index ce8e09e..0835ef7 100644
--- a/tests/test_hub/test_add_volume.py
+++ b/tests/test_hub/test_add_volume.py
@@ -1,3 +1,6 @@
+import os
+import shutil
+import tempfile
 import unittest
 
 from unittest import mock
@@ -9,10 +12,26 @@ import kojihub
 class TestAddVolume(unittest.TestCase):
 
     def setUp(self):
+        # set up topdir
+        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 test volume
+        vol = self.tempdir + '/vol_test'
+        self.volmount = vol
+        toplink = vol + '/toplink'
+        koji.ensuredir(vol)
+        voldir = self.pathinfo.volumedir('test-volume')
+        koji.ensuredir(os.path.dirname(voldir))  # koji/vol
+        os.symlink(vol, voldir)
+        os.symlink(self.topdir, toplink)
+
         self.verify_name_internal = mock.patch('kojihub.kojihub.verify_name_internal').start()
         self.lookup_name = mock.patch('kojihub.kojihub.lookup_name').start()
-        self.isdir = mock.patch('os.path.isdir').start()
-        self.pathinfo_volumedir = mock.patch('koji.pathinfo.volumedir').start()
+        # self.isdir = mock.patch('os.path.isdir').start()
+        # self.pathinfo_volumedir = mock.patch('koji.pathinfo.volumedir').start()
         self.context = mock.patch('kojihub.kojihub.context').start()
         # It seems MagicMock will not automatically handle attributes that
         # start with "assert"
@@ -21,6 +40,7 @@ class TestAddVolume(unittest.TestCase):
 
     def tearDown(self):
         mock.patch.stopall()
+        shutil.rmtree(self.tempdir)
 
     def test_add_volume_wrong_format(self):
         volume_name = 'volume-name+'
@@ -36,34 +56,88 @@ class TestAddVolume(unittest.TestCase):
             kojihub.add_volume(volume_name)
 
     def test_non_exist_directory(self):
-        volume_name = 'test-volume'
-        self.isdir.return_value = False
-        self.pathinfo_volumedir.return_value = 'path/to/volume'
+        volume_name = 'no-such-volume'
+
         with self.assertRaises(koji.GenericError) as cm:
             kojihub.add_volume(volume_name)
+
         self.assertEqual("please create the volume directory first", str(cm.exception))
         self.verify_name_internal.assert_called_once_with(volume_name)
         self.lookup_name.assert_not_called()
 
     def test_valid(self):
         volume_name = 'test-volume'
-        volume_dict = {'id': 0, 'name': volume_name}
-        self.isdir.return_value = True
-        self.pathinfo_volumedir.return_value = 'path/to/volume'
+        volume_dict = {'id': 1, 'name': volume_name}
         self.lookup_name.return_value = volume_dict
+
         rv = kojihub.add_volume(volume_name, strict=False)
+
         self.assertEqual(rv, volume_dict)
         self.verify_name_internal.assert_called_once_with(volume_name)
         self.lookup_name.assert_called_once_with('volume', volume_name, strict=False, create=True)
 
     def test_volume_exists(self):
         volume_name = 'test-volume'
-        volume_dict = {'id': 0, 'name': volume_name}
-        self.isdir.return_value = True
-        self.pathinfo_volumedir.return_value = 'path/to/volume'
+        volume_dict = {'id': 1, 'name': volume_name}
         self.lookup_name.return_value = volume_dict
+
         with self.assertRaises(koji.GenericError) as cm:
             kojihub.add_volume(volume_name, strict=True)
+
         self.assertEqual(f'volume {volume_name} already exists', str(cm.exception))
         self.verify_name_internal.assert_called_once_with(volume_name)
         self.lookup_name.assert_called_once_with('volume', volume_name, strict=False)
+
+    def test_volume_broken_toplink(self):
+        volume_name = 'test-volume'
+        volume_dict = {'id': 1, 'name': volume_name}
+        self.lookup_name.side_effect = [None, volume_dict]
+        toplink = self.volmount + '/toplink'
+        os.unlink(toplink)
+        os.symlink('BROKEN-LINK', toplink)
+
+        with self.assertRaises(koji.GenericError) as cm:
+            kojihub.add_volume(volume_name, strict=True)
+
+        assert str(cm.exception).startswith('Broken volume toplink')
+
+    def test_volume_invalid_toplink(self):
+        volume_name = 'test-volume'
+        volume_dict = {'id': 1, 'name': volume_name}
+        self.lookup_name.side_effect = [None, volume_dict]
+        toplink = self.volmount + '/toplink'
+        os.unlink(toplink)
+        os.symlink('..', toplink)
+
+        with self.assertRaises(koji.GenericError) as cm:
+            kojihub.add_volume(volume_name, strict=True)
+
+        assert str(cm.exception).startswith('Invalid volume toplink')
+
+    def test_volume_nonlink_toplink(self):
+        volume_name = 'test-volume'
+        volume_dict = {'id': 1, 'name': volume_name}
+        self.lookup_name.side_effect = [None, volume_dict]
+        toplink = self.volmount + '/toplink'
+        os.unlink(toplink)
+        os.mkdir(toplink)
+
+        with self.assertRaises(koji.GenericError) as cm:
+            kojihub.add_volume(volume_name, strict=True)
+
+        assert str(cm.exception).startswith('Not a symlink')
+
+    def test_volume_create_toplink(self):
+        # test that the toplink is automatically created if missing
+        volume_name = 'test-volume'
+        volume_dict = {'id': 1, 'name': volume_name}
+        self.lookup_name.side_effect = [None, volume_dict]
+        toplink = self.volmount + '/toplink'
+        os.unlink(toplink)
+
+        kojihub.add_volume(volume_name, strict=True)
+
+        self.assertEqual(os.readlink(toplink), self.pathinfo.topdir)
+
+
+# the end

From 0e3324eeb535d0bc66ca2b884f95f97d152f3188 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: May 06 2025 20:55:16 +0000
Subject: [PATCH 8/11] update volume docs


---

diff --git a/docs/source/volumes.rst b/docs/source/volumes.rst
index 096f29a..be5ae17 100644
--- a/docs/source/volumes.rst
+++ b/docs/source/volumes.rst
@@ -73,9 +73,17 @@ for builds on other volumes.
 Adding a new volume
 -------------------
 
-The new volume directory should initially contain a packages/
-subdirectory, and the permissions should be the same as the default
-packages directory.
+The new volume directory should initially contain:
+
+* a ``packages`` subdirectory
+* a ``toplink`` symlink to the primary volume (i.e. /mnt/koji)
+
+The permissions for the ``packages`` directory should be the same as the\
+default packages directory.
+
+The ``toplink`` symlink should normally be an absolute symlink.
+If you do not create it, then Koji will do so for you when you run the
+add-volume command.
 
 Assuming you do use a mount for a vol/NAME directory, you will want to
 ensure that the same mounts are created on all systems that interface with

From 51045839905e184a733bc9675a0d5fa060296cec Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: May 06 2025 21:06:37 +0000
Subject: [PATCH 9/11] unrelated docs typo


---

diff --git a/.bandit b/.bandit
new file mode 100644
index 0000000..09e6e3c
--- /dev/null
+++ b/.bandit
@@ -0,0 +1,2 @@
+[markupsafe_xss]
+allowed_calls = _MarkTrustedValue
diff --git a/docs/source/using_the_koji_build_system.rst b/docs/source/using_the_koji_build_system.rst
index cc237f1..7f9c00c 100644
--- a/docs/source/using_the_koji_build_system.rst
+++ b/docs/source/using_the_koji_build_system.rst
@@ -440,7 +440,7 @@ environment follows:
 
 * ``mock.bootstrap_image_ready`` - For the specification that we are ready for bootstrapping.
   If we want to specify, that we are ready for bootstrapping, we need to know that these
-  two packages `python3-dnf` and `python3-dnf-plugins-coreare installed in the container.
+  two packages ``python3-dnf`` and ``python3-dnf-plugins-core`` are installed in the container.
 
 * ``mock.module_setup_commands`` - commands for configuring the modules active
   in a buildroot. Available in `mock 2.4

From 335052f6fd7e4fb93da5545aaf03844ac45584f9 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: May 06 2025 21:29:08 +0000
Subject: [PATCH 10/11] typo


---

diff --git a/.bandit b/.bandit
deleted file mode 100644
index 09e6e3c..0000000
--- a/.bandit
+++ /dev/null
@@ -1,2 +0,0 @@
-[markupsafe_xss]
-allowed_calls = _MarkTrustedValue
diff --git a/docs/source/volumes.rst b/docs/source/volumes.rst
index be5ae17..1645c38 100644
--- a/docs/source/volumes.rst
+++ b/docs/source/volumes.rst
@@ -78,7 +78,7 @@ The new volume directory should initially contain:
 * a ``packages`` subdirectory
 * a ``toplink`` symlink to the primary volume (i.e. /mnt/koji)
 
-The permissions for the ``packages`` directory should be the same as the\
+The permissions for the ``packages`` directory should be the same as the
 default packages directory.
 
 The ``toplink`` symlink should normally be an absolute symlink.

From c4a7bd28c873e0d48c76b150d684fc0d2ecbe462 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: May 06 2025 21:37:00 +0000
Subject: [PATCH 11/11] clean up test dir


---

diff --git a/tests/test_hub/test_promote_build.py b/tests/test_hub/test_promote_build.py
index 104e20b..26d69d0 100644
--- a/tests/test_hub/test_promote_build.py
+++ b/tests/test_hub/test_promote_build.py
@@ -1,4 +1,5 @@
 import datetime
+import shutil
 import os.path
 import tempfile
 from unittest import mock
@@ -247,6 +248,7 @@ class TestPromoteBuildFiles(unittest.TestCase):
 
     def tearDown(self):
         mock.patch.stopall()
+        shutil.rmtree(self.tempdir)
 
     def test_promote_build_volume_link(self):
         self.get_build.side_effect = [