#1251 Add greenwave query to done handler
Merged 4 months ago by jkaluza. Opened 4 months ago by vmaljulin.
vmaljulin/fm-orchestrator FACTORY-4007  into  master

@@ -37,6 +37,7 @@ 

  )

  from module_build_service.errors import UnprocessableEntity, Forbidden, ValidationError

  from module_build_service.utils.ursine import handle_stream_collision_modules

+ from module_build_service.utils.greenwave import greenwave

  

  from requests.exceptions import ConnectionError

  from module_build_service.utils import mmd_to_str

@@ -131,10 +132,11 @@ 

          # This is ok.. it's a race condition we can ignore.

          pass

  

-     # Scratch builds stay in 'done' state, otherwise move to 'ready'

+     # Scratch builds stay in 'done' state

      if not build.scratch:

-         build.transition(config, state="ready")

-         session.commit()

+         if greenwave is None or greenwave.check_gating(build):

+             build.transition(config, state="ready")

+             session.commit()

  

      build_logs.stop(build)

      module_build_service.builder.GenericBuilder.clear_cache(build)

@@ -24,6 +24,7 @@ 

  

  import requests

  import json

+ from functools import reduce

  from module_build_service import log, conf

  from module_build_service.errors import GreenwaveError

  

@@ -34,50 +35,51 @@ 

          Initialize greenwave instance with config

          """

          self.url = conf.greenwave_url

-         if not self.url:

-             raise GreenwaveError("No Greenwave URL set")

          self._decision_context = conf.greenwave_decision_context

          if not self.decision_context:

              raise GreenwaveError("No Greenwave decision context set")

          self._subj_type = conf.greenwave_subject_type

          self._gw_timeout = conf.greenwave_timeout

  

-     def query_decision(self, build, prod_version):

+     def _greenwave_query(self, query_type, payload=None):

          """

-         Query decision to greenwave

-         :param build: build object

-         :type build: module_build_service.models.ModuleBuild

-         :param prod_version: The product version string used for querying WaiverDB

-         :type prod_version: str

+         Make a query to greenwave

+         :param query_type: will be part of url

+         :type query_type: str

+         :param payload: request payload used in 'decision' query

+         :type payload: str

          :return: response

          :rtype: dict

          """

-         payload = {

-             "decision_context": self.decision_context,

-             "product_version": prod_version,

-             "subject_type": self.subject_type,

-             "subject_identifier": build.nvr_string

-         }

-         url = "{0}/decision".format(self.url)

-         headers = {"Content-Type": "application/json"}

+         query_func = requests.post if payload else requests.get

+         kwargs = {"url": "{0}/{1}".format(self.url, query_type), "timeout": self.timeout}

+ 

+         if payload:

+             kwargs["headers"] = {"Content-Type": "application/json"}

+             kwargs["data"] = payload

+ 

          try:

-             response = requests.post(

-                 url=url, headers=headers, data=json.dumps(payload), timeout=self.timeout)

+             response = query_func(**kwargs)

          except requests.exceptions.Timeout:

              raise GreenwaveError("Greenwave request timed out")

          except Exception as exc:

-             log.exception(str(exc))

-             raise GreenwaveError("Greenwave request error")

+             error_message = "Unspecified greenwave request error" \

+                             '(original exception was: "{0}")'.format(str(exc))

+             log.exception(error_message)

+             raise GreenwaveError(error_message)

  

          try:

              resp_json = response.json()

          except ValueError:

              log.debug("Greenwave response content (status {0}): {1}".format(

-                 response.status_code, response.text))

+                 response.status_code, response.text

+             ))

              raise GreenwaveError("Greenwave returned invalid JSON.")

  

-         log.debug('Query to Greenwave result: status=%d, content="%s"',

-                   (response.status_code, resp_json))

+         log.debug(

+             'Query to Greenwave (%s) result: status=%d, content="%s"',

+             (kwargs["url"], response.status_code, resp_json)

+         )

  

          if response.status_code == 200:

              return resp_json

@@ -87,7 +89,88 @@ 

          except KeyError:

              err_msg = response.text

          raise GreenwaveError("Greenwave returned {0} status code. Message: {1}".format(

-             response.status_code, err_msg))

+             response.status_code, err_msg

+         ))

+ 

+     def query_decision(self, build, prod_version):

+         """

