Make redfish firmware updates work on AMI MegaRAC

Three things stopped a redfish firmware update on MegaRAC. The AMI handler
opened by asking the bmc to preserve fourteen named settings, and a build that
knows a different set rejects the whole request, which aborted the update
before anything was uploaded; send only the keys the bmc advertises. The
multipart push carried the image alone, and the specification has it carry an
UpdateParameters part too, which this firmware enforces. AMI also wants an
OemParameters part naming the kind of image, and nothing was supplying one.

The kind of image is asked for rather than worked out from the file. The
extension is vendor habit rather than format, and the leading bytes answer just
as confidently about an image they have never seen, while being wrong means a
bmc flashed with a bios image. So nodefirmware takes --type, it travels as far
as the handler that wants it, and where the bmc publishes the types it accepts,
an unknown one is refused with the list, as is asking with none. A platform
that reads the kind of firmware out of the image itself refuses the option
rather than dropping it, so nobody aims an update somewhere they did not mean
to. A parameter file still wins, since it can carry more than the image type.

Updating the bmc takes the bmc, and the task being watched, away for minutes.
That is the update working rather than the monitoring failing, so wait a
bounded while for it to answer again instead of reporting a successful flash
as an error.

confluent_client/bin/nodefirmware

@@ -56,11 +56,14 @@ components = ['all']
 
 argparser = optparse.OptionParser(
     usage="Usage: "
-          "%prog <noderange> [list][updatestatus][update [--backup <file>]]|[<components>]")
+          "%prog <noderange> [list][updatestatus][update [--backup] "
+          "[--parameterfile <file>] <file>]|[<components>]")
 argparser.add_option('-b', '--backup', action='store_true',
                      help='Target a backup bank rather than primary')
 argparser.add_option('-p', '--parameterfile', type='string',
-                     help='When updating, use the specified parameter file')
+                     help='When updating, use the specified parameter file; see '
+                          'nodefirmware(8), some platforms need it to say what '
+                          'kind of firmware the image holds')
 argparser.add_option('-m', '--maxnodes', type='int',
                      help='When updating, prompt if more than the specified '
                           'number of servers will be affected')

confluent_client/doc/man/nodefirmware.ronn

@@ -3,7 +3,7 @@ nodefirmware(8) -- Report firmware information on confluent nodes
 
 ## SYNOPSIS
 
-`nodefirmware <noderange> [list][updatestatus][update [--backup <file>]]|[<components>]`
+`nodefirmware <noderange> [list][updatestatus][update [--backup] [--parameterfile <file>] <file>]|[<components>]`
 
 ## DESCRIPTION
 
@@ -37,11 +37,31 @@ the out of band facilities.  Firmware updates can end in one of three states:
   be affected  
 
 * `-p PARAMETERFILE`, `--parameterfile=PARAMETERFILE`:
-  For updating, a parameter file to provide along with the update payload
+  For updating, a parameter file to provide along with the update payload.  See
+  PARAMETER FILE below
 
 * `-h`, `--help`:
   Show help message and exit    
 
+## PARAMETER FILE
+
+The parameter file given with `-p` holds JSON that is sent alongside the image.
+Its keys are the parts a Redfish multipart update accepts, so `UpdateParameters`
+for the standard fields and `OemParameters` for whatever a vendor asks for on top
+of them.  Without one, an update names no targets, which means "whatever this
+image is for", and that is what most platforms want.
+
+Some platforms will not take an image without being told what kind of firmware it
+holds, and refuse the update rather than guess.  On an AMI MegaRAC that is an
+`ImageType` under `OemParameters`:
+
+    {"OemParameters": {"ImageType": "BIOS"}}
+
+Attempting the update without it names the types the platform accepts in the
+error, so there is no need to look them up first.  The value is checked against
+the same list before anything is uploaded, so a name the platform does not accept
+is refused rather than sent.
+
 ## EXAMPLES
 
 * Pull firmware from a node:

confluent_server/aiohmi/redfish/oem/ami/megarac.py

@@ -12,6 +12,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import os
+
 import aiohmi.redfish.oem.generic as generic
 import aiohmi.util.webclient as webclient
 from urllib.parse import urlencode
@@ -54,29 +56,88 @@ class OEMHandler(generic.OEMHandler):
             msgents.append(self.format_messages(msg))
         return ';'.join(msgents)
 
-    async def update_firmware(self, filename, data=None, progress=None, bank=None, otherfields=()):
+    # What to keep across a firmware update, for the builds that let us say.
+    # SDR is excluded because a new firmware image is expected to bring its own.
+    _preserveconfig = {
+        'Syslog': True,
+        'NTP': True,
+        'Network': True,
+        'Authentication': True,
+        'EXTLOG': True,
+        'FRU': True,
+        'IPMI': True,
+        'KVM': True,
+        'REDFISH': True,
+        'SDR': False,
+        'SEL': True,
+        'SNMP': True,
+        'SSH': True,
+        'WEB': True,
+    }
+
+    async def _preserve_configuration(self):
+        """Ask the bmc to keep its configuration across the update.
+
+        Builds differ in which settings they are willing to preserve, and they
+        reject the whole request if it names one they do not know, so send only
+        the intersection with what this bmc advertises.
+        """
+        usd = await self._do_web_request('/redfish/v1/UpdateService', cache=False)
+        advertised = usd.get('Oem', {}).get('AMIUpdateService', {}).get(
+            'PreserveConfiguration', None)
+        if not advertised:
+            return
+        preserve = {k: v for k, v in self._preserveconfig.items() if k in advertised}
+        if not preserve:
+            return
         await self._do_web_request('/redfish/v1/UpdateService', {
-            "Oem": {
-                "AMIUpdateService": {
-                "@odata.type": "#AMIUpdateService.v1_0_0.AMIUpdateService",
-                "PreserveConfiguration": {
-                    "Syslog": True,
-                    "NTP": True,
-                    "Network": True,
-                    "Authentication": True,
-                    "EXTLOG": True,
-                    "FRU": True,
-                    "IPMI": True,
-                    "KVM": True,
-                    "REDFISH": True,
-                    "SDR": False,
-                    "SEL": True,
-                    "SNMP": True,
-                    "SSH": True,
-                    "WEB": True
-            }
-            }}}, method='PATCH', etag='*')
-        return await super().update_firmware(filename, data, progress, bank, otherfields)
+            'Oem': {
+                'AMIUpdateService': {
+                    '@odata.type': '#AMIUpdateService.v1_0_0.AMIUpdateService',
+                    'PreserveConfiguration': preserve,
+                }}}, method='PATCH', etag='*')
+
+    async def _allowed_imagetypes(self):
+        actinfo = await self._do_web_request(
+            '/redfish/v1/UpdateService/SimpleUpdateActionInfo')
+        for param in actinfo.get('Parameters', []):
+            if param.get('Name', None) == 'ImageType':
+                return param.get('AllowableValues', [])
+        return []
+
+    async def _checked_imagetype(self, imagetype, filename):
+        """Check the image type the parameter file gave against what is accepted.
+
+        Flashing the wrong kind of image is not something to be clever about, so
+        the type is asked for rather than worked out from the file.
+        """
+        allowed = await self._allowed_imagetypes()
+        choices = ', '.join(allowed) if allowed else 'BMC, BIOS'
+        if not imagetype:
+            raise pygexc.InvalidParameterValue(
+                'This bmc needs to be told what kind of firmware "{0}" is, '
+                'one of: {1}'.format(os.path.basename(filename), choices))
+        if allowed and imagetype not in allowed:
+            raise pygexc.InvalidParameterValue(
+                '"{0}" is not a firmware image type this bmc accepts, it '
+                'offers: {1}'.format(imagetype, choices))
+        return imagetype
+
+    async def update_firmware(self, filename, data=None, progress=None, bank=None, otherfields=()):
+        await self._preserve_configuration()
+        otherfields = dict(otherfields) if otherfields else {}
+        oemparams = dict(otherfields.get('OemParameters', None) or {})
+        imagetype = await self._checked_imagetype(
+            oemparams.get('ImageType', None), filename)
+        oemparams['ImageType'] = imagetype
+        otherfields['OemParameters'] = oemparams
+        # Updating the bmc takes the bmc away mid flight, which is expected
+        # rather than a failure to watch the update
+        self._updateresetsbmc = imagetype == 'BMC'
+        try:
+            return await super().update_firmware(filename, data, progress, bank, otherfields)
+        finally:
+            self._updateresetsbmc = False
 
 
     async def get_wc(self):

