PR#521: How to delete outdated chroots and some babyproofing - copr/copr

		`@@ -0,0 +1,84 @@`
		`+ .. _how_to_delete_outdated_chroots:`
		`+`
		`+ How to delete outdated chroots`
		`+ ==============================`
		`+`
		`+ .. note:: All of these tasks are automatized for the main Copr instance in Fedora.`
		`+ This page might be rather useful for maintainers of other instances or developers enhancing this feature.`
		`+`
		+ .. note:: Please read :ref:`copr_outdated_chroots_removal_policy` to see
		`+ how the deletion of outdated chroots work from the user perspective.`
		`+`
		`+`
		`+ This article explains how to mark a chroot as outdated, how to notify project owners, that some of their chroots are`
		`+ outdated and going to be deleted in the future, and lastly how to actually delete them.`
		`+`
		`+`
		`+ Mark chroot as outdated`
		`+ -----------------------`
		`+`
		+ This step is described as :ref:`eol_deactivation_process` in the :ref:`how_to_manage_chroots` document.
		`+ To briefly summarize it, first, it is required to mark a chroot as EOL (aka outdated).`
		`+`
		`+ ::`
		`+`
		`+ copr-frontend alter_chroot --action eol fedora-25-x86_64`
		`+`
		+ It doesn't matter whether the chroot is currently activated or deactivated, using ``--action eol``
		+ always deactivates it. More importantly, for every related ``CoprChroot`` it generates a ``delete_after`` timestamp
		`+ saying when the copr chroot should be deleted.`
		`+`
		`+`
		`+ Notify project admins`
		`+ ---------------------`
		`+`
		+ Once the ``delete_after`` is set, the notification command starts to notice such copr chroot. The following command
		`+ prints to the stdout a list of people and about what they should be notified. Please be aware, that not only`
		`+ project owners are going to be notified, but rather all project admins.`
		`+`
		`+ ::`
		`+`
		`+ copr-frontend notify_outdated_chroots --dry-run`
		`+`
		`+ When working on a non-production instance and wanting to really send the emails, filter the recipients to just yourself`
		`+ or team members. Any real users shouldn't be contacted from devel instances!`
		`+`
		`+ ::`
		`+`
		`+ copr-frontend notify_outdated_chroots --dry-run -e myself@redhat.com`
		`+`
		+ If this command prints that it would notify just the expected people (which were specified with the ``-e`` parameter),
		+ then it is safe to run it without ``--dry-run`` parameter.
		`+`
		`+ ::`
		`+`
		`+ copr-frontend notify_outdated_chroots -e myself@redhat.com`
		`+`
		`+`
		+ When the notification about a particular copr chroot is sent and then the ``notify_outdated_chroots`` command
		`+ is executed again, it will not send the notification for the second time. It is designed to be daily executed via Cron`
		`+ and it needs to avoid spamming the people over and over again. Therefore when a notification is sent, a timestamp when`
		+ to send a next one is stored to the ``delete_notify`` column. In a case that this logic needs to be suppressed,
		+ please use ``--all`` parameter. Then notifications are going to be sent regardless of the previous notification.
		`+`
		`+`
		`+ Delete the data`
		`+ ---------------`
		`+`
		+ Once the ``delete_after`` timestamp is reached, the particular copr chroot should be deleted. To print
		`+ all the chroots for which this applies, use this command.`
		`+`
		`+ ::`
		`+`
		`+ copr-frontend delete_outdated_chroots --dry-run`
		`+`
		`+ To really delete them (i.e. creating an action which will delete the chroot directory on the backend),`
		+ run the command without ``--dry-run`` parameter.
		`+`
		`+ ::`
		`+`
		`+ copr-frontend delete_outdated_chroots`
		`+`
		+ When deleting the chroot (creating an action to delete the data on the backend), the ``delete_after``
		+ and ``delete_notify`` columns are set to NULL and therefore ``notify_outdated_chroots``
		+ and ``delete_outdated_chroots`` commands don't see the chroot anymore.

doc/how_to_manage_chroots.rst

file modified

+3 -1

		`@@ -64,6 +64,8 @@`
		When everything is done, `send an information email to a mailing list <#mailing-lists>`_.


		`+ .. _eol_deactivation_process:`
		`+`
		`EOL deactivation process`
		`------------------------`

		`@@ -73,7 +75,7 @@`

		`::`

		`- copr-frontend alter_chroot --action deactivate fedora-25-x86_64 fedora-25-i386 fedora-25-ppc64le`
		`+ copr-frontend alter_chroot --action eol fedora-25-x86_64 fedora-25-i386 fedora-25-ppc64le`

		`After running such command, no data are going to be removed. All repositories for the chroot are preserved. It is just`
		`disabled and users can't build new packages in it anymore.`

doc/maintenance_documentation.rst

file modified

		`@@ -10,3 +10,5 @@`
		* :ref:`how_to_upgrade_builders` -- how to upgrade openstack builders

		* :ref:`how_to_manage_chroots` -- how to manage active chroots
		`+`
		+ * :ref:`how_to_delete_outdated_chroots` -- sending notifications and removing data from outdated chroots