+         Query decision to greenwave

+         :param build: build object

+         :type build: module_build_service.models.ModuleBuild

+         :param prod_version: The product version string used for querying WaiverDB

+         :type prod_version: str

+         :return: response

+         :rtype: dict

+         """

+         payload = {

+             "decision_context": self.decision_context,

+             "product_version": prod_version,

+             "subject_type": self.subject_type,

+             "subject_identifier": build.nvr_string

+         }

+         return self._greenwave_query('decision', json.dumps(payload))

+ 

+     def query_policies(self, return_all=False):

+         """

+         Query policies to greenwave

+         :param return_all: Return all policies, if False select by subject_type and decision_context

+         :type return_all: bool

+         :return: response

+         :rtype: dict

+         """

+         response = self._greenwave_query('policies')

+ 

+         if return_all:

+             return response

+ 

+         try:

+             selective_resp = {

+                 "policies": [

+                     pol for pol in response["policies"]

+                     if pol["decision_context"] == self.decision_context

+                     and pol["subject_type"] == self.subject_type

+                 ]

+             }

+         except KeyError:

+             log.exception("Incorrect greenwave response (Mandatory key is missing)")

+             raise GreenwaveError("Incorrect greenwave response (Mandatory key is missing)")

+         return selective_resp

+ 

+     def get_product_versions(self):

+         """

+         Return a set of product versions according to decision_context and subject_type

+         :return: product versions

+         :rtype: set

+         """

+         return reduce(

+             lambda old, new: old.union(new),

+             [pol["product_versions"] for pol in self.query_policies()["policies"]],

+             set()

+         )

+ 

+     def check_gating(self, build):

+         """

+         Query decision to greenwave

+         :param build: build object

+         :type build: module_build_service.models.ModuleBuild

+         :return: True if at least one GW response contains policies_satisfied set to true

+         :rtype: bool

+         """

+         try:

+             versions = self.get_product_versions()

+         except GreenwaveError:

+             log.warning('An error occured while getting a product versions')

+             return False

+ 

+         for ver in versions:

+             try:

+                 if self.query_decision(build, ver)["policies_satisfied"]:

+                     # at least one positive result is enough

+                     return True

+             except (KeyError, GreenwaveError) as exc:

+                 log.warning('Incorrect greenwave result "%s", ignoring', str(exc))

+ 

+         return False

  

      @property

      def url(self):

@@ -96,8 +179,9 @@ 

      @url.setter

      def url(self, value):

          value = value.rstrip("/")

-         if value:

-             self._url = value

+         if not value:

+             raise GreenwaveError("No Greenwave URL set")

+         self._url = value

  

      @property

      def decision_context(self):

@@ -114,3 +198,10 @@ 

      @timeout.setter

      def timeout(self, value):

          self._gw_timeout = value

+ 

+ 

+ try:

+     greenwave = Greenwave()

+ except GreenwaveError:

+     log.warning('Greenwave is not configured or configured improperly')

+     greenwave = None

@@ -406,9 +406,12 @@ 

                  pass

  

      @pytest.mark.parametrize("mmd_version", [1, 2])

+     @patch("module_build_service.utils.greenwave.Greenwave.check_gating", return_value=True)

      @patch("module_build_service.auth.get_user", return_value=user)

      @patch("module_build_service.scm.SCM")

-     def test_submit_build(self, mocked_scm, mocked_get_user, conf_system, dbg, hmsc, mmd_version):

+     def test_submit_build(

+         self, mocked_scm, mocked_get_user, mocked_greenwave, conf_system, dbg, hmsc, mmd_version

+     ):

          """

          Tests the build of testmodule.yaml using FakeModuleBuilder which

          succeeds everytime.

@@ -481,12 +484,17 @@ 

          assert module_build.module_builds_trace[4].state == models.BUILD_STATES["ready"]

          assert len(module_build.module_builds_trace) == 5

  

+     @pytest.mark.parametrize("gating_result", (True, False))

+     @patch("module_build_service.utils.greenwave.Greenwave.check_gating")

      @patch("module_build_service.auth.get_user", return_value=user)

      @patch("module_build_service.scm.SCM")

-     def test_submit_build_no_components(self, mocked_scm, mocked_get_user, conf_system, dbg, hmsc):

+     def test_submit_build_no_components(

+         self, mocked_scm, mocked_get_user, mocked_greenwave, conf_system, dbg, hmsc, gating_result

+     ):

          """

          Tests the build of a module with no components

          """

+         mocked_greenwave.return_value = gating_result

          FakeSCM(

              mocked_scm,

              "python3",

@@ -512,7 +520,10 @@ 

          # Make sure no component builds were registered

          assert len(module_build.component_builds) == 0

          # Make sure the build is done

-         assert module_build.state == models.BUILD_STATES["ready"]

+         if gating_result:

+             assert module_build.state == models.BUILD_STATES["ready"]

+         else:

+             assert module_build.state == models.BUILD_STATES["done"]

  

      @patch(

          "module_build_service.config.Config.check_for_eol",

@@ -1412,10 +1423,11 @@ 

                  models.BUILD_STATES["ready"],

              ]

  

+     @patch("module_build_service.utils.greenwave.Greenwave.check_gating", return_value=True)

      @patch("module_build_service.auth.get_user", return_value=user)

      @patch("module_build_service.scm.SCM")

      def test_submit_build_resume_init_fail(

-         self, mocked_scm, mocked_get_user, conf_system, dbg, hmsc

+         self, mocked_scm, mocked_get_user, mock_greenwave, conf_system, dbg, hmsc

      ):

          """

          Tests that resuming the build fails when the build is in init state

@@ -1654,10 +1666,11 @@ 

          module = db.session.query(models.ModuleBuild).get(module_build_id)

          assert module.state == models.BUILD_STATES["build"]

  

+     @patch("module_build_service.utils.greenwave.Greenwave.check_gating", return_value=True)

      @patch("module_build_service.auth.get_user", return_value=user)

      @patch("module_build_service.scm.SCM")

      def test_submit_br_metadata_only_module(

-         self, mocked_scm, mocked_get_user, conf_system, dbg, hmsc

+         self, mocked_scm, mocked_get_user, mock_greenwave, conf_system, dbg, hmsc

      ):

          """

          Test that when a build is submitted with a buildrequire without a Koji tag,

@@ -23,13 +23,14 @@ 

  

  import json

  from mock import patch, Mock

- import module_build_service.utils.greenwave

+ import pytest

+ from module_build_service.utils.greenwave import greenwave

  from tests import make_module

  

  

  class TestGreenwaveQuery():

      @patch("module_build_service.utils.greenwave.requests")

-     def test_greenwave_decision(self, mock_requests):

+     def test_greenwave_query_decision(self, mock_requests):

          resp_status = 200

          resp_content = {

              "applicable_policies": ["osci_compose_modules"],

@@ -56,8 +57,7 @@ 

  

          fake_build = make_module("pkg:0.1:1:c1", requires_list={"platform": "el8"})

  

-         gw = module_build_service.utils.greenwave.Greenwave()

-         got_response = gw.query_decision(fake_build, prod_version="xxxx-8")

+         got_response = greenwave.query_decision(fake_build, prod_version="xxxx-8")

  

          assert got_response == resp_content

          assert json.loads(mock_requests.post.call_args_list[0][1]["data"]) == {

@@ -68,3 +68,112 @@ 

              "Content-Type": "application/json"}

          assert mock_requests.post.call_args_list[0][1]["url"] == \

              "https://greenwave.example.local/api/v1.0/decision"

+ 

+     @pytest.mark.parametrize("return_all", (False, True))

+     @patch("module_build_service.utils.greenwave.requests")

+     def test_greenwave_query_policies(self, mock_requests, return_all):

+         resp_status = 200

+         resp_content = {

+             "policies": [

+                 {

+                     "decision_context": "test_dec_context",

+                     "product_versions": ["ver1", "ver3"],

+                     "rules": [],

+                     "subject_type": "some-module"

+                 },

+                 {

+                     "decision_context": "test_dec_context",

+                     "product_versions": ["ver1", "ver2"],

+                     "rules": [],

+                     "subject_type": "some-module"

+                 },

+                 {

+                     "decision_context": "decision_context_2",

+                     "product_versions": ["ver4"],

+                     "rules": [],

+                     "subject_type": "subject_type_2"

+                 }

+             ]

+         }

+         selected_policies = {"policies": resp_content["policies"][:-1]}

+ 

+         response = Mock()

+         response.json.return_value = resp_content

+         response.status_code = resp_status

+         mock_requests.get.return_value = response

+ 

+         got_response = greenwave.query_policies(return_all)

+ 

+         if return_all:

+             assert got_response == resp_content

+         else:

+             assert got_response == selected_policies

+         assert mock_requests.get.call_args_list[0][1]["url"] == \

+             "https://greenwave.example.local/api/v1.0/policies"

+ 

+     @patch("module_build_service.utils.greenwave.requests")

+     def test_greenwave_get_product_versions(self, mock_requests):

+         resp_status = 200

+         resp_content = {

+             "policies": [

+                 {

+                     "decision_context": "test_dec_context",

+                     "product_versions": ["ver1", "ver3"],

+                     "rules": [],

+                     "subject_type": "some-module"

+                 },

+                 {

+                     "decision_context": "test_dec_context",

+                     "product_versions": ["ver1", "ver2"],

+                     "rules": [],

+                     "subject_type": "some-module"

+                 },

+                 {

+                     "decision_context": "decision_context_2",

+                     "product_versions": ["ver4"],

+                     "rules": [],

+                     "subject_type": "subject_type_2"

+                 }

+             ]

+         }

+         expected_versions = {"ver1", "ver2", "ver3"}

+ 

+         response = Mock()

+         response.json.return_value = resp_content

+         response.status_code = resp_status

+         mock_requests.get.return_value = response

+ 

+         versions_set = greenwave.get_product_versions()

+ 

+         assert versions_set == expected_versions

+         assert mock_requests.get.call_args_list[0][1]["url"] == \

+             "https://greenwave.example.local/api/v1.0/policies"

+ 

+     @pytest.mark.parametrize("policies_satisfied", (True, False))

+     @patch("module_build_service.utils.greenwave.requests")

+     def test_greenwave_check_gating(self, mock_requests, policies_satisfied):

+         resp_status = 200

+         policies_content = {

+             "policies": [

+                 {

+                     "decision_context": "test_dec_context",

+                     "product_versions": ["ver1", "ver3"],

+                     "rules": [],

+                     "subject_type": "some-module"

+                 }

+             ]

+         }

+ 

+         responses = [Mock() for i in range(3)]

+         for r in responses:

+             r.status_code = resp_status

+         responses[0].json.return_value = policies_content

+         responses[1].json.return_value = {"policies_satisfied": False}

+         responses[2].json.return_value = {"policies_satisfied": policies_satisfied}

+         mock_requests.get.return_value = responses[0]

+         mock_requests.post.side_effect = responses[1:]

+ 

+         fake_build = make_module("pkg:0.1:1:c1", requires_list={"platform": "el8"})

+         result = greenwave.check_gating(fake_build)

+ 

+         assert result == policies_satisfied

Build 289a906f2a2a363fc4fcfdea3c9ad2fef81e53d1 FAILED!
Rebase or make new commits to rebuild.

Hm, why do you create new instance here and not in the done method?

rebased onto 234c811a7a4ffc8471f2c979cc335446c9fdbbc5

4 months ago

Build 234c811a7a4ffc8471f2c979cc335446c9fdbbc5 FAILED!
Rebase or make new commits to rebuild.

log.exception already contains the exception information, so it might be better to just do log.exception("Querying the Greenwave policies API endpoint failed.

Explaining what request failed would be nice here

This same try/except is in query_decision. How about breaking it out to a separate private method so you can just do resp_json = self._get_response_json(response) in both places.

This return_all flag is not used by code that calls this method. My preference is to just remove it. It's less code to maintain and write unit tests for (a unit test for this is missing in this PR anyway).

I think this except is not necessary since Greenwave will always return those keys if the response is 200.

Optional: If you make rv a set to begin with, you could do:

rv = rv | set(pol["product_versions"])

Then duplicates would never be stored. I'm not sure which is more efficient, but at this scale, it doesn't really matter. I just thought I'd propose an alternative in case you prefer it.

What do you think about the following instead?

selected_policies = {"policies": resp_content["policies"][:-1]}

I'm not sure we should log this as an exception since it seems like it's desired behavior to just ignore it. I think a debug log is more suitable here.

As I mentioned elsewhere in the review, log.exception already contains the exception info, so repeating the exception isn't very useful. Perhaps you can add context to the exception such as the module this failed with.

I think the code in the done handler should be in a method that can be reused in the MBS poller. Since it's likely the gating decision will fail in the done handler since the tests are still running, we need a mechanism to check later on if the gating decision changed.

Build 234c811 FAILED!
Rebase or make new commits to rebuild.

15:05:38  [logs:build/mbs-backend-build-21-ddd8a00-1] 2019-05-10T13:05:38.589085000Z =================== 558 passed, 1 warnings in 184.06 seconds ===================
15:05:39  [logs:build/mbs-backend-build-21-ddd8a00-1] 2019-05-10T13:05:39.563638000Z ___________________________________ summary ____________________________________
15:05:39  [logs:build/mbs-backend-build-21-ddd8a00-1] 2019-05-10T13:05:39.567320000Z   py3: commands succeeded
15:05:39  [logs:build/mbs-backend-build-21-ddd8a00-1] 2019-05-10T13:05:39.567551000Z   congratulations :)
15:05:40  [logs:build/mbs-backend-build-21-ddd8a00-1] 2019-05-10T13:05:40.652236000Z error: build error: no such image

@mikeb What does that means?

Build 234c811a7a4ffc8471f2c979cc335446c9fdbbc5 FAILED!
Rebase or make new commits to rebuild.

Build 234c811a7a4ffc8471f2c979cc335446c9fdbbc5 FAILED!
Rebase or make new commits to rebuild.

@vmaljulin This is a legitimate failure:

11:26:29  [logs:deploymentconfig/mbs-jenkins-15-60c1e9a-backend] [2019-05-10 18:26:24][  stomp.py   DEBUG] Received frame: 'RECEIPT', headers={'receipt-id': 'b05caf5b-f245-4016-a58b-421e7074bf55'}, body=''
11:26:29  [logs:deploymentconfig/mbs-jenkins-15-60c1e9a-backend] [2019-05-10 18:26:24][  stomp.py    INFO] Receiver loop ended
11:26:29  [logs:deploymentconfig/mbs-jenkins-15-60c1e9a-backend] [2019-05-10 18:26:24][MBS.scheduler.consumer   ERROR] Could not process message handler. See the traceback.
11:26:29  [logs:deploymentconfig/mbs-jenkins-15-60c1e9a-backend] Traceback (most recent call last):
11:26:29  [logs:deploymentconfig/mbs-jenkins-15-60c1e9a-backend]   File "/usr/lib/python3.7/site-packages/module_build_service/scheduler/consumer.py", line 256, in process_message
11:26:29  [logs:deploymentconfig/mbs-jenkins-15-60c1e9a-backend]     further_work = handler(conf, session, msg) or []
11:26:29  [logs:deploymentconfig/mbs-jenkins-15-60c1e9a-backend]   File "/usr/lib/python3.7/site-packages/module_build_service/scheduler/handlers/modules.py", line 141, in done
11:26:29  [logs:deploymentconfig/mbs-jenkins-15-60c1e9a-backend]     greenwave = Greenwave()
11:26:29  [logs:deploymentconfig/mbs-jenkins-15-60c1e9a-backend]   File "/usr/lib/python3.7/site-packages/module_build_service/utils/greenwave.py", line 37, in __init__
11:26:29  [logs:deploymentconfig/mbs-jenkins-15-60c1e9a-backend]     if not self.url:
11:26:29  [logs:deploymentconfig/mbs-jenkins-15-60c1e9a-backend]   File "/usr/lib/python3.7/site-packages/module_build_service/utils/greenwave.py", line 153, in url
11:26:29  [logs:deploymentconfig/mbs-jenkins-15-60c1e9a-backend]     return self._url
11:26:29  [logs:deploymentconfig/mbs-jenkins-15-60c1e9a-backend] AttributeError: 'Greenwave' object has no attribute '_url'

https://jenkins-mbs-koji-int-test.cloud.paas.psi.redhat.com/job/mbs-koji-int-test/job/mbs-koji-int-test-mbs-dev-integration-test/15/consoleFull

1 new commit added

  • fixup! Add greenwave query to done handler
4 months ago

Build 514a5db23a9c0a35cb75b33a8ae7f1bf6f4ce2f9 FAILED!
Rebase or make new commits to rebuild.

pretty please pagure-ci rebuild

4 months ago

pretty please pagure-ci rebuild

4 months ago

rebased onto 79d43cbd5b65977edd3a613d32de323692e54977

4 months ago

Comments were addressed, please check it out.

I like what @mprahl mentioned earlier about this part. Maybe you can move it to Greenwave class. The name could be for example is_build_gated and here you can do something like this:

try:
    greenwave = Greenwave()
    gw_result = Greenwave.is_build_gated(build)
    ...
except:
    ...

The benefit would be cleaner code here and we can also reuse that method in the Poller later.

This transition to ready needs to be removed, otherwise the module is always moved from "done" to "ready", no matter what is the Greenwave decision.

You need to do something like this here instead:

    # Scratch builds stay in 'done' state.
    if build.scratch:
        return

This code-block also needs to handle the case when Greenwave is not configured in the MBS configuration. In this case, MBS needs to move module build from "done" to "ready" without querying Greenwave. That's the case for Fedora.

1 new commit added

  • fixup! Add greenwave query to done handler
4 months ago

rebased onto 086f47d26079c72958f9de8558037c5fda67ed1e

4 months ago

Build 086f47d26079c72958f9de8558037c5fda67ed1e FAILED!
Rebase or make new commits to rebuild.

rebased onto 0a85dbc7897ae816766d4f8ed65c3694a5258690

4 months ago

How about:

if greenwave is None or greenwave.check_gating(build):
    build.transition(config, state="ready")
    session.commit()

Why did you stop logging the exception? With this change, we can't see the original traceback anymore.

This seems overly complicated for what you need for an internal method. It's also not flexible. Ideally, we just want a wrapper around Python requests for error handling. How about the following (untested)?

    def _greenwave_query(self, request_type, *args, **kwargs):
        request_func = getattr(request_type, requests)
        kwargs.setdefault("timeout", 30)
        kwargs.setdefault("headers", {"Content-Type": "application/json"})
        try:
            response = request_func(*args, **kwargs)
        except requests.exceptions.Timeout:
            raise GreenwaveError("Greenwave request timed out")
        except Exception as exc:
            msg = "An unspecified Greenwave request error was encountered"
            log.exception(msg)
            raise GreenwaveError(msg)

        try:
            resp_json = response.json()
        except ValueError:
            log.debug("Greenwave response content (status {0}): {1}".format(
                response.status_code, response.text
            ))
            raise GreenwaveError("Greenwave returned invalid JSON.")

        log.debug(
            'Query to Greenwave (%s) result: status=%d, content="%s"',
            (response.url, response.status_code, resp_json)
        )

        if response.status_code == 200:
            return resp_json

        try:
            err_msg = resp_json["message"]
        except KeyError:
            err_msg = response.text
        raise GreenwaveError("Greenwave returned {0} status code. Message: {1}".format(
            response.status_code, err_msg
        ))

Optional: On the requests side, if you use json=payload instead of data=payload, the conversion to JSON happens for you. Although I'm not 100% sure the requests version on RHEL 7 supports this. It might be worth checking.

Please assign "Incorrect greenwave response (Mandatory key is missing)" to a variable to duplicate it.

Build 0a85dbc7897ae816766d4f8ed65c3694a5258690 FAILED!
Rebase or make new commits to rebuild.

@vmaljulin could you please rebase? The C3I tests should be passing in master now.

rebased onto 95bacc4

4 months ago

I looks good me to I think.

Pull-Request has been merged by jkaluza

4 months ago