#50 Ondemand task scheduling based on testcase
Closed 5 years ago by jskladan. Opened 5 years ago by jskladan.

file modified
+1
@@ -18,6 +18,7 @@ 

  

  # pytest cache

  /testing/.cache

+ .pytest_cache

  

  # virtualenv

  /env_trigger/

@@ -63,3 +63,8 @@ 

      message_type: GitHubPullRequestOpened

    do:

      - {tasks: [task-mtf]}

+ - when:

Nitpick - add an empty line above? :-)

Yaay, master of nitpicks! Absolutely though

+     message_type: TaskotronResultMissing

+     testcase: {$regex: '/^dist.rpmgrill(\..+)*/'}

Would it be more readable if you replaced * with ? ? It looks more obvious to me.

I guess. Incidentaly, in the way I wrote the regexp, it ends up doing the same, although it was not the initial intention. My original intent was '/^dist.rpmgrill(\.[^.]+)*/'.
Anyway, this is just an example config. I could absolutely change it, don't really care about it. Nice find, though!

+   do:

+     - {tasks: [rpmgrill], arches: ${arches}}

Can you please explain where arches get used and how?

arches or ${arches}?
arches is used in the trigger code to overload the config.valid_arches. Aka, if you wanted the rpmgrill task to be executed only on i386 you'd add the arches: "i386" parameter to the tasks dict.
ad ${arches} - all the ${} replacements, are populated from the data extracted from the received fedmsg (see ${repo}, ${distgit_branch} or ${critpath_pkgs} for other examples of the same concept).

The trick here (with TaskotronResultMissing type messages) is, that instead of hard-coding which arches are to be executed, you allow the fedmsg to contain an arch key-valu in their body to explicitly ask for a rerun of (say rpmlint) for that one arch (see the taskotron_result_missing_msg.py).

@@ -6,4 +6,5 @@ 

      'taskotron.cloudcomposecompletedconsumer.enabled': True,

      'taskotron.modulebuilddonejobconsumer.enabled': True,

      'taskotron.githubpropenedjobconsumer.enabled': True,

+     'taskotron.taskotronresultmissingjobconsumer.enabled': True,

  }

@@ -92,6 +92,9 @@ 

              else:

                  self.log.error("Rule has invalid `do` section: %r", rule)

  

+             if 'arches' in item:

+                 task['arches'] = item['arches']

+ 

              tasks.append(task)

  

          return tasks

@@ -79,6 +79,12 @@ 

                  val = mapping[named]

                  # We use this idiom instead of str() because the latter will

                  # fail if val is a Unicode containing non-ASCII characters.

+                 # FIXME - list, tuple and dict value handling hotfixed and not properly tested

I don't really understand the purpose of this code. Do you intend to add test suite coverage before merging this, or keep this comment in and do it some other time?

The 'problem' here is, that the original code (return '%s' % (val,)) worked fine for most of the stuff, but if you try applying that on [u"unicode"] for example, the output value is a string like this: '[u"unicode"]' which is, sadly, not a valid yaml list.
They also specifically call this out in the comments above the FIXME to some level.
This change makes sure that for the iterables, "%s" is called on each element (key, value..) to get rid of the potential u prefix.

This is important when we want to be able to do ${variable} replacements in the trigger template with lists. And although we already do that with the list of blacklisted packages for abicheck. We always only ever provided a list of non-unicode strings to the method, which gets "%s"-ed "properly".

I don't plan on writing a test for this before merge though, ATM.

+                 if isinstance(val, list) or isinstance(val, tuple):

+                     return "[%s]" % ",".join(val)

+                 if isinstance(val, dict):

+                     items = ["%s: %s" % (k, v) for k, v in val.iteritems()]

+                     return "{%s}" % ",".join(items)

                  return '%s' % (val,)

              if mo.group('escaped') is not None:

                  return self.delimiter

@@ -0,0 +1,45 @@ 

+ import fedmsg.consumers

+ 

+ from . import config

+ from .jobtrigger import JobTrigger

+ from . import exceptions as exc

+ 

+ MESSAGE_TYPE = 'TaskotronResultMissing'

+ 

+ class TaskotronResultMissingJobTrigger(JobTrigger):

+     def process(self, msg):

+         item = msg['msg']['item']

