[WIP] Add script and tests for moving a batched request to a stable request #1678

crungehottman · 2017-07-11T01:22:08Z

This will require a cron job added to https://infrastructure.fedoraproject.org/cgit/ansible.git/tree/roles/bodhi2/backend/tasks/main.yml

- name: bodhi-dequeue-stable cron job.
  cron: name="bodhi-dequeue-stable" weekday="1" hour="6" minute=0 user="apache"
        job="/usr/bin/bodhi-dequeue-stable /etc/bodhi/production.ini > /dev/null"
        cron_file=bodhi-dequeue-stable-job
  when: inventory_hostname.startswith('bodhi-backend02') and env == "production"
  tags:
  - config
  - bodhi
  - cron

Note: Monday was arbirtarily suggested following #1157. Times are arbirtrarily selected as well and will likely require review/further discussion.

centos-ci · 2017-07-11T01:22:28Z

This pull request fails CI testing. Please review the Jenkins job.

bowlofeggs · 2017-07-11T15:48:12Z

It looks like CI failed due to the conflict. Jenkins attempts to rebase all PRs on the develop branch before testing so that we can know the PR was tested against the latest commits.

Can you rebase your branch on develop to resolve the conflicts?

bowlofeggs

Just a few initial thoughts. Please do rebase your pull request, and I'll give some more feedback after that!

bowlofeggs · 2017-07-11T15:53:51Z

bodhi/server/scripts/dequeue_stable.py

+"""This script is responsible for moving all updates with a batched request to a stable request."""
+
+import sys
+import click


For PEP-8, there should be a space between the sys import and the click import, since sys is in the standard library and click is a third party library.

bowlofeggs · 2017-07-11T15:55:14Z

bodhi/server/scripts/dequeue_stable.py

+    db = Session()
+
+    try:
+        batched = db.query(models.Update).filter_by(request=models.UpdateRequest.batched).all()


@jeremycline recently made a cool way to get a query object that will allow you to drop the db = Session() line above and convert this line to be:

batches = models.Update.query.filter_by(request=models.UpdateRequest.batched).all()

It's a little simpler.

bowlofeggs · 2017-07-11T16:00:06Z

bodhi/server/scripts/dequeue_stable.py

+        batched = db.query(models.Update).filter_by(request=models.UpdateRequest.batched).all()
+
+        for update in batched:
+            update.request = models.UpdateRequest.stable


I saw in the models.py code above that it was doing something like this:

self.set_request(db, UpdateRequest.stable, agent) self.request = models.UpdateRequest.stable

The second line of that seemed a little redundant to me since set_request() seems to do what its name implies. Anyways, I think we probably do want to call set_request() since it does do a few things other than just setting the request.

I also just noted that set_request() currently wants the db Session object which I just advised you to get rid of. This leaves you with two choices: you can refactor set_request() so that it doesn't need the db Session, or you can ignore my previous advice and we can refactor set_request() another day. I'd advise keeping the concerns separate and refactoring another day.

bowlofeggs · 2017-07-11T16:05:38Z

bodhi/server/scripts/dequeue_stable.py

+        print(str(e))
+        db.rollback()
+        Session.remove()
+        sys.exit(1)


This is a fine way to do it, but I'll mention that we also have a TransactionalSessionMaker that you can use to handle the db commit/rollback for you. Here's an example of using it:

initialize_db(config) db_factory = transactional_session_maker() with db_factory() as session: # Your code goes here, and the db_factory will automatically commit for you if no Exception is raised. It will also # automatically rollback if one is. Oh, and the ```session``` it gives you is the same thing as the ```db``` variable # you have defined, so that's handy too.

bowlofeggs · 2017-07-11T16:06:40Z

bodhi/server/scripts/untag_branched.py

@@ -14,7 +14,7 @@
 """
 Used to remove the pending and testing tags from updates in a branched release.

-Since a seperate task mashes the branched stable repos, this will leave
+Since a separate task mashes the branched stable repos, this will leave


I recommend fixing this spelling error in a separate pull request. This way we can keep the concerns separate and make this PR focus only on making the batched stuff work. Also, the spelling error PR can help pad your GitHub stats ☺

bowlofeggs · 2017-07-11T16:09:13Z

bodhi/tests/server/scripts/test_dequeue_stable.py

+
+        result = runner.invoke(dequeue_stable.dequeue_stable, [])
+        self.assertEqual(result.exit_code, 0)
+        self.assertEqual(update.request, models.UpdateRequest.stable)


I would expect that you would need to do another query to refresh the update. Does this line not raise an Exception? Usually sqlalchemy gets real upset if you use an object outside of the transaction it came from. You could/should probably put this line first:

update = self.db.query(models.Update).all()[0]

bowlofeggs · 2017-07-11T16:11:01Z

bodhi/server/validators.py

@@ -1036,7 +1036,7 @@ def validate_request(request):
        return
    if request.validated['request'] is UpdateRequest.stable:
        target = UpdateStatus.stable
-    elif request.validated['request'] is UpdateRequest.testing:
+    elif request.validated['request'] is (UpdateRequest.testing or UpdateRequest.batched):


I noticed that there are some places where you added or statements between testing and batched, where I would expect batched to make more sense in comparison to stable than testing. However, my expectations could also be flawed. Do you remember why each of these testing lines were or'd with batched?

centos-ci · 2017-07-11T16:30:08Z

This pull request fails CI testing. Please review the Jenkins job.

bowlofeggs · 2017-07-11T20:45:02Z

The tests pass, but it appears we need a bit more coverage on a few lines:

$ diff-cover coverage.xml --compare-branch=origin/develop --fail-under=100
-------------
Diff Coverage
Diff: origin/develop...HEAD, staged and unstaged changes
-------------
bodhi/server/models.py (100%)
bodhi/server/scripts/dequeue_stable.py (72.2%): Missing lines 36-40
bodhi/server/validators.py (100%)
-------------
Total:   32 lines
Missing: 5 lines
Coverage: 84%
-------------
Failure. Coverage is below 100.0%.

bowlofeggs

Can you add a line at the end of your commit message that says

re #1157

? That will make it so your commit gets referenced on that ticket, but won't close the ticket when it is merged (since we'll have more to do after this). It's nice to be able to see the related work on the ticket.

Just a few more things to change and we'll be good on this one. This looks really good Caleigh, nice work!

bowlofeggs · 2017-07-20T15:31:46Z

bodhi/server/models.py

+                if self.severity is UpdateSeverity.urgent or self.type is UpdateType.security:
+                    log.info("Automatically marking %s as stable" % self.title)
+                    self.set_request(db, UpdateRequest.stable, agent)
+                    # self.request = UpdateRequest.stable


I think we can delete this line since set_request sets the request to stable.

bowlofeggs · 2017-07-20T15:32:38Z

bodhi/server/models.py

+                    log.info("Automatically adding %s to batch of updates that will be pushed to"
+                             " stable at a later date" % self.title)
+                    self.set_request(db, UpdateRequest.batched, agent)
+                    # self.request = UpdateRequest.batched


I think we can similarly delete this line.

bowlofeggs · 2017-07-20T15:33:33Z

bodhi/server/models.py

+                    self.date_pushed = None
+                    notifications.publish(
+                        topic='update.karma.threshold.reach',
+                        msg=dict(update=self, status='stable'))


Technically this event has also happened for the batched one in the else clause, so we can probably move this publish to be outside the if/else.

bowlofeggs · 2017-07-20T15:43:50Z

bodhi/server/scripts/dequeue_stable.py

@@ -0,0 +1,40 @@
+# This program is free software; you can redistribute it and/or


Let's add a Copyright claim here. Only, I'm not quite sure who to put as the Copyright holder. I started a thread on outreachy-list to ask. If you are on that list, perhaps you can use their feedback to know what to do here. Anyways, once we know who or what organization to give the Copyright to, let's do something like this here:

# -*- coding: utf-8 -*- # Copyright © 2017 <copyright holder here> # # This file is part of Bodhi. #

and then the rest of the header like you have it here.

According to Tony Sebro, you get to hold the copyright on this code (neat, huh? In most places, your employer would hold the copyright on code that you are paid to write so that's pretty cool I think). So you should do this in this file and the test file you wrote:

# -*- coding: utf-8 -*- # Copyright © 2017 Caleigh Runge-Hottman # # This file is part of Bodhi. # <the rest of the file as you have it, including the GPL stuff>

bowlofeggs · 2017-07-20T15:44:51Z

bodhi/tests/server/scripts/test_dequeue_stable.py

@@ -0,0 +1,59 @@
+# This program is free software; you can redistribute it and/or


Similarly, let's put a Copyright claim here.

bowlofeggs · 2017-07-20T15:45:48Z

bodhi/tests/server/scripts/test_dequeue_stable.py

+"""
+from click import testing
+
+from datetime import datetime, timedelta


Let's swap the ordering of these imports. For PEP-8, the first group is standard library stuff (like datetime), and the second group is third-party libraries (like click).

bowlofeggs · 2017-07-20T15:46:16Z

bodhi/tests/server/scripts/test_dequeue_stable.py

+        self.db.commit()
+
+        result = runner.invoke(dequeue_stable.dequeue_stable, [])
+        print 'runner result:', result


Let's drop this print statement.

bowlofeggs · 2017-07-20T15:46:27Z

bodhi/tests/server/scripts/test_dequeue_stable.py

+        self.assertEqual(result.exit_code, 0)
+
+        update = self.db.query(models.Update).all()[0]
+        self.assertEqual(update.request, models.UpdateRequest.stable)


bowlofeggs · 2017-07-20T15:47:54Z

bodhi/tests/server/scripts/test_dequeue_stable.py

+        self.db.commit()
+        update = self.db.query(models.Update).all()[0]
+        result = runner.invoke(dequeue_stable.dequeue_stable, [])
+        self.assertEqual(result.exit_code, 1)


You could also assert that result.output (I think it's output, but it might be a different attribute - but basically you can get whatever click wrote to the terminal…) is an expected value.

bowlofeggs · 2017-07-20T15:50:26Z

bodhi/tests/server/services/test_updates.py

@@ -3644,7 +3644,7 @@ def test_autopush_non_critical_update_with_no_negative_karma(self, publish, *arg
        self.assertEquals(up.autokarma, True)

        up = self.db.query(Update).filter_by(title=resp.json['title']).one()
-        self.assertEquals(up.request, UpdateRequest.stable)
+        self.assertEquals(up.request, UpdateRequest.batched)

    @mock.patch(**mock_valid_requirements)
    @mock.patch('bodhi.server.notifications.publish')


Let's add another 2 tests to this module:

One that asserts that security updates skip batched and go straight to stable when getting the right karma.

One that asserts that high severity updates skip batched and go straight to stable when getting the right karma.

I did a quick search and didn't see existing tests like the above, but it's possible I missed them.

bowlofeggs

Hi @crungehottman!

I apologize for missing these logic issues during my earlier reviews - not sure how I didn't notice them before. They should be pretty easy to fix though, let me know if you want me to explain any of them further!

bowlofeggs · 2017-07-24T16:56:25Z

bodhi/server/models.py

@@ -1732,7 +1732,7 @@ def set_request(self, db, action, username):
        # If status is testing going to stable request and action is revoke,
        # keep the status at testing
        elif self.status is UpdateStatus.testing and self.request is UpdateRequest.stable \
-                and action is UpdateRequest.revoke:
+                or UpdateRequest.batched and action is UpdateRequest.revoke:


There are two problems I see in this block. First, with Python's order of operations the and's are evaluated before the or. Thus, the above is logically this:

elif (self.status is UpdateStatus.testing and self.request is UpdateRequest.stable) or (UpdateRequest.batched and action is UpdateRequest.revoke):

Note that we only check the action on batched updates now, where I think the intention would be to also check it on stable updates.

Secondly, UpdateRequest.batched will evaluate as True here - I think the intention was probably to say self.request is UpdateRequest.batched.

This is probably a cleaner way to express the above:

elif self.status is UpdateStatus.testing and (self.request is UpdateRequest.stable or self.request is UpdateRequest.batched) and action is UpdateRequest.revoke:

We need the parenthesis around the or clause so that it evaluates before the and operators.

You could also replace (self.request is UpdateRequest.stable or self.request is UpdateRequest.batched) in my suggestion with self.request in (UpdateRequest.stable, UpdateRequest.batched), eliminating the or and maybe a little shorter too, like this:

elif self.status is UpdateStatus.testing and self.request in (UpdateRequest.stable, UpdateRequest.batched) and action is UpdateRequest.revoke:

bowlofeggs · 2017-07-24T17:02:07Z

bodhi/server/models.py

@@ -1748,7 +1748,7 @@ def set_request(self, db, action, username):
            return

        # Disable pushing critical path updates for pending releases directly to stable
-        if action is UpdateRequest.stable and self.critpath:
+        if action is (UpdateRequest.stable or UpdateRequest.batched) and self.critpath:


The is operator doesn't quite work this way. I believe the above may always evaluate to False because UpdateRequest.stable or UpdateRequest.batched will evaluate to True, and then action is True will evaluate to False.

Probably we want this:

if action in (UpdateRequest.stable, UpdateRequest.batched) and self.critpath:

Or alternatively,

if (action is UpdateRequest.stable or action is UpdateRequest.batched) and self.critpath:

bowlofeggs · 2017-07-24T17:02:47Z

bodhi/server/models.py

@@ -1774,7 +1774,7 @@ def set_request(self, db, action, username):

        # Ensure this update meets the minimum testing requirements
        flash_notes = ''
-        if action is UpdateRequest.stable and not self.critpath:
+        if action is (UpdateRequest.stable or UpdateRequest.batched) and not self.critpath:


Similar problem here.

bowlofeggs · 2017-07-24T17:07:32Z

bodhi/server/models.py

@@ -2571,7 +2576,7 @@ def requested_tag(self):
            # release to the Release.dist-tag
            if self.release.state is ReleaseState.pending:
                tag = self.release.dist_tag
-        elif self.request is UpdateRequest.testing:
+        elif self.request is UpdateRequest.testing or UpdateRequest.batched:


This one also has the problem - the or UpdateRequest.batched will become or True, which will make this if statement always be True. You can use the in trick I described above, or repeat the self.request is on the second clause.

bowlofeggs · 2017-07-24T17:11:16Z

bodhi/tests/server/scripts/test_dequeue_stable.py

+        update.request = models.UpdateRequest.batched
+        self.db.commit()
+        update = self.db.query(models.Update).all()[0]
+        result = runner.invoke(dequeue_stable.dequeue_stable, [])


Here's a stylistic suggestion that you can feel free to ignore - just something I find helpful when reading tests (it's certainly not a broader Python convention or anything, and again, you can keep it this way if you like this better):

I like to break my tests into three blocks usually, each block separated by a space. The first block I do things that set up the test, the second block (often just one line) executes the function/method being tested, and the third block has my assertions. So basically, to do that here you could stick an empty line above and block this runner.invoke() line.

Again, just a suggestion, feel free to leave it this way.

bowlofeggs · 2017-07-24T17:12:19Z

bodhi/tests/server/services/test_updates.py

+        up = self.db.query(Update).filter_by(title=resp.json['title']).one()
+
+        up = self.db.query(Update).filter_by(title=resp.json['title']).one()
+        self.assertEquals(up.request, UpdateRequest.stable)


Nice tests!

bowlofeggs

Very nice Caleigh, this is coming along well!

I spent some time this morning looking for other places we might want to consider the new batched state, and I think I found a couple where we should also consider the new state.

In set_request() I think we should also check requirements on requests for batched.
In validate_request() I think we might want to consider the batched request to also be target = UpdateStatus.stable, because that code is ensuring that people don't promote builds that are older than what is already in the target.

bowlofeggs

I just noticed that we do need to add the dequeue_stable.py script to the setup.py entry_points so that the script gets generated. I think something like this should work:

bodhi-dequeue-stable = bodhi.server.scripts.dequeue_stable:dequeue_stable

bowlofeggs · 2017-07-31T21:23:46Z

bodhi/server/scripts/dequeue_stable.py

+@click.version_option(message='%(version)s')
+def dequeue_stable():
+    """Convert all batched requests to stable requests."""
+    initialize_db(config.config)


We also need to initialize the buildsys here, otherwise the script gets an error:

[vagrant@bodhi-dev bodhi]$ bodhi-dequeue-stable Buildsys needs to be setup

Here's a patch that sets up the buildsys and also sets up the console script entry point:

diff --git a/bodhi/server/scripts/dequeue_stable.py b/bodhi/server/scripts/dequeue_stable.py index 70d1ecd..4ca7d00 100644 --- a/bodhi/server/scripts/dequeue_stable.py +++ b/bodhi/server/scripts/dequeue_stable.py @@ -22,7 +22,7 @@ import sys import click -from bodhi.server import config, models, Session, initialize_db +from bodhi.server import buildsys, config, models, Session, initialize_db @click.command() @@ -30,6 +30,7 @@ from bodhi.server import config, models, Session, initialize_db def dequeue_stable(): """Convert all batched requests to stable requests.""" initialize_db(config.config) + buildsys.setup_buildsystem(config.config) db = Session() try: diff --git a/setup.py b/setup.py index ba739ed..be5180a 100644 --- a/setup.py +++ b/setup.py @@ -154,6 +154,7 @@ setup( initialize_bodhi_db = bodhi.server.scripts.initializedb:main bodhi-babysit-ci = bodhi.server.scripts.babysit_ci:babysit bodhi-clean-old-mashes = bodhi.server.scripts.clean_old_mashes:clean_up + bodhi-dequeue-stable = bodhi.server.scripts.dequeue_stable:dequeue_stable bodhi-push = bodhi.server.push:push bodhi-expire-overrides = bodhi.server.scripts.expire_overrides:main bodhi-untag-branched = bodhi.server.scripts.untag_branched:main

With that patch I was able to see an update get to batched by me giving it karma, and then I was able to see it get to stable with the script. Very cool!

TC01 · 2017-08-01T03:03:35Z

bodhi/server/models.py

@@ -2571,7 +2576,7 @@ def requested_tag(self):
            # release to the Release.dist-tag
            if self.release.state is ReleaseState.pending:
                tag = self.release.dist_tag
-        elif self.request is UpdateRequest.testing:
+        elif self.request in (UpdateRequest.testing, UpdateRequest.batched):


(I made this comment on the fedora devel list and was told to post it here).

It would be nice if new package updates were also sent directly to stable with something like or self.type is newpackage-- there isn't a compelling reason to batch new package updates (since they won't automatically get installed on an end-user system) and this way they don't get artificially made any slower than they already are, ensuring brand-new packages still get to users in a timely manner.

I think this suggestion makes sense. @crungehottman, if you can add one more if statement and one more test for new packages, that would be great!

mattdm · 2017-08-01T20:27:03Z

Many security updates are really of low priority, as they happen only in unlikely configurations or have extremely minor consequences. I think we want to batch these too. Given that the word "urgent" means what it does, it makes most sense to me to batch all updates (security or otherwise) unless marked urgent.

However, I'm very open to being convinced that we should include "important" security updates too. (One point in favor of this: GNOME Software's security update notifier claims that "important updates" are pending when there is a security update available.)

Additionally — and possibly a whole separate issue — I think we should force Severity to "unspecified" for New Package and Enhancement updates. Because, as Randy says, "This newpackage update is urgently severe! Have some severe new features!" is not really the... intent.

re fedora-infra#1157

bowlofeggs

Cool, this looks great! I only found one minor thing to fix, and I also suggest writing one more test to ensure the "push to stable" button appears when an update is already batched.

I don't want to hold up this PR any longer for those two little changes so I'll go ahead and merge this. Can you send two new PRs for the minor fix and the one extra test?

Thanks, this is very good work @crungehottman!

bowlofeggs · 2017-08-11T18:02:02Z

bodhi/server/models.py

-                log.info("Automatically marking %s as stable" % self.title)
-                self.set_request(db, UpdateRequest.stable, agent)
-                self.request = UpdateRequest.stable
+                if self.severity is UpdateSeverity.urgent or self.type is UpdateType.newpackage:


Excellent, I think this addresses the feedback from the devel list.

bowlofeggs · 2017-08-11T18:05:17Z

bodhi/tests/server/services/test_updates.py

+    @mock.patch('bodhi.server.notifications.publish')
+    def test_newpackage_update_bypass_batched(self, publish, *args):
+        """
+        Make sure a security update skips the 'batched' request and immediately enters stable


We should correct this docblock to say "new package update" instead of security.

bowlofeggs suggested changes Jul 11, 2017

View reviewed changes

crungehottman force-pushed the batched-update-request-flow branch from 44e5d19 to f09377f Compare July 11, 2017 16:21

crungehottman force-pushed the batched-update-request-flow branch 2 times, most recently from fa91e6b to e7dd767 Compare July 20, 2017 01:37

bowlofeggs suggested changes Jul 20, 2017

View reviewed changes

crungehottman force-pushed the batched-update-request-flow branch from e7dd767 to 9d0ab82 Compare July 20, 2017 18:58

bowlofeggs suggested changes Jul 24, 2017

View reviewed changes

crungehottman force-pushed the batched-update-request-flow branch from 9d0ab82 to 7c48a9f Compare July 24, 2017 19:41

bowlofeggs suggested changes Jul 28, 2017

View reviewed changes

jeremycline mentioned this pull request Jul 31, 2017

Refactor the update state machine #1723

Open

crungehottman force-pushed the batched-update-request-flow branch 2 times, most recently from 7e1a927 to 5255f1b Compare July 31, 2017 20:23

bowlofeggs suggested changes Jul 31, 2017

View reviewed changes

crungehottman force-pushed the batched-update-request-flow branch from 5255f1b to ca4ad99 Compare July 31, 2017 22:10

TC01 reviewed Aug 1, 2017

View reviewed changes

Add script and tests for moving a batched request to a stable request

2024bd5

re fedora-infra#1157

crungehottman force-pushed the batched-update-request-flow branch from ca4ad99 to 2024bd5 Compare August 3, 2017 00:25

add web ui option to push to batched

e6a4dab

bowlofeggs approved these changes Aug 11, 2017

View reviewed changes

bowlofeggs merged commit d06a28a into fedora-infra:develop Aug 11, 2017

This was referenced Aug 12, 2017

Add additional test for push to stable button #1748

Merged

Correct docblock to read "newpackage update" instead of "security upd… #1749

Merged

		@@ -0,0 +1,40 @@
		# This program is free software; you can redistribute it and/or

		@@ -0,0 +1,59 @@
		# This program is free software; you can redistribute it and/or

[WIP] Add script and tests for moving a batched request to a stable request #1678

[WIP] Add script and tests for moving a batched request to a stable request #1678

Conversation

crungehottman commented Jul 11, 2017 • edited

centos-ci commented Jul 11, 2017

bowlofeggs commented Jul 11, 2017

bowlofeggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

centos-ci commented Jul 11, 2017

bowlofeggs commented Jul 11, 2017

bowlofeggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowlofeggs left a comment

Choose a reason for hiding this comment

bowlofeggs Jul 24, 2017 • edited

Choose a reason for hiding this comment

bowlofeggs Jul 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowlofeggs left a comment • edited

Choose a reason for hiding this comment

bowlofeggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TC01 Aug 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattdm commented Aug 1, 2017

bowlofeggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crungehottman commented Jul 11, 2017 •

edited

bowlofeggs Jul 24, 2017 •

edited

bowlofeggs Jul 24, 2017 •

edited

bowlofeggs left a comment •

edited

TC01 Aug 1, 2017 •

edited