Merge ~fwieffering/maas:tgt-redfish-bugfix into maas:master

[Freddy]

Fixes LP: #1845032 - Fixes issues that causes MAAS Redfish power driver to not work on some BMC's.

* removed trailing slashes from redfish power driver. This caused errors for some manufacturers implementations that we use
* catch json parsing errors and return a sensible error message if it fails to parse
* increase timeout on power query calls
* check state in redfish power driver power_off prior to submitting power_off request. Some redfish implementations we encountered errors if the state was already off and a power off request was submitted

We have switched to the redfish power driver for MaaS and encountered a few bugs that we needed to fix.

- removed trailing slashes from redfish power driver. This caused errors for some manufacturers implementations that we use
- catch json parsing errors and return a sensible error message if it fails to parse
- increase timeout on power query calls
- check state in redfish power driver power_off prior to submitting power_off request. Some redfish implementations we encountered errors if the state was already off and a power off request was submitted

Will comment with branch once bug is created

[MAAS Lander (maas-lander)]

UNIT TESTS
-b tgt-redfish-bugfix lp:~fwieffering/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6476/console
COMMIT: 18bdccb5235e69f27e6a387aa010f489c469dac0

review: Needs Fixing

[Blake Rouse (blake-rouse)]

I like the fixes, overall looks acceptable. But tests need to be updated and more need to be added to cover the changes.

review: Needs Fixing

[Freddy (fwieffering)]

I will take care of that.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b tgt-redfish-bugfix lp:~fwieffering/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6478/console
COMMIT: ecdda8fc9aa154861e2b056c683170d4e566efba

review: Needs Fixing

[Freddy (fwieffering)]

The tests should be all set. I don't know if this jenkins job is going to run again or not, but I have them passing.

[Newell Jensen (newell-jensen)]

Some manufacturers require the trailing slash. We should probably tailor the driver to attempt both cases.

[Blake Rouse (blake-rouse)]

Code change looks good and the tests look good. Only issue I see is that you have not signed the Canonical contributors agreement. Already having a launchpad account makes that really easy.

https://ubuntu.com/legal/contributors/agreement

[MAAS Lander (maas-lander)]

UNIT TESTS
-b tgt-redfish-bugfix lp:~fwieffering/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6485/console
COMMIT: d0470f99ac1de4f6cac3fcc814b9751982eabc88

review: Needs Fixing

[Freddy (fwieffering)]

I have signed the canonical contributors agreement

[MAAS Lander (maas-lander)]

UNIT TESTS
-b tgt-redfish-bugfix lp:~fwieffering/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: ae549d3b087f66e0ccfff7ae2b72334cfe99f517

review: Approve

[Blake Rouse (blake-rouse)]

Seems that @newell-jensen, our resident power driver expert and original author of the Redfish driver says that some BMC's require the '/' at the end. We need to confirm this is the case before landing this.

review: Needs Information

[Freddy (fwieffering)]

I can add a retry to add a trailing slash if the power driver call fails for any reason. Would you like that approach or is this something that can be investigated?

[Blake Rouse (blake-rouse)]

I would hope in the case that a ending slash is required the BMC would return a 301 redirect to the URL with a slash append, we should ensure that we handle a 301 or 302 redirect.

If it returns a 404 we could retry with a pending slash, just to be sure. Seems like its better to be defensive, sense this is not documented in the specification on the proper way.

[Freddy (fwieffering)]

@blake-rouse @newell-jensen I've added a retry with a trailing slash if the request fails with a 404 and the uri does not include a trailing slash

[MAAS Lander (maas-lander)]

UNIT TESTS
-b tgt-redfish-bugfix lp:~fwieffering/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 1c395302c2e11011fd5f66702a7d122a3e2a6e5c

review: Approve

[Freddy (fwieffering)]

Just bumping this in case it fell off the radar

[Blake Rouse (blake-rouse)]