+         item_type = msg['msg']['item_type']

+         testcase = msg['msg']['testcase']

+         if 'arch' in msg['msg']:

+             arches = [msg['msg']['arch']]

+         else:

+             arches = config.valid_arches

This means the missing result fedmsg can either ask for a particular arch, or otherwise all supported arches are triggered, right? That's great.

Precisely

+ 

+         data = {

+             "_msg": msg,

+             "message_type": MESSAGE_TYPE,

+             "item": item,

+             "item_type": item_type,

+             "testcase": testcase,

+             "arches": arches,

+         }

+         return data

+ 

+ 

+ class TasktoronResultMissingJobConsumer(fedmsg.consumers.FedmsgConsumer):

+     topic = "org.fedoraproject.%s.taskotron.result_missing" % config.deployment_type

+     config_key = 'taskotron.taskotronresultmissingjobconsumer.enabled'

+ 

+     def __init__(self, *args, **kw):

+         super(TasktoronResultMissingJobConsumer, self).__init__(*args, **kw)

+         self.trigger = TaskotronResultMissingJobTrigger(self.log)

+ 

+     def consume(self, message):

+         msg = fedmsg.encoding.loads(message.body)

+         try:

+             data = self.trigger.process(msg)

+             self.trigger.do_trigger(data)

+         except exc.TriggerMsgError, e:

+             self.log.debug(e)

+             return

file modified
+1
@@ -11,6 +11,7 @@ 

  mongoquery

  requests

  Twisted

+ koji

Does this have any relevance to this patch? Koji has been added into pypi recently, but I don't know how well it works, have you tested it? In readme, we still require it to be installed from an rpm.

Well, I have a virtualenv that does not accept external libs (because of coverage/pytest issues). Tested it with koji from pypi, and it works OK, so I decided to keep it in. Can absolutely get rid of it, if you deem it better/cleaner. /me does not care that much.

If it worked for you, keep it in. Just perhaps just make it a separate commit, thanks.

roger

  PyYAML >= 3.11

  

  # Test suite requirements

@@ -81,6 +81,17 @@ 

          assert tasks[0]['item_type'] == self.ref_message_data['item_type']

          assert tasks[0]['arches'] == self.ref_default_arches

  

+ 

+     def test__tasks_from_rule_with_arches(self):

+         rule = {'do': [{'tasks': self.ref_tasks, 'arches': ['bogusarch']}]}

+         tasks = self.helper._tasks_from_rule(rule, self.ref_message_data)

+         assert len(tasks) == 1

+         assert tasks[0]['tasks'] == self.ref_tasks

+         assert tasks[0]['item'] == self.ref_message_data['item']

+         assert tasks[0]['item_type'] == self.ref_message_data['item_type']

+         assert tasks[0]['arches'] == ['bogusarch']

+ 

+ 

      def test__tasks_from_rule_discover(self):

          ref_repo = 'http://bogus.repo/firefox.git'

          rule = {'do': [{'discover': {'repo': ref_repo}}]}
@@ -140,6 +151,34 @@ 

          assert runner_calls[3][1] == (

              self.ref_item, self.ref_item_type, self.ref_tasks[1], self.ref_default_arches[1])

  

+     def test_do_trigger_runtasks_with_arches(self, monkeypatch):

+         ref_arches = ['arch_foo', 'arch_bar']

+         mock__load_rules = mock.Mock(return_value=[

+                 {

+                     'do': [{'tasks': self.ref_tasks, 'arches': ref_arches}],

+                     'when': {'message_type': 'RunTasks'}

+                 },

+             ])

+         monkeypatch.setattr(self.helper, '_load_rules', mock__load_rules)

+ 

+         message_data = copy.deepcopy(self.ref_message_data)

+         message_data['message_type'] = 'RunTasks'

+ 

+         self.helper.do_trigger(message_data)

+ 

+         runner_calls = self.helper.runner.trigger_job.calls()

+ 

+         assert len(runner_calls) == 4

+ 

+         assert runner_calls[0][1] == (

+             self.ref_item, self.ref_item_type, self.ref_tasks[0], ref_arches[0])

+         assert runner_calls[1][1] == (

+             self.ref_item, self.ref_item_type, self.ref_tasks[0], ref_arches[1])

