PR#440: Do not remove compose if latest-* symlink points to it. - odcs

odcs

#440 Do not remove compose if latest-* symlink points to it.

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

jkaluza/odcs keep-latest into master

Do not remove compose if latest-* symlink points to it.

Jan Kaluza • 3 years ago

e11b925

server/odcs/server/backend.py

file modified

+28 -9

		`@@ -218,6 +218,11 @@`
		`if not os.path.exists(compose.target_dir):`
		`continue`

		`+ # Do not remove the latest successfull compose.`
		`+ if get_latest_symlink(compose):`
		`+ log.info("%r: Not removing compose, the latest-* symlink points to it.")`
		`+ continue`
		`+`
		`log.info("%r: Removing compose", compose)`
		`if compose.removed_by:`
		`state_reason = "Removed by {}.".format(compose.removed_by)`
		`@@ -837,6 +842,27 @@`
		`os.symlink(compose_dir, symlink)`


		`+ def get_latest_symlink(compose):`
		`+ """`
		`+ Returns the latest-* symlink associated with this compose or None`
		`+ if it does not exists.`
		`+ """`
		`+ # Ignore old composes which do not have the compose_type and pungi_compose_id set.`
		`+ if not compose.compose_type or not compose.pungi_compose_id:`
		`+ return None`
		`+`
		`+ symlink_dir = os.path.join(compose.target_dir, compose.compose_type)`
		`+ symlink = os.path.join(symlink_dir, compose.pungi_compose_id)`
		`+ latest_name = "latest-%s" % "-".join(compose.pungi_compose_id.split("-")[:2])`
		`+ latest_symlink = os.path.join(symlink_dir, latest_name)`
		`+`
		+ # Return Non if `latest_symlink` points to the different dir than `symlink`.
		`+ if os.path.realpath(symlink) != os.path.realpath(latest_symlink):`
		`+ return None`
		`+`
		`+ return latest_symlink`
		`+`
		`+`
		`def remove_compose_symlink(compose):`
		`"""`
		Removes non-latest symlink generated by the `generate_compose_symlink`.
		`@@ -848,14 +874,7 @@`

		`symlink_dir = os.path.join(compose.target_dir, compose.compose_type)`
		`symlink = os.path.join(symlink_dir, compose.pungi_compose_id)`
		`-`
		`- # Check if latest_symlink points to the same directory as the non-latest`
		`- # symlink. In this case, we will remove the latest-symlink later too.`
		`- latest_name = "latest-%s" % "-".join(compose.pungi_compose_id.split("-")[:2])`
		`- latest_symlink = os.path.join(symlink_dir, latest_name)`
		`- remove_latest_symlink = os.path.realpath(symlink) == os.path.realpath(`
		`- latest_symlink`
		`- )`
		`+ latest_symlink = get_latest_symlink(compose)`

		`# Remove non-latest symlink.`
		`log.info("%r: Removing %s symlink.", compose, symlink)`
		`@@ -866,7 +885,7 @@`
		`raise`

		`# Remove latest symlink.`
		`- if remove_latest_symlink:`
		`+ if latest_symlink:`
		`log.info("%r: Removing %s symlink.", compose, latest_symlink)`
		`try:`
		`os.unlink(latest_symlink)`

server/tests/test_remove_expired_composes_thread.py

file modified

+25 -2

		`@@ -76,10 +76,14 @@`
		`@patch("os.unlink")`
		`@patch("os.path.realpath")`
		`@patch("os.path.exists")`
		`- def test_a_compose_which_state_is_done_is_removed(self, exists, realpath, unlink):`
		`+ @patch("odcs.server.backend.get_latest_symlink")`
		`+ def test_a_compose_which_state_is_done_is_removed(`
		`+ self, latest_symlink, exists, realpath, unlink`
		`+ ):`
		`"""`
		`Test that we do remove a compose in done state.`
		`"""`
		`+ latest_symlink.return_value = None`
		`realpath.return_value = "/odcs-real"`
		`c = db.session.query(Compose).filter(Compose.id == 1).one()`
		`c.time_to_expire = datetime.utcnow() - timedelta(seconds=120)`
		`@@ -98,11 +102,30 @@`
		`mock.call(`
		`AnyStringWith("test_composes/nightly/compose-1-10-2020110.n.0")`
		`),`
		`- mock.call(AnyStringWith("test_composes/nightly/latest-compose-1")),`
		`mock.call(AnyStringWith("test_composes/odcs-1")),`
		`]`
		`)`

		`+ @patch("os.unlink")`
		`+ @patch("os.path.realpath")`
		`+ @patch("os.path.exists")`
		`+ def test_latest_compose_not_removed(self, exists, realpath, unlink):`
		`+ """`
		`+ Test that we do remove a compose in done state.`
		`+ """`
		`+ realpath.return_value = "/odcs-real"`
		`+ c = db.session.query(Compose).filter(Compose.id == 1).one()`
		`+ c.time_to_expire = datetime.utcnow() - timedelta(seconds=120)`
		`+ c.state = COMPOSE_STATES["done"]`
		`+ c.compose_type = "nightly"`
		`+ c.pungi_compose_id = "compose-1-10-2020110.n.0"`
		`+ db.session.add(c)`
		`+ db.session.commit()`
		`+ self.thread.do_work()`
		`+ db.session.expunge_all()`
		`+ c = db.session.query(Compose).filter(Compose.id == 1).one()`
		`+ self.assertEqual(c.state, COMPOSE_STATES["done"])`
		`+`
		`def test_a_compose_which_state_is_done_is_removed_keep_state_reason(self):`
		`"""`
		`Test that we do remove a compose in done state.`

jkaluza commented 3 years ago

There are two reasons for this:

The compose can still be reused by future composes to significantly
reduce their generation time. If we remove it after 24 hours, next
compose will have to generate everything from scratch again.
For periodic composes, we always want to have the latest compose
available so other service can point to this location. This is
needed for Fedora ELN or internal Integration composes.