Looks good, thanks for the fixes and the tests!

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b tgt-redfish-bugfix lp:~fwieffering/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6542/consoleText

[Freddy (fwieffering)]

Is there anything more I need to do here?

diff --git a/src/maasserver/api/nodes.py b/src/maasserver/api/nodes.py
index 878c418..f228e1a 100644
--- a/src/maasserver/api/nodes.py
+++ b/src/maasserver/api/nodes.py
@@ -930,7 +930,7 @@ class PowerMixin:
             system_id=system_id, user=request.user,
             perm=NodePermission.view)
         return {
-            "state": node.power_query().wait(45),
+            "state": node.power_query().wait(60),
         }
 
     @operation(idempotent=False)
diff --git a/src/maasserver/clusterrpc/power.py b/src/maasserver/clusterrpc/power.py
index c117386..7eafab6 100644
--- a/src/maasserver/clusterrpc/power.py
+++ b/src/maasserver/clusterrpc/power.py
@@ -215,7 +215,7 @@ def pick_best_power_state(power_states):
 
 
 @asynchronous(timeout=FOREVER)
-def power_query_all(system_id, hostname, power_info, timeout=30):
+def power_query_all(system_id, hostname, power_info, timeout=60):
     """Query every connected rack controller and get the power status from all
     rack controllers.
 
diff --git a/src/provisioningserver/drivers/power/redfish.py b/src/provisioningserver/drivers/power/redfish.py
index ef37367..1bd31b2 100644
--- a/src/provisioningserver/drivers/power/redfish.py
+++ b/src/provisioningserver/drivers/power/redfish.py
@@ -34,14 +34,15 @@ from twisted.web.client import (
     FileBodyProducer,
     PartialDownloadError,
     readBody,
+    RedirectAgent,
 )
 from twisted.web.http_headers import Headers
 
-
+# no trailing slashes
 REDFISH_POWER_CONTROL_ENDPOINT = (
-    b"redfish/v1/Systems/%s/Actions/ComputerSystem.Reset/")
+    b"redfish/v1/Systems/%s/Actions/ComputerSystem.Reset")
 
-REDFISH_SYSTEMS_ENDPOINT = b"redfish/v1/Systems/"
+REDFISH_SYSTEMS_ENDPOINT = b"redfish/v1/Systems"
 
 
 class RedfishPowerDriverBase(PowerDriver):
@@ -61,6 +62,7 @@ class RedfishPowerDriverBase(PowerDriver):
         return Headers(
             {
                 b"User-Agent": [b"MAAS"],
+                b"Accept": [b"application/json"],
                 b"Authorization": [b"Basic " + authorization],
                 b"Content-Type": [b"application/json; charset=utf-8"],
             }
@@ -69,7 +71,8 @@ class RedfishPowerDriverBase(PowerDriver):
     @asynchronous
     def redfish_request(self, method, uri, headers=None, bodyProducer=None):
         """Send the redfish request and return the response."""
-        agent = Agent(reactor, contextFactory=WebClientContextFactory())
+        agent = RedirectAgent(
+            Agent(reactor, contextFactory=WebClientContextFactory()))
         d = agent.request(
             method, uri, headers=headers, bodyProducer=bodyProducer)
 
@@ -90,16 +93,33 @@ class RedfishPowerDriverBase(PowerDriver):
                 data = data.decode('utf-8')
                 # Only decode non-empty response bodies.
                 if data:
-                    return json.loads(data)
+                    # occasionally invalid json is returned. provide a clear
+                    # error in that case
+                    try:
+                        return json.loads(data)
+                    except ValueError as error:
+                        raise PowerActionError(
+                            "Redfish request failed from a JSON parse error:"
+                            " %s." % error)
 
             def cb_attach_headers(data, headers):
                 return data, headers
 
             # Error out if the response has a status code of 400 or above.
             if response.code >= int(HTTPStatus.BAD_REQUEST):
-                raise PowerActionError(
-                    "Redfish request failed with response status code:"
-                    " %s." % response.code)
+                # if there was no trailing slash, retry with a trailing slash
+                # because of varying requirements of BMC manufacturers
+                if (response.code == HTTPStatus.NOT_FOUND and
+                        uri.decode('utf-8')[-1] != "/"):
+                    d = agent.request(
+                        method,
+                        uri + "/".encode('utf-8'),
+                        headers=headers,
+                        bodyProducer=bodyProducer)
+                else:
+                    raise PowerActionError(
+                        "Redfish request failed with response status code:"
+                        " %s." % response.code)
 
             d = readBody(response)
             d.addErrback(eb_catch_partial)
@@ -165,7 +185,7 @@ class RedfishPowerDriver(RedfishPowerDriverBase):
     @inlineCallbacks
     def set_pxe_boot(self, url, node_id, headers):
         """Set the machine with node_id to PXE boot."""
-        endpoint = REDFISH_SYSTEMS_ENDPOINT + b'%s/' % node_id
+        endpoint = join(REDFISH_SYSTEMS_ENDPOINT, b'%s' % node_id)
         payload = FileBodyProducer(
             BytesIO(
                 json.dumps(
@@ -213,14 +233,16 @@ class RedfishPowerDriver(RedfishPowerDriverBase):
         url, node_id, headers = yield self.process_redfish_context(context)
         # Set to PXE boot.
         yield self.set_pxe_boot(url, node_id, headers)
-        # Power off the machine.
-        yield self.power("ForceOff", url, node_id, headers)
+        # Power off the machine if it is not already off
+        power_state = yield self.power_query(node_id, context)
+        if power_state != 'off':
+            yield self.power("ForceOff", url, node_id, headers)
 
     @asynchronous
     @inlineCallbacks
     def power_query(self, node_id, context):
         """Power query machine."""
         url, node_id, headers = yield self.process_redfish_context(context)
-        uri = join(url, REDFISH_SYSTEMS_ENDPOINT + b'%s/' % node_id)
+        uri = join(url, REDFISH_SYSTEMS_ENDPOINT, b'%s' % node_id)
         node_data, _ = yield self.redfish_request(b"GET", uri, headers)
         return node_data.get('PowerState').lower()
diff --git a/src/provisioningserver/drivers/power/tests/test_redfish.py b/src/provisioningserver/drivers/power/tests/test_redfish.py
index ca2a771..a937190 100644
--- a/src/provisioningserver/drivers/power/tests/test_redfish.py
+++ b/src/provisioningserver/drivers/power/tests/test_redfish.py
@@ -224,6 +224,7 @@ class TestRedfishPowerDriver(MAASTestCase):
         authorization = b64encode(creds.encode('utf-8'))
         attributes = {
             b"User-Agent": [b"MAAS"],
+            b"Accept": [b"application/json"],
             b"Authorization": [b"Basic " + authorization],
             b"Content-Type": [b"application/json; charset=utf-8"],
         }
@@ -255,6 +256,60 @@ class TestRedfishPowerDriver(MAASTestCase):
         self.assertEquals(expected_headers.headers, headers)
 
     @inlineCallbacks
+    def test_wrap_redfish_request_retries_404s_trailing_slash(self):
+        driver = RedfishPowerDriver()
+        context = make_context()
+        url = driver.get_url(context)
+        uri = join(url, b"redfish/v1/Systems")
+        headers = driver.make_auth_headers(**context)
+        mock_agent = self.patch(redfish_module, 'Agent')
+        mock_agent.return_value.request = Mock()
+        expected_headers = Mock()
+        expected_headers.code = HTTPStatus.NOT_FOUND
+        expected_headers.headers = "Testing Headers"
+        happy_headers = Mock()
+        happy_headers.code = HTTPStatus.OK
+        happy_headers.headers = "Testing Headers"
+        mock_agent.return_value.request.side_effect = [
+            succeed(expected_headers),
+            succeed(happy_headers)
+        ]
+        mock_readBody = self.patch(redfish_module, 'readBody')
+        mock_readBody.return_value = succeed(
+            json.dumps(SAMPLE_JSON_SYSTEMS).encode('utf-8'))
+        expected_response = SAMPLE_JSON_SYSTEMS
+
+        response, return_headers = yield driver.redfish_request(
+            b"GET", uri, headers)
+        self.assertThat(
+            mock_agent.return_value.request,
+            MockCallsMatch(
+                call(b"GET", uri, headers, None),
+                call(b"GET", uri + "/".encode('utf-8'), headers, None)))
+        self.assertEquals(expected_response, response)
+        self.assertEquals(expected_headers.headers, return_headers)
+
+    @inlineCallbacks
+    def test_redfish_request_raises_invalid_json_error(self):
+        driver = RedfishPowerDriver()
+        context = make_context()
+        url = driver.get_url(context)
+        uri = join(url, b"redfish/v1/Systems")
+        headers = driver.make_auth_headers(**context)
+        mock_agent = self.patch(redfish_module, 'Agent')
+        mock_agent.return_value.request = Mock()
+        expected_headers = Mock()
+        expected_headers.code = HTTPStatus.OK
+        expected_headers.headers = "Testing Headers"
+        mock_agent.return_value.request.return_value = succeed(
+            expected_headers)
+        mock_readBody = self.patch(redfish_module, 'readBody')
+        mock_readBody.return_value = succeed(
+            '{"invalid": "json"'.encode('utf-8'))
+        with ExpectedException(PowerActionError):
+            yield driver.redfish_request(b"GET", uri, headers)
+
+    @inlineCallbacks
     def test_redfish_request_continues_partial_download_error(self):
         driver = RedfishPowerDriver()
         context = make_context()
@@ -371,7 +426,7 @@ class TestRedfishPowerDriver(MAASTestCase):
 
         yield driver.set_pxe_boot(url, node_id, headers)
         self.assertThat(mock_redfish_request, MockCalledOnceWith(
-            b"PATCH", join(url, b"redfish/v1/Systems/%s/" % node_id),
+            b"PATCH", join(url, b"redfish/v1/Systems/%s" % node_id),
             headers, payload))
 
     @inlineCallbacks
@@ -409,6 +464,8 @@ class TestRedfishPowerDriver(MAASTestCase):
         mock_redfish_request.return_value = (
             SAMPLE_JSON_SYSTEMS, None)
         mock_set_pxe_boot = self.patch(driver, 'set_pxe_boot')
+        mock_power_query = self.patch(driver, 'power_query')
+        mock_power_query.return_value = "on"
         mock_power = self.patch(driver, 'power')
 
         yield driver.power_off(node_id, context)
@@ -418,6 +475,26 @@ class TestRedfishPowerDriver(MAASTestCase):
             "ForceOff", url, node_id, headers))
 
     @inlineCallbacks
+    def test__power_off_already_off(self):
+        driver = RedfishPowerDriver()
+        context = make_context()
+        url = driver.get_url(context)
+        headers = driver.make_auth_headers(**context)
+        node_id = b'1'
+        mock_redfish_request = self.patch(driver, 'redfish_request')
+        mock_redfish_request.return_value = (
+            SAMPLE_JSON_SYSTEMS, None)
+        mock_set_pxe_boot = self.patch(driver, 'set_pxe_boot')
+        mock_power_query = self.patch(driver, 'power_query')
+        mock_power_query.return_value = "off"
+        mock_power = self.patch(driver, 'power')
+
+        yield driver.power_off(node_id, context)
+        self.assertThat(mock_set_pxe_boot, MockCalledOnceWith(
+            url, node_id, headers))
+        self.assertThat(mock_power, MockNotCalled())
+
+    @inlineCallbacks
     def test_power_query_queries_on(self):
         driver = RedfishPowerDriver()
         power_change = "On"