+         assert runner_calls[2][1] == (

+             self.ref_item, self.ref_item_type, self.ref_tasks[1], ref_arches[0])

+         assert runner_calls[3][1] == (

+             self.ref_item, self.ref_item_type, self.ref_tasks[1], ref_arches[1])

+ 

      def test_do_trigger_discover(self, monkeypatch):

          mock__load_rules = mock.Mock(return_value=self.ref_rules)

          monkeypatch.setattr(self.helper, '_load_rules', mock__load_rules)

@@ -0,0 +1,68 @@ 

+ import pytest

+ from dingus import Dingus

+ from munch import Munch

+ from copy import deepcopy

+ import fedmsg.encoding

+ import fedmsg.consumers

+ 

+ from jobtriggers import taskotron_result_missing_msg

+ 

+ 

+ @pytest.mark.usefixtures('prepare')

+ class TestTaskotronResultMissingJobConsumer():

+ 

+     @pytest.fixture

+     def prepare(self, monkeypatch):

+         self.ref_item = 'ref_item'

+         self.ref_item_type = 'ref_item_type'

+         self.ref_testcase = 'ref.testcase.subcase'

+         self.ref_arch = 'ref_arch'

+         self.ref_task = 'ref_task'

+         self.ref_validarches = ['i386', 'x86_64']

+ 

+         self._create_msg(

+             self.ref_item, self.ref_item_type, self.ref_testcase, self.ref_arch)

+ 

+         self.ref_data = {

+             "_msg": {},

+             "message_type": "TaskotronResultMissing",

+             "item": self.ref_item,

+             "item_type": self.ref_item_type,

+             "testcase": self.ref_testcase,

+             "arches": [self.ref_arch],

+         }

+ 

+         stub_hub = Munch(config=Munch(get=0))

+         self.helper = taskotron_result_missing_msg.TasktoronResultMissingJobConsumer(stub_hub)

+ 

+         self.helper.trigger.runner = Dingus()

+         taskotron_result_missing_msg.config.trigger_rules_template = """---

+ - do:

+   - {tasks: [ref_task], arches: ${arches}}

+   when: {message_type: TaskotronResultMissing, testcase: {$regex: '/^ref.testcase(\..+)*/'}}

+ """

+ 

+         taskotron_result_missing_msg.config.valid_arches = self.ref_validarches

+         taskotron_result_missing_msg.config.job_logging = False

+ 

+     def _create_msg(self, ref_item, ref_item_type, ref_testcase, ref_arch):

+         self.ref_message = Munch(body='{"i": 1,\

+                                         "msg": {"item": "%s",\

+                                                 "item_type": "%s",\

+                                                 "testcase": "%s",\

+                                                 "arch": "%s"},\

+                                         "timestamp": 1359603469.21164,\

+                                         "topic": "org.fedoraproject.prod.taskotron.result_missing",\

+                                         "username": "apache"}' %

+                                  (ref_item, ref_item_type, ref_testcase, ref_arch))

+ 

+     def test_consume(self):

+         self.helper.consume(self.ref_message)

+ 

+         runner_calls = self.helper.trigger.runner.trigger_job.calls()

+ 

+         assert len(runner_calls) == 1

+ 

+         assert runner_calls[0][1] == (self.ref_item, self.ref_item_type,

+                                       self.ref_task, self.ref_arch)

+ 

Required (or at least quality of life) change for (rawhide) gating. Trigger can now consume messages with testcase, item, item_type and arch.

The fedmsg format/name/details will need to be fixed once we have the actual format, but the code stands as is.

Would it be more readable if you replaced * with ? ? It looks more obvious to me.

Nitpick - add an empty line above? :-)

I don't really understand the purpose of this code. Do you intend to add test suite coverage before merging this, or keep this comment in and do it some other time?

Yaay, master of nitpicks! Absolutely though

Can you please explain where arches get used and how?

I guess. Incidentaly, in the way I wrote the regexp, it ends up doing the same, although it was not the initial intention. My original intent was '/^dist.rpmgrill(\.[^.]+)*/'.
Anyway, this is just an example config. I could absolutely change it, don't really care about it. Nice find, though!

This means the missing result fedmsg can either ask for a particular arch, or otherwise all supported arches are triggered, right? That's great.