docker/frontend/files/etc/copr/copr.conf

file modified

		`@@ -1,3 +1,7 @@`
		`+ # Purpose of this instance`
		`+ # Use "production", "devel", "local" or "test"`
		`+ ENV="local"`
		`+`
		`# Directory and files where is stored Copr database files`
		`#DATA_DIR = '/var/lib/copr/data'`
		`#DATABASE = '/var/lib/copr/data/copr.db'`

frontend/coprs_frontend/commands/delete_outdated_chroots.py

file modified

+19 -9

		`@@ -14,21 +14,31 @@`
		`]`

		`def run(self, dry_run):`
		`- self.dry_run = dry_run`
		`+ deleter = DryRunDeleter() if dry_run else Deleter()`

		`chroots = coprs_logic.CoprChrootsLogic \`
		`.filter_outdated_to_be_deleted(coprs_logic.CoprChrootsLogic.get_multiple())`
		`for i, chroot in enumerate(chroots, start=1):`
		`# This command will possibly delete a lot of chroots and can be a performance issue when committing`
		`# all at once. We are going to commit every x actions to avoid that.`
		`- if i % 1000 == 0 and not self.dry_run:`
		`- db.session.commit()`
		`- self.delete(chroot)`
		`+ if i % 1000 == 0:`
		`+ deleter.commit()`
		`+ deleter.delete(chroot)`
		`+ deleter.commit()`
		`+`
		`+`
		`+ class Deleter(object):`
		`+ def delete(self, chroot):`
		`+ actions_logic.ActionsLogic.send_delete_chroot(chroot)`
		`+ chroot.delete_after = None`
		`+`
		`+ def commit(self):`
		`db.session.commit()`

		`+`
		`+ class DryRunDeleter(object):`
		`def delete(self, chroot):`
		`- if self.dry_run:`
		`- print("Add delete_chroot action for {} in {}".format(chroot.name, chroot.copr.full_name))`
		`- else:`
		`- actions_logic.ActionsLogic.send_delete_chroot(chroot)`
		`- chroot.delete_after = None`
		`+ print("Add delete_chroot action for {} in {}".format(chroot.name, chroot.copr.full_name))`
		`+`
		`+ def commit(self):`
		`+ pass`

frontend/coprs_frontend/commands/notify_outdated_chroots.py

file modified

+30 -15

		`@@ -1,6 +1,7 @@`
		`+ import sys`
		`import datetime`
		`from flask_script import Command, Option`
		`- from coprs import db`
		`+ from coprs import db, app`
		`from coprs.logic import coprs_logic`
		`from coprs.mail import send_mail, OutdatedChrootMessage`

		`@@ -19,15 +20,20 @@`
		`]`

		`def run(self, dry_run, email_filter, all):`
		`- self.dry_run = dry_run`
		`self.email_filter = email_filter`
		`self.all = all`

		`+ if not dry_run:`
		`+ self.dev_instance_warning()`
		`+`
		`+ notifier = DryRunNotifier() if dry_run else Notifier()`
		`outdated = coprs_logic.CoprChrootsLogic.filter_outdated(coprs_logic.CoprChrootsLogic.get_multiple())`
		`for user, chroots in self.get_user_chroots_map(outdated).items():`
		`chroots = self.filter_chroots([chroot for chroot in chroots])`
		`- self.notify(user, chroots)`
		`- self.store_notify_timesamp(chroots)`
		`+ if not chroots:`
		`+ continue`
		`+ notifier.notify(user, chroots)`
		`+ notifier.store_timestamp(chroots)`

		`def get_user_chroots_map(self, chroots):`
		`user_chroot_map = {}`
		`@@ -56,19 +62,28 @@`

		`return filtered`

		`+ def dev_instance_warning(self):`
		`+ if app.config["ENV"] != "production" and not self.email_filter:`
		`+ sys.stderr.write("I will not let you send emails to all Copr users from the dev instance!\n")`
		`+ sys.stderr.write("Please use this command with -e myself@foo.bar\n")`
		`+ sys.exit(1)`
		`+`
		`+`
		`+ class Notifier(object):`
		`def notify(self, user, chroots):`
		`- if self.dry_run:`
		`- about = ["{0} ({1})".format(chroot.copr.full_name, chroot.name) for chroot in chroots]`
		`- print("Notify {} about {}".format(user.mail, about))`
		`- else:`
		`- msg = OutdatedChrootMessage(chroots)`
		`- send_mail(user.mail, msg)`
		`-`
		`- def store_notify_timesamp(self, chroots):`
		`- if self.dry_run:`
		`- return`
		`+ msg = OutdatedChrootMessage(chroots)`
		`+ send_mail(user.mail, msg)`
		`+`
		`+ def store_timestamp(self, chroots):`
		`for chroot in chroots:`
		`- chroot.delete_after_notify = datetime.datetime.now()`
		`+ chroot.delete_notify = datetime.datetime.now()`
		`db.session.commit()`


		`+ class DryRunNotifier(object):`
		`+ def notify(self, user, chroots):`
		`+ about = ["{0} ({1})".format(chroot.copr.full_name, chroot.name) for chroot in chroots]`
		`+ print("Notify {} about {}".format(user.mail, about))`
		`+`
		`+ def store_timestamp(self, chroots):`
		`+ pass`

