PR#336: Store composes in directories in `odcs-$COMPOSE_ID` format. - odcs

odcs

#336 Store composes in directories in `odcs-$COMPOSE_ID` format.

Merged 4 years ago by lsedlar. Opened 4 years ago by jkaluza.

jkaluza/odcs remove-latest-dir into master

Store composes in directories in `odcs-$COMPOSE_ID` format.

Jan Kaluza • 4 years ago

2dcbdb9

server/odcs/server/backend.py

file modified

+1 -21

		`@@ -704,7 +704,7 @@`
		`odcs.server.utils.makedirs(symlink_dir)`

		`# Generate the non-latest symlink.`
		`- compose_dir = os.path.relpath(compose.toplevel_work_dir, symlink_dir)`
		`+ compose_dir = os.path.relpath(compose.toplevel_dir, symlink_dir)`
		`symlink = os.path.join(symlink_dir, compose.pungi_compose_id)`
		`log.info("%r: Generating %s symlink.", compose, symlink)`
		`os.symlink(compose_dir, symlink)`
		`@@ -922,26 +922,6 @@`

		`compose = Compose.query.filter(Compose.id == compose_id).one()`

		- # Create `odcs-$COMPOSE_ID` symlink pointing to toplevel_dir (usually
		`- # latest-odcs-$COMPOSE_ID-1 directory). This is needed for future`
		`- # to switch the directory layout - we need to ensure that`
		- # `odcs-$COMPOSE_ID` directory exists for all the already generated
		`- # composes.`
		`- # It also makes it easier to access generated compose, because the`
		- # `odcs-$COMPOSE_ID` directory name can be predicted easily.
		`- try:`
		`- symlink = os.path.join(compose.target_dir, compose.name)`
		`- os.symlink(compose.toplevel_dir, symlink)`
		`- except OSError as e:`
		`- if e.errno == errno.EEXIST:`
		`- # This can happen in case the compose reused another compose. In this`
		- # case, the `compose.name` and also the `compose.toplevel_dir` will point
		`- # to another compose. We still need to create the symlink even for this old`
		- # compose which is reused to ensure it has the `odcs-$COMPOSE_ID` directory.
		`- pass`
		`- else:`
		`- raise`
		`-`
		`koji_tag_cache = KojiTagCache(compose)`
		`koji_tag_cache.cleanup_reused(compose)`

server/odcs/server/models.py

file modified