Does this have any relevance to this patch? Koji has been added into pypi recently, but I don't know how well it works, have you tested it? In readme, we still require it to be installed from an rpm.

Overall looks good to me, once we clarify some details. I wonder, could you add a trivial document into docs/ describing how to retrigger a task? We could then send a link to Fedora Infra folks and ask them to implement it into Bodhi or Greenwave, and it would be helpful for them to see a required fedmsg structure as a reference. Or we can ask them to suggest modifications to the structure, if needed.

The 'problem' here is, that the original code (return '%s' % (val,)) worked fine for most of the stuff, but if you try applying that on [u"unicode"] for example, the output value is a string like this: '[u"unicode"]' which is, sadly, not a valid yaml list.
They also specifically call this out in the comments above the FIXME to some level.
This change makes sure that for the iterables, "%s" is called on each element (key, value..) to get rid of the potential u prefix.

This is important when we want to be able to do ${variable} replacements in the trigger template with lists. And although we already do that with the list of blacklisted packages for abicheck. We always only ever provided a list of non-unicode strings to the method, which gets "%s"-ed "properly".

I don't plan on writing a test for this before merge though, ATM.

Well, I have a virtualenv that does not accept external libs (because of coverage/pytest issues). Tested it with koji from pypi, and it works OK, so I decided to keep it in. Can absolutely get rid of it, if you deem it better/cleaner. /me does not care that much.

If it worked for you, keep it in. Just perhaps just make it a separate commit, thanks.

Overall looks good to me ...

Nothing against it, but this is mostly a placeholder for the moment. I don't really care for the name of the topic, or precise structure of the fedmsg. It just needs to have the right fields, and then we'll change the consumer code based on that.

I'm not sure what the process behind proposing new fedmessages is, and whether it could be plain "taskotron.result_missing" style that anybody could just fire, or if each respective system (bodhi/greenwave) would have it's own topic, and we would have to consume those.

@ralph @pingou - would you be able to shine some light on this?

This whole patch is a working (and tested), but still in the PoC territory, IMO. When writing this, I expected changes once we hit the reality of the fedmessages being emitted, but all it needs (then) is a change of the one consumer file.

@kparal Makes sense?

arches or ${arches}?
arches is used in the trigger code to overload the config.valid_arches. Aka, if you wanted the rpmgrill task to be executed only on i386 you'd add the arches: "i386" parameter to the tasks dict.
ad ${arches} - all the ${} replacements, are populated from the data extracted from the received fedmsg (see ${repo}, ${distgit_branch} or ${critpath_pkgs} for other examples of the same concept).

The trick here (with TaskotronResultMissing type messages) is, that instead of hard-coding which arches are to be executed, you allow the fedmsg to contain an arch key-valu in their body to explicitly ask for a rerun of (say rpmlint) for that one arch (see the taskotron_result_missing_msg.py).

I'm not sure what the process behind proposing new fedmessages is, and whether it could be plain "taskotron.result_missing" style that anybody could just fire, or if each respective system (bodhi/greenwave) would have it's own topic, and we would have to consume those.

We already have taskotron.result.new, so taskotron.results.missing seems like an obvious choice. This fedmsg can then be triggered by anyone on the bus, I believe.

This whole patch is a working (and tested), but still in the PoC territory, IMO. When writing this, I expected changes once we hit the reality of the fedmessages being emitted, but all it needs (then) is a change of the one consumer file.

We can either wait until the fedmsg structure is clarified with all the parties, or commit this, send it as an existing proposal, and make changes in future commits, if needed. I think the latter would be fine.

The idea is that RATS will be the tool sending these messages, so the message topic and structure are still to be defined, giving us quite some flexibility :)

@pingou - yeah. I guess I should have been clearer in what I ask, so:

Can any application on the bus send any fedmsg topic message, or is it just one app per topic?

thx!

Can any application on the bus send any fedmsg topic message, or is it just one app per topic?

Part of the topic is hardcoded, it's normally something like:
org.fedoraproject.<environment>.<service>.... (the rest of the topic being free-form).

Where environment is dev|stg|prod and service the name of the app, so rats would send something like:
org.fedoraproject.<environment>.rats...

Note that there is no technical reasons for this, it is all a standard we decided on in order to have some consistency between applications regarding messages.

Pull-Request has been closed by jskladan

5 years ago