confluent_server/aiohmi/redfish/oem/generic.py

@@ -17,6 +17,7 @@ from fnmatch import fnmatch
 import json
 import os
 import re
+import time
 import uuid
 
 import base64
@@ -1427,6 +1428,14 @@ class OEMHandler(object):
     async def update_firmware(self, filename, data=None, progress=None, bank=None, otherfields=()):
         # disable cache to make sure we trigger the token renewal logic if needed
         usd, upurl, ismultipart = await self.retrieve_firmware_upload_url()
+        if ismultipart:
+            # A multipart push has to carry an UpdateParameters part beside the
+            # image, and implementations are entitled to reject the request
+            # when it is missing. Targets empty means "whatever this image is
+            # for", and OperationApplyTime is left out because it is optional
+            # and some implementations refuse the ones they do not implement.
+            otherfields = dict(otherfields) if otherfields else {}
+            otherfields.setdefault('UpdateParameters', {'Targets': []})
         try:
             uploadthread = await webclient.make_uploader(
                 self.webclient, upurl, filename, data, formname='UpdateFile', formwrap=ismultipart,
@@ -1465,6 +1474,20 @@ class OEMHandler(object):
             monitorurl = rsp['@odata.id']
             return await self.monitor_update_progress(monitorurl, progress)
 
+    # How long to keep waiting for a bmc that is expected to restart while it
+    # applies an update to itself. Observed reset to serving again is a bit over
+    # three minutes, so this leaves generous room without hanging forever.
+    _resetgracetime = 300
+
+    async def _bmc_is_back(self):
+        """Check whether the redfish service is serving requests again."""
+        try:
+            rsp, status = await self.webclient.grab_json_response_with_status(
+                '/redfish/v1/')
+        except Exception:
+            return False
+        return status == 200 and bool(rsp)
+
     async def monitor_update_progress(self, monitorurl, progress):
             complete = False
             phase = "apply"
@@ -1473,12 +1496,30 @@ class OEMHandler(object):
             # the validating phase; add a retry here so we don't exit the loop in this case
             retry = 3
             pct = 0.0
+            # Updating the bmc itself takes the bmc, and with it the task we are
+            # watching, away for minutes. That is the update working, not the
+            # monitoring failing, so wait it out and take the bmc coming back
+            # with the task gone as the update having landed.
+            deadline = None
+            if getattr(self, '_updateresetsbmc', False):
+                deadline = time.monotonic() + self._resetgracetime
             while not complete and retry > 0:
                 try:
                     pgress = await self._do_web_request(monitorurl, cache=False)
-                except socket.timeout:
+                except (socket.timeout, exc.PyghmiException, OSError):
                     pgress = None
                 if not pgress:
+                    if deadline is not None:
+                        if time.monotonic() > deadline:
+                            raise Exception(
+                                'The bmc did not return within {0} seconds of '
+                                'starting to apply its firmware'.format(
+                                    self._resetgracetime))
+                        await asyncio.sleep(10)
+                        if await self._bmc_is_back():
+                            progress({'phase': phase, 'progress': 100.0})
+                            return 'pending'
+                        continue
                     retry -= 1
                     await asyncio.sleep(3)
                     continue