frontend/coprs_frontend/config/copr.conf

file modified

		`@@ -1,3 +1,7 @@`
		`+ # Purpose of this instance`
		`+ # Use "production", "devel", "local" or "test"`
		`+ ENV="devel"`
		`+`
		`# Directory and files where is stored Copr database files`
		`#DATA_DIR = '/var/lib/copr/data'`
		`#DATABASE = '/var/lib/copr/data/copr.db'`

frontend/coprs_frontend/coprs/config.py

file modified

		`@@ -3,6 +3,7 @@`


		`class Config(object):`
		`+ ENV = "devel"`
		`DATA_DIR = os.path.join(os.path.dirname(__file__), "../../data")`
		`DATABASE = os.path.join(DATA_DIR, "copr.db")`
		`OPENID_STORE = os.path.join(DATA_DIR, "openid_store")`

frontend/coprs_frontend/coprs/mail.py

file modified

		`@@ -91,6 +91,9 @@`
		`"Please, visit the projects settings if you want to extend the time.\n\n"`
		`.format(app.config["DELETE_EOL_CHROOTS_AFTER"]))`

		`+ if not copr_chroots:`
		`+ raise AttributeError("No outdated chroots to notify about")`
		`+`
		`for chroot in copr_chroots:`
		`self.text += (`
		`"Project: {0}\n"`

frontend/coprs_frontend/tests/test_mail.py

file modified

		`@@ -1,3 +1,4 @@`
		`+ import pytest`
		`import datetime`
		`from coprs.mail import PermissionRequestMessage, PermissionChangeMessage, LegalFlagMessage, OutdatedChrootMessage`
		`from tests.coprs_test_case import CoprsTestCase`
		`@@ -62,3 +63,8 @@`
		`"Chroot: fedora-17-x86_64\n"`
		`"Remaining: 7 days\n"`
		`"http://localhost/coprs/user2/foocopr/repositories/\n\n")`
		`+`
		`+ def test_outdated_chroot_message_empty_chroots(self):`
		`+ with pytest.raises(AttributeError) as ex:`
		`+ OutdatedChrootMessage(copr_chroots=[])`
		`+ assert "No outdated chroots" in str(ex)`

frostyx commented 5 years ago

Finally, I am adding a document describing how to notify and delete about outdated chroots. Also, I am adding some safeties, to ensure that maintainer can't send emails to all users from dev instance or notify about an empty list of chroots.

frostyx commented 5 years ago

@msuchy I know you wanted this written in SOP, but it is quite a long document and I prefer it as a separate page. We can surely link this from SOP if you want.

msuchy commented on line 39 of frontend/coprs_frontend/commands/notify_outdated_chroots.py 5 years ago

I would rather see some app config option like ALLOW_TO_SEND_EMAIL or even something more general like DEV_INSTANCE.

msuchy commented 5 years ago

Otherwise +1

frostyx commented on line 39 of frontend/coprs_frontend/commands/notify_outdated_chroots.py 5 years ago

I was also thinking about adding a variable ENV which would be set to either "production", "devel" (for copr-*-dev) or "local" (for docker/vagrant/whatever instance on your laptop). I gave up on it since the only usage was for this use-case. But what do you think about that? I think, that it is a bit better than ALLOW_TO_SEND_EMAIL or DEV_INSTANCE.

frostyx commented 5 years ago

ad hardcoding "copr-fe-dev" when warning about dev instance:

We have discussed several options in a meeting. I suggested, that we can abuse SEND_EMAILS config variable for this and @praiskup suggested whitelisting only admin emails.

I thought it through and I am afraid, that it won't make things easier for us. First, the SEND_EMAILS is about sending emails, not sending public emails, so using it for this use case would be abusing, but whatever. It is set to True on production and to False in docker, so that's good. However, on copr-fe-dev, it is set to True because in general, we want to send emails from that instance (when asking for permissions or raising a legal flag - we want to test even emailing). Therefore, SEND_EMAILS is not an option.

Whitelisting sounds like a good feature and at first, I thought, that we can filter all admins from database and whitelist just them. Then I realized, that I can't do just that because we want this only for dev - and here we are again in the original issue - how to recognize, that we are not on production. Other option would be explicitly defining something like EMAIL_WHITELIST in the config. But it is also a config change and we would additionally have to manage the emails there.

While the whitelist feature is good, I am going to proceed in this PR with adding ENV variable into config as I've described it in the comment above ^^. Then, in other PR, we can add a whitelist to admins-only on the mail.send_mail level, so we can sleep peacefully, that we can't send emails from dev instances to the public.

Do you agree with me?

praiskup commented 5 years ago

Other option would be explicitly defining something like EMAIL_WHITELIST in the config. But it is also a config change and we would additionally have to manage the emails there.

I originally thought about some configuration option for this purpose, even though used for development only boxes.