+12 -30

		`@@ -25,7 +25,6 @@`
		`"""`

		`import os`
		`- import glob`

		`from datetime import datetime, timedelta`

		`@@ -269,30 +268,15 @@`
		`return "odcs-%d" % self.id`

		`@property`
		`- def latest_dir(self):`
		`- return "latest-%s-1" % self.name`
		`+ def toplevel_dir(self):`
		`+ return os.path.join(conf.target_dir, self.name)`

		`@property`
		`- def toplevel_work_dir(self):`
		`- # In case this compose failed, there won't be latest-* directory,`
		`- # but there might be the odcs-$id-1-$date.n.0 directory.`
		`- # The issue is that we cannot really know the date, because there is`
		`- # a race between we start Pungi and when Pungi generates that dir,`
		- # so just use `glob` to find out the rigth directory.
		`- glob_str = os.path.join(`
		`- self.target_dir, "odcs-%d-1-*.n.0" % self.id)`
		`- toplevel_dirs = glob.glob(glob_str)`
		`- if toplevel_dirs:`
		`- return toplevel_dirs[0]`
		`- return None`
		`+ def toplevel_url(self):`
		`+ if not self.on_default_target_dir:`
		`+ return ""`

		`- @property`
		`- def toplevel_dir(self):`
		`- if self.state == COMPOSE_STATES["failed"]:`
		`- toplevel_dir = self.toplevel_work_dir`
		`- if toplevel_dir:`
		`- return toplevel_dir`
		`- return os.path.join(self.target_dir, self.latest_dir)`
		`+ return conf.target_dir_url + "/" + self.name`

		`@property`
		`def result_repo_dir(self):`
		`@@ -310,17 +294,15 @@`
		`if not self.on_default_target_dir:`
		`return ""`

		`- target_dir_url = conf.target_dir_url`
		`-`
		`- return target_dir_url + "/" \`
		`- + os.path.join(self.latest_dir, "compose", "Temporary")`
		`+ return conf.target_dir_url + "/" \`
		`+ + os.path.join(self.name, "compose", "Temporary")`

		`@property`
		`def result_repofile_path(self):`
		`"""`
		`Returns path to .repo file.`
		`"""`
		`- return os.path.join(self.toplevel_dir, "compose", "Temporary",`
		`+ return os.path.join(self.name, "compose", "Temporary",`
		`self.name + ".repo")`

		`@property`
		`@@ -331,9 +313,8 @@`
		`if not self.on_default_target_dir:`
		`return ""`

		`- target_dir_url = conf.target_dir_url`
		`- return target_dir_url + "/" \`
		`- + os.path.join(self.latest_dir, "compose", "Temporary",`
		`+ return conf.target_dir_url + "/" \`
		`+ + os.path.join(self.name, "compose", "Temporary",`
		`self.name + ".repo")`

		`@validates('state')`
		`@@ -381,6 +362,7 @@`
		`'removed_by': self.removed_by,`
		`'result_repo': self.result_repo_url,`
		`'result_repofile': self.result_repofile_url,`
		`+ 'toplevel_url': self.toplevel_url,`
		`'flags': flags,`
		`'results': results,`
		`'sigkeys': self.sigkeys if self.sigkeys else "",`

server/odcs/server/pungi.py

file modified

+6 -6

		`@@ -449,22 +449,22 @@`
		`"""`
		`Returns the path to pungi.global.log if it exists.`
		`"""`
		`- toplevel_work_dir = self.compose.toplevel_work_dir`
		`- if not toplevel_work_dir:`
		`+ toplevel_dir = self.compose.toplevel_dir`
		`+ if not toplevel_dir:`
		`return None`
		`return os.path.join(`
		`- toplevel_work_dir, "logs", "global", "pungi.global.log")`
		`+ toplevel_dir, "logs", "global", "pungi.global.log")`

		`@property`
		`def config_dump_path(self):`
		`"""`
		`Returns path to Pungi config dump.`
		`"""`
		`- toplevel_work_dir = self.compose.toplevel_work_dir`
		`- if not toplevel_work_dir:`
		`+ toplevel_dir = self.compose.toplevel_dir`
		`+ if not toplevel_dir:`
		`return None`
		`return os.path.join(`
		`- toplevel_work_dir, "logs", "global", "config-dump.global.log")`
		`+ toplevel_dir, "logs", "global", "config-dump.global.log")`

		`def _get_global_log_errors(self):`
		`"""`

server/tests/test_backend.py

file modified

+7 -4

		`@@ -595,8 +595,7 @@`
		`}`
		`pulp_rest_post.assert_called_once_with('repositories/search/',`
		`expected_query)`
		`- symlink.assert_called_once_with(`
		`- AnyStringWith("/odcs-1-2018-1"), AnyStringWith("/odcs-1"))`
		`+ symlink.assert_not_called()`

		`@patch("odcs.server.pulp.Pulp._rest_post")`
		`@patch("odcs.server.backend._write_repo_file")`
		`@@ -1034,9 +1033,9 @@`

		`makedirs.assert_called_once_with(AnyStringWith("/test_composes/production"))`
		`symlink.assert_has_calls([`
		`- call('../odcs-1-2018-1',`
		`+ call('../odcs-1',`
		`AnyStringWith('/test_composes/production/compose-1-10-2020110.n.0')),`
		`- call('../odcs-1-2018-1',`
		`+ call('../odcs-1',`
		`AnyStringWith('/test_composes/production/latest-compose-1')),`
		`])`
		`unlink.assert_called_with(`
		`@@ -1054,6 +1053,10 @@`
		`COMPOSE_RESULTS["repository"], 60, packages='pkg1 pkg2 pkg3')`
		`db.session.commit()`

		`+ # Remove any previous toplevel_dir.`
		`+ if os.path.exists(self.c.toplevel_dir):`
		`+ shutil.rmtree(self.c.toplevel_dir)`
		`+`
		`compose_dir = os.path.join(self.c.toplevel_dir, 'compose')`
		`metadata_dir = os.path.join(compose_dir, 'metadata')`
		`self.rpms_metadata = os.path.join(metadata_dir, 'rpms.json')`

server/tests/test_composerthread.py

file modified

+4 -4

		`@@ -276,8 +276,8 @@`
		`self.assertEqual(c.reused_id, 1)`
		`self.assertEqual(c.state, COMPOSE_STATES["done"])`
		`self.assertEqual(c.result_repo_dir,`
		`- os.path.join(odcs.server.conf.target_dir, "latest-odcs-1-1/compose/Temporary"))`
		`- self.assertEqual(c.result_repo_url, "http://localhost/odcs/latest-odcs-1-1/compose/Temporary")`
		`+ os.path.join(odcs.server.conf.target_dir, "odcs-1/compose/Temporary"))`
		`+ self.assertEqual(c.result_repo_url, "http://localhost/odcs/odcs-1/compose/Temporary")`

		`@patch("odcs.server.utils.execute_cmd")`
		`@patch("odcs.server.backend.create_koji_session")`
		`@@ -301,8 +301,8 @@`
		`self.assertEqual(c.reused_id, None)`
		`self.assertEqual(c.state, COMPOSE_STATES["done"])`
		`self.assertEqual(c.result_repo_dir,`
		`- os.path.join(odcs.server.conf.target_dir, "latest-odcs-2-1/compose/Temporary"))`
		`- self.assertEqual(c.result_repo_url, "http://localhost/odcs/latest-odcs-2-1/compose/Temporary")`
		`+ os.path.join(odcs.server.conf.target_dir, "odcs-2/compose/Temporary"))`
		`+ self.assertEqual(c.result_repo_url, "http://localhost/odcs/odcs-2/compose/Temporary")`


		`class TestComposerThreadLostComposes(ModelsBaseTest):`

server/tests/test_models.py

file modified

+4 -3

		`@@ -56,8 +56,8 @@`
		`'state_reason': None,`
		`'source': u'testmodule-master',`
		`'owner': u'me',`
		`- 'result_repo': 'http://localhost/odcs/latest-odcs-1-1/compose/Temporary',`
		`- 'result_repofile': 'http://localhost/odcs/latest-odcs-1-1/compose/Temporary/odcs-1.repo',`
		`+ 'result_repo': 'http://localhost/odcs/odcs-1/compose/Temporary',`
		`+ 'result_repofile': 'http://localhost/odcs/odcs-1/compose/Temporary/odcs-1.repo',`
		`'time_submitted': c.json()["time_submitted"], 'id': 1,`
		`'time_started': None,`
		`'time_removed': None,`
		`@@ -80,7 +80,8 @@`
		`'compose_type': None,`
		`'pungi_compose_id': None,`
		`'pungi_config_dump': 'test',`
		`- 'target_dir': 'default'}`
		`+ 'target_dir': 'default',`
		`+ 'toplevel_url': 'http://localhost/odcs/odcs-1'}`
		`self.assertEqual(c.json(True), expected_json)`

		`def test_target_dir_none(self):`

server/tests/test_pulp.py

file modified

+1 -1

		`@@ -186,7 +186,7 @@`
		`ret,`
		`{`
		`"foo-1": {`
		`- "url": "http://localhost/odcs/latest-odcs-1-1/compose/Temporary/foo-1/$basearch",`
		`+ "url": "http://localhost/odcs/odcs-1/compose/Temporary/foo-1/$basearch",`
		`"arches": set(["x86_64", "ppc64le"]),`
		`"sigkeys": ["SIG1", "SIG2"],`
		`}`

server/tests/test_pungi.py

file modified

-1

		`@@ -603,7 +603,6 @@`

		`def test_toplevel_work_dir(self):`
		`# The self.glob is inherited from ModelsBaseTest.`
		`- self.glob.return_value = []`
		`pungi_logs = PungiLogs(self.compose)`
		`errors = pungi_logs.get_error_string()`
		`self.assertEqual(errors, "")`

server/tests/test_remove_expired_composes_thread.py

file modified

+3 -2

		`@@ -138,12 +138,13 @@`
		`@patch("odcs.server.backend.RemoveExpiredComposesThread._remove_compose_dir")`
		`def test_remove_left_composes(self, remove_compose_dir, glob, isdir):`
		`isdir.return_value = True`
		`- self._mock_glob(glob, ["latest-odcs-96-1", "odcs-96-1-20171005.n.0"])`
		`+ self._mock_glob(glob, ["latest-odcs-96-1", "odcs-96-1-20171005.n.0", "odcs-96"])`
		`self.thread.do_work()`
		`self.assertEqual(`
		`remove_compose_dir.call_args_list,`
		`[mock.call(os.path.join(conf.target_dir, "latest-odcs-96-1")),`
		`- mock.call(os.path.join(conf.target_dir, "odcs-96-1-20171005.n.0"))])`
		`+ mock.call(os.path.join(conf.target_dir, "odcs-96-1-20171005.n.0")),`
		`+ mock.call(os.path.join(conf.target_dir, "odcs-96"))])`

		`@patch("os.path.isdir")`
		`@patch("glob.glob")`

server/tests/test_views.py

file modified

+8 -6

		`@@ -299,8 +299,8 @@`
		`'state_reason': None,`
		`'source': u'testmodule:master',`
		`'owner': u'dev',`
		`- 'result_repo': 'http://localhost/odcs/latest-odcs-%d-1/compose/Temporary' % data['id'],`
		`- 'result_repofile': 'http://localhost/odcs/latest-odcs-%d-1/compose/Temporary/odcs-%d.repo' % (data['id'], data['id']),`
		`+ 'result_repo': 'http://localhost/odcs/odcs-%d/compose/Temporary' % data['id'],`
		`+ 'result_repofile': 'http://localhost/odcs/odcs-%d/compose/Temporary/odcs-%d.repo' % (data['id'], data['id']),`
		`'time_submitted': data["time_submitted"], 'id': data['id'],`
		`'time_started': None,`
		`'time_removed': None,`
		`@@ -322,7 +322,8 @@`
		`'label': None,`
		`'compose_type': 'test',`
		`'pungi_compose_id': None,`
		`- 'target_dir': 'default'}`
		`+ 'target_dir': 'default',`
		`+ 'toplevel_url': 'http://localhost/odcs/odcs-%d' % data['id']}`
		`self.assertEqual(data, expected_json)`

		`db.session.expire_all()`
		`@@ -1252,8 +1253,8 @@`
		`'state_reason': None,`
		`'source': u'testmodule:master',`
		`'owner': u'unknown',`
		`- 'result_repo': 'http://localhost/odcs/latest-odcs-%d-1/compose/Temporary' % data['id'],`
		`- 'result_repofile': 'http://localhost/odcs/latest-odcs-%d-1/compose/Temporary/odcs-%d.repo' % (data['id'], data['id']),`
		`+ 'result_repo': 'http://localhost/odcs/odcs-%d/compose/Temporary' % data['id'],`
		`+ 'result_repofile': 'http://localhost/odcs/odcs-%d/compose/Temporary/odcs-%d.repo' % (data['id'], data['id']),`
		`'time_submitted': data["time_submitted"], 'id': data['id'],`
		`'time_started': None,`
		`'time_removed': None,`
		`@@ -1275,7 +1276,8 @@`
		`'label': None,`
		`'compose_type': 'test',`
		`'pungi_compose_id': None,`
		`- 'target_dir': 'default'}`
		`+ 'target_dir': 'default',`
		`+ 'toplevel_url': 'http://localhost/odcs/odcs-%d' % data['id']}`
		`self.assertEqual(data, expected_json)`

		`db.session.expire_all()`

server/tests/utils.py

file modified

-10

		`@@ -21,14 +21,12 @@`
		`#`
		`# Written by Chenxiong Qi <cqi@redhat.com>`

		`- import os`
		`import unittest`

		`from odcs.server import db`
		`from sqlalchemy import event`
		`from odcs.server.events import cache_composes_if_state_changed`
		`from odcs.server.events import start_to_publish_messages`
		`- from odcs.server import conf`

		`from flask_sqlalchemy import SignallingSession`
		`from mock import patch`
		`@@ -102,12 +100,6 @@`
		`event.listen(SignallingSession, 'after_commit',`
		`start_to_publish_messages)`

		`- # Make Compose.toplevel_work_dir to work everytime.`
		`- self.patch_glob = patch("odcs.server.models.glob.glob")`
		`- self.glob = self.patch_glob.start()`
		`- self.glob.return_value = [`
		`- os.path.join(conf.target_dir, "odcs-1-2018-1")]`
		`-`
		`def tearDown(self):`
		`if not self.disable_event_handlers:`
		`event.remove(SignallingSession, 'after_flush',`
		`@@ -125,5 +117,3 @@`
		`cache_composes_if_state_changed)`
		`event.listen(SignallingSession, 'after_commit',`
		`start_to_publish_messages)`
		`-`
		`- self.patch_glob.stop()`

jkaluza commented 4 years ago

The code is simpler now, because there is no need to make
differences between toplevel_work_dir and toplevel_dir. We
only use toplevel_dir now which points to odcs-$COMPOSE_ID.
The toplevel_url is included in the API response, so it is
possible to use it to get to compose root directly from API.
The full-path to compose can now be easily generated by external
scripts by simply knowing the compose id.

Signed-off-by: Jan Kaluza jkaluza@redhat.com

rebased onto 1cf476577ebbc48bc1273c8ba9f4711c1b553644

4 years ago

rebased onto cf9de09a965f8faa9ae40e33109eb38ee84c2c71

4 years ago

lsedlar commented 4 years ago

Is deleting expired composes working for composes that were generated using the old scheme? It should be, since they store the path, right?

jkaluza commented 4 years ago

The paths are not stored in database, but the code to remove composes is quite robust and it checks all compose directories and is able to find out compose in old/new scheme and remove it.

The issue might be with Pulp composes. The old Pulp composes will have data stored in latest- directory but after this upgrade, the API will start returing odcs- directory. I will think what to do about it. :(

jkaluza commented 4 years ago

Please let keep this PR open. I will need to do two PRs which will have to be done in two ODCS release:

First PR will store the compose name in database and we will need to release and deploy ODCS with this PR.
Once this is done, we can actually work on this PR changing the directory to odcs-$COMPOSE_ID.

rebased onto e301b1047ced0f7a280a480a74413a6f9aea383d

4 years ago

rebased onto eabde17e9837981fcfbd8c3d8e266099291702e8

4 years ago

jkaluza commented 4 years ago

@lsedlar , this is ready for another review I think.

In the previous release, we ensured that odcs-$COMPOSE_ID directory will always exist by creating symlink to old odcs toplevel directory. This is already deployed for some time and all the current composes have this directory.

We can therefore do new release with this PR and start using the $odcs-COMPOSE_ID directory instead of the old odcs toplevel directory.

I think we should test this on Fedora at first for any issues related to migration and removal of old compose directories.

rebased onto 2dcbdb9

4 years ago