PR#1251: Add greenwave query to done handler - fm-orchestrator

fm-orchestrator

#1251 Add greenwave query to done handler

Merged 4 years ago by jkaluza. Opened 4 years ago by vmaljulin.

vmaljulin/fm-orchestrator FACTORY-4007 into master

Add greenwave query to done handler

Valerij Maljulin • 4 years ago

95bacc4

module_build_service/scheduler/handlers/modules.py

file modified

+5 -3

		`@@ -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)`

module_build_service/utils/greenwave.py

file modified

+117 -26

		`@@ -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`

tests/test_build/test_build.py

file modified

+18 -5

		`@@ -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,`

tests/test_utils/test_greenwave.py

file modified

+113 -4

		`@@ -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`

vmaljulin commented 4 years ago

Signed-off-by: Valerij Maljulin vmaljuli@redhat.com

mikeb commented 4 years ago

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

jkaluza commented on line 14 of module_build_service/scheduler/handlers/modules.py 4 years ago

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

rebased onto 234c811a7a4ffc8471f2c979cc335446c9fdbbc5

4 years ago

mprahl commented on line 32 of module_build_service/scheduler/handlers/modules.py 4 years ago

This line can be removed

mikeb commented 4 years ago

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

mprahl commented on line 15 of module_build_service/scheduler/handlers/modules.py 4 years ago

Why do you have this if statement?

mprahl commented on line 30 of module_build_service/utils/greenwave.py 4 years ago

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

mprahl commented on line 31 of module_build_service/utils/greenwave.py 4 years ago

Explaining what request failed would be nice here

mprahl commented on line 33 of module_build_service/utils/greenwave.py 4 years ago

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.

mprahl commented on line 41 of module_build_service/utils/greenwave.py 4 years ago

Please see the style guide section about hanging indentation:
https://pagure.io/fm-orchestrator/blob/master/f/docs/CONTRIBUTING.rst

mprahl commented on line 16 of module_build_service/utils/greenwave.py 4 years ago

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).

mprahl commented on line 52 of module_build_service/utils/greenwave.py 4 years ago

This exception should be logged

mprahl commented on line 52 of module_build_service/utils/greenwave.py 4 years ago

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

mprahl commented on line 72 of module_build_service/utils/greenwave.py 4 years ago

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.

mprahl commented on line 40 of tests/test_utils/test_greenwave.py 4 years ago

What do you think about the following instead?

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

mprahl commented on line 25 of module_build_service/scheduler/handlers/modules.py 4 years ago

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.

mprahl commented on line 26 of module_build_service/scheduler/handlers/modules.py 4 years ago

This can be removed

mprahl commented on line 32 of module_build_service/scheduler/handlers/modules.py 4 years ago

This can be removed

mprahl commented on line 31 of module_build_service/scheduler/handlers/modules.py 4 years ago

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.

mprahl commented 4 years ago

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.

vmaljulin commented 4 years ago

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?

mikeb commented 4 years ago

@vmaljulin Not sure, maybe a transient error communicating with the container repo? I'm re-running the job:

https://jenkins-mbs-koji-int-test.cloud.paas.psi.redhat.com/job/mbs-koji-int-test/job/mbs-koji-int-test-mbs-premerge/24/

mikeb commented 4 years ago

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

mikeb commented 4 years ago

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

mikeb commented 4 years ago

@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 years ago

mikeb commented 4 years ago

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

pretty please pagure-ci rebuild

4 years ago

pretty please pagure-ci rebuild

4 years ago

rebased onto 79d43cbd5b65977edd3a613d32de323692e54977

4 years ago

vmaljulin commented 4 years ago

Comments were addressed, please check it out.

jkaluza commented on line 13 of module_build_service/scheduler/handlers/modules.py 4 years ago

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.

jkaluza commented on line 10 of module_build_service/scheduler/handlers/modules.py 4 years ago

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

jkaluza commented on line 13 of module_build_service/scheduler/handlers/modules.py 4 years ago

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 years ago

jkaluza commented on line 153 of module_build_service/utils/greenwave.py 4 years ago

Can you add comment please?

rebased onto 086f47d26079c72958f9de8558037c5fda67ed1e

4 years ago

mikeb commented 4 years ago

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

rebased onto 0a85dbc7897ae816766d4f8ed65c3694a5258690

4 years ago

mprahl commented on line 18 of module_build_service/scheduler/handlers/modules.py 4 years ago

How about:

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

mprahl commented on line 64 of module_build_service/utils/greenwave.py 4 years ago

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

mprahl commented on line 22 of module_build_service/utils/greenwave.py 4 years ago

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
        ))

mprahl commented on line 113 of module_build_service/utils/greenwave.py 4 years ago

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.