Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure the consumers to use fedora_messaging. #2957

Merged

Conversation

@bowlofeggs
Copy link
Member

commented Jan 23, 2019

This is needed as part of #2838.

Signed-off-by: Randy Barlow randy@electronsweatshop.com

@bowlofeggs bowlofeggs requested a review from fedora-infra/bodhi as a code owner Jan 23, 2019

with self.assertRaises(ValueError) as exc:
Masher(FakeHub(), db_factory=self.db_factory, mash_dir=self.tempdir)

self.assertEqual(

This comment has been minimized.

Copy link
@sebwoj

sebwoj Jan 24, 2019

Collaborator

Lets move this assertion outside "with" statement. Here, it will be skipped by test.

@abompard

This comment has been minimized.

Copy link
Member

commented Feb 5, 2019

My attempt to add Celery to Bodhi will modify the tasks/masher.py and tasks/updates.py files too. Should I wait for this PR to land or should we do it the other way around? Once Celery is in, using a fedora-messaging listener for masher and updates will be obsolete, since they're only distributed tasks actually. But you're more advanced than I am, so there's a timing question too.

What do you prefer?

@bowlofeggs

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

@abompard

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

Oh, you want to bring fedora-messaging support to the 3.x branch? In that case then yeah let's finish that PR first, and add Celery later.

@bowlofeggs

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

@abompard well, we may want to do that. 3.13.0 does have the ability to send, but not to receive, AMQP. I think it's up to us to decide whether we want to make a 3.14 or just wait for 4.0.

Note: This should be folded into __call__() when support for fedmsg is removed. It is
separate only because Masher.consume() receives a Munch object and fedora_messaging
receives a Message object and so this gives us a nice way to hand a Mapping from both.

This comment has been minimized.

Copy link
@abompard

abompard Feb 8, 2019

Member

One thing to remember with fedora-messaging, is that duplicate messages may happen. Therefore I would recommend consuming classes to write explicitely in their documentation how they plan to handle duplicates. It may be as simple as "in case of duplicates, the task will be run again", but for more expensive tasks we could be looking at timestamps, use a locking system (and avoid deadlocks), etc.
It's one thing to keep in mind, and I think each consumers should document how they plan to handle it so we're sure it's not overlooked.

}
The message can contain additional keys.

This comment has been minimized.

Copy link
@abompard

abompard Feb 8, 2019

Member

Same thing here, we should document how duplicate messages are handled.

@abompard

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

A couple of other PRs are adding FM consumers so I think we should finish this one to standardize on the proper way to write FM consumers in Bodhi before adding new ones.

@bowlofeggs bowlofeggs added this to To do in CI Gating via automation Feb 15, 2019

@bowlofeggs bowlofeggs added this to In progress in Release Bodhi 4.0.0 via automation Feb 15, 2019

@bowlofeggs bowlofeggs moved this from To do to Minimum Viable Product in CI Gating Feb 15, 2019

@abompard

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

@bowlofeggs do you need help with that PR?

@bowlofeggs

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

@abompard You can take it if you really want to - I've just been swamped dealing with Greenwave and other things. I will eventually get to it if I can find some time…

@abompard abompard self-assigned this Mar 5, 2019

@cverna cverna force-pushed the bowlofeggs:fedora-messaging-consumers branch from d1416dc to 98db3f2 Mar 6, 2019

@cverna

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

@abompard, @bowlofeggs if you want to give a look at my commit, I think what is left to do is

  • Add fedora-messaging config file and systemd service file that run fedora-messaging consume

  • Decide how we handle possible duplicated message ?

  • Most likely something else ?

Also I am not sure why the CI does not trigger

@cverna

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

jenkies test

CI Gating automation moved this from Minimum Viable Product to Needs review Mar 7, 2019

Release Bodhi 4.0.0 automation moved this from In progress to Needs review Mar 7, 2019

bodhi/server/consumers/__init__.py Outdated Show resolved Hide resolved
threads for each repo tag being mashed.
If there are any security updates in the push, then those repositories
will be executed before all others.

This comment has been minimized.

Copy link
@abompard

abompard Mar 7, 2019

Member

We should document how duplicate messages are handled.

'%s' % pprint.pformat(self.topic))

def consume(self, message):
def __call__(self, message: fedora_messaging.api.Message):
"""
Process the given message, updating relevant bugs and test cases.

This comment has been minimized.

Copy link
@abompard

abompard Mar 7, 2019

Member

Same here, we should document how duplicate messages are handled.

body={}
)
messaging_callback(msg)
handler.assert_called_once_with()

This comment has been minimized.

Copy link
@abompard

abompard Mar 7, 2019

Member

You're only checking that the correct class has been instantiated, not that it has been called with the message as argument. We should probably test that but it's not extremely important since they are all called the same way.

is created and used.
mash_dir (basestring): The directory in which to place mashes.
Raises:
ValueError: If pungi.cmd is set to a path that does not exist.

This comment has been minimized.

Copy link
@abompard

abompard Mar 7, 2019

Member

It may be worthwhile to keep this docstring (minus the hub).

bodhi/server/consumers/signed.py Outdated Show resolved Hide resolved
@abompard

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

Add fedora-messaging config file and systemd service file that run fedora-messaging consume

For the systemd service file, fedora-messaging already provides one as a service template. You just need to drop the configuration file in /etc/fedora-messaging/bodhi.toml and enable the fm-consumer@bodhi.service service and it should work.

Decide how we handle possible duplicated message ?

I don't know the inner workings of Bodhi well enough to answer that, I suspect that we want to avoid running two long-running tasks at the same time if we receive duplicate messages, but we may want to do something else. @bowlofeggs do you have an opinion on that?

@cverna cverna force-pushed the bowlofeggs:fedora-messaging-consumers branch 3 times, most recently from 31942ee to 2d6f81a Mar 7, 2019

@abompard

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Now that Python2 support has been dropped, there are a few places where we could make the code a bit simpler (like classes don't have to inherit from object anymore, super() can be called without arguments, etc...) but I can make a PR for that later, I understand Clément's pain of dealing with merge conflicts ;-)

@abompard

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

It looks OK to me, but there are two issues I'd like to address, and I need info from @bowlofeggs for that:

  • are the actions triggered by these messages idempotent? With AMQP, it's possible to receive duplicate messages.
  • can the actions triggered by these messages last more than 50 seconds? The message callback is run in the main thread, so the client can't heartbeat to the server while the action is running (see FM issue #130. If there are no long-lasting actions it's not a problem, but if there are we must think of a way to handle that. In our case, I think that the best solution will be to pass the action to a task scheduling system like celery, but that's the subject of another PR, so in the meantime we can leave it as-is: the client will get disconnected, will reconnect and get the missing messages, no big deal.
@abompard

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Those two issues have been logged as #3069 and #3070, so they're not blocking this one.

@cverna cverna force-pushed the bowlofeggs:fedora-messaging-consumers branch 3 times, most recently from 614607f to 9ebb702 Mar 18, 2019

@cverna

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Ok I have most of the tests passing now, not sure why the 3 others tests are failing.

@abompard

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

One is pydocstyle being unhappy that bodhi/server/consumers/signed.py:44's __init__ method has no docstring, the other is a timeout waiting for PostgreSQL to start up (you can ignore that one).

@bowlofeggs

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

@abompard

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

I don't think it has it, the timeout was 30s IIRC.

Replace fedmsg by fedora-messaging in consumers.
This commit drops support of fedmsg for the consumers. It introduce a callback
function that will be called by the fedora-messaging consume command. This callback
then route the message to the correct consumer based on the message topic.

Co-authored-by: Randy Barlow <randy@electronsweatshop.com>
Co-authored-by: Clement Verna <cverna@tutanota.com>

Signed-off-by: Clement Verna <cverna@tutanota.com>

@cverna cverna force-pushed the bowlofeggs:fedora-messaging-consumers branch from 9ebb702 to bac1a57 Mar 19, 2019

@cverna

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

jenkies test

@abompard

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

The integration tests were "Killed by signal 15", I don't know what happened...

@cverna

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

Ok so locally all the integration tests pass except for rawhide where starting the container seems to fail.

See https://paste.fedoraproject.org/paste/y3uIUqe2Dh-zpztvO-Berw

@abompard abompard changed the title [WIP] Configure the consumers to use fedora_messaging. Configure the consumers to use fedora_messaging. Mar 19, 2019

@abompard
Copy link
Member

left a comment

OK, looks good, we'll address the issues of duplicate messages in following PRs

Release Bodhi 4.0.0 automation moved this from Needs review to Reviewer approved Mar 19, 2019

@abompard abompard merged commit 657e20a into fedora-infra:develop Mar 19, 2019

38 of 44 checks passed

continuous-integration/jenkins/pr-head This commit cannot be built
Details
f28-integration
Details
f29-integration
Details
f30-integration
Details
pip-integration
Details
rawhide-integration
Details
DCO DCO
Details
Mergify — Summary 4 potential rules
Details
f28-build
Details
f28-diff-cover
Details
f28-docs
Details
f28-flake8
Details
f28-integration-build
Details
f28-pydocstyle
Details
f28-unit
Details
f29-build
Details
f29-diff-cover
Details
f29-docs
Details
f29-flake8
Details
f29-integration-build
Details
f29-pydocstyle
Details
f29-unit
Details
f30-build
Details
f30-diff-cover
Details
f30-docs
Details
f30-flake8
Details
f30-integration-build
Details
f30-pydocstyle
Details
f30-unit
Details
pip-build
Details
pip-diff-cover
Details
pip-docs
Details
pip-flake8
Details
pip-integration-build
Details
pip-mypy
Details
pip-pydocstyle
Details
pip-unit
Details
rawhide-build
Details
rawhide-diff-cover
Details
rawhide-docs
Details
rawhide-flake8
Details
rawhide-integration-build
Details
rawhide-pydocstyle
Details
rawhide-unit
Details

CI Gating automation moved this from Needs review to Merged to develop Mar 19, 2019

Release Bodhi 4.0.0 automation moved this from Reviewer approved to Done Mar 19, 2019

@bowlofeggs

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

@bowlofeggs

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

@bowlofeggs bowlofeggs deleted the bowlofeggs:fedora-messaging-consumers branch Mar 19, 2019

cverna added a commit to cverna/bodhi that referenced this pull request Mar 19, 2019
Merge pull request fedora-infra#2957 from bowlofeggs/fedora-messaging…
…-consumers

Configure the consumers to use fedora_messaging.
@bowlofeggs

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

This patch is planned for inclusion in the upcoming 4.0.0 release: #3221

@bowlofeggs

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

@cverna cverna moved this from Merged to develop to Deployed to production in CI Gating May 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.