826065 - Rework data functions

Status: RESOLVED FIXED

(Testing Graveyard :: Mozpool, defect)

Description

[Mark Côté [:mcote]]

The functions in mozpool/db/data.py are a grab-bag of functionality, intended for different purposes, with no clear design.

We should go through the uses of these functions, categorize them, and come up with a more unified approach.  At the very least, it should make it more obvious where new functionality should go.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: db.patch

This is a big patch!  Switching from treating the DB interface as a global to passing it as necessary required moving a bunch of stuff around.

You'll note I merged the SUT API into the BMM API, and made sut.py a sibling to relay.py and ping.py.

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

Oh, and for my next trick, I'm refactoring the tests.  All tests pass in this patch, but you'll note I deleted all of the DB tests.  They'll be back.

Comment 3

[Mark Côté [:mcote]]

Comment on attachment 716054 [details] [diff] [review]
db.patch

Review of attachment 716054 [details] [diff] [review]:
-----------------------------------------------------------------

This is great!  Much of my commentary is just about consistency, since this is a good opportunity to streamline the mix of techniques in mozpool (e.g. returning results from select is done in at least three distinct but compatible ways).  Aside from specifics below, the only other thing is a bit of a mix of quotation marks and spacing (inconsistent indentation in multi-line statements, only one line between blank line between class definitions).  I didn't bother marking every one of those.  Nothing serious.

::: API.txt
@@ +196,5 @@
>  
>  /api/image/list/
> +* GET to get details on all images.  Returns an object with an 'images'
> +  key, whose value is a list of image objects.  Each image object has
> +  keys ''id', 'name', 'boot_config_keys', 'can_reuse', 'hidden', and

Extra single quote in front of 'id'.

::: README.md
@@ +247,4 @@
>  Release Notes
>  =============
>  
> + * The documentation for ``/api/image/list`` was updated to match its behavior.

Is it not worth at least mentioning the internal changes, given that they're so large?

::: mozpool/db/devices.py
@@ +6,5 @@
> +from sqlalchemy.sql import and_, select
> +from mozpool.db import model, base, exceptions
> +from mozpool import config
> +
> +class Methods(base.MethodsBase, 

Trailing whitespace.

@@ +32,5 @@
> +        if detail:
> +            devices = model.devices
> +            img_svrs = model.imaging_servers
> +            images = model.images
> +            stmt = sqlalchemy.select(

select is imported directly on line 6 so no need to prefix it.  Might as well use select directly and remove the sqlalchemy import on line 5.

@@ +70,5 @@
> +        if environment != 'any':
> +            q = q.where(model.devices.c.environment == environment)
> +        res = self.db.execute(q)
> +        return [{'name': row['name'], 'image': row['image'],
> +                'boot_config': row['boot_config']} for row in res]

Any reason we don't just return [dict(row) for row in res] here?

@@ +108,5 @@
> +        """
> +        res = self.db.execute(select([model.devices.c.mac_address],
> +                                            model.devices.c.name==device_name))
> +        row = res.fetchone()
> +        return row['mac_address']

For consistency this should probably be return row[0].

@@ +189,5 @@
> +                    model.devices.c.last_image_id==model.images.c.id),
> +            whereclause=(model.devices.c.name==device_name)))
> +        row = res.fetchone()
> +        if row:
> +            return {'boot_config': row['boot_config'], 'image': row['name']}

Again could just return dict(row).

@@ +196,5 @@
> +
> +    def set_image(self, device_name, image_name, boot_config):
> +        """
> +        Set the named device's image and boot_config.  The boot_config should
> +        be a JSON string.

Maybe we should make this consistent with other db functions in that it expects a JSON-compatible structure and do the conversion here?  It would also make failures due to invalid data happen earlier (in the rest handlers).  Similarly, get_image() above would then return an object instead of a string.

@@ +206,5 @@
> +        self.db.execute(model.devices.update().
> +                    where(model.devices.c.name==device_name).
> +                    values(last_image_id=image_id,
> +                            boot_config=boot_config))
> +        return config

This line is from before your refactor but makes no sense--it returns a reference to the mozpool.config module. :) Looks like it's left over from some old code (though not sure if it ever made sense); it only continued to work because we happened to import mozpool.config in data.py.  That import (line 8) is no longer needed here either once the above line is removed.

::: mozpool/db/environments.py
@@ +13,5 @@
> +        name.
> +        """
> +        res = self.db.execute(
> +                sqlalchemy.select([sqlalchemy.distinct(model.devices.c.environment)]))
> +        return [ row.environment for row in res.fetchall() ]

Heh this is yet another way of accessing the results of the select.  Again [row[0] for row in res.fetchall()] is consistent with most other uses.

::: mozpool/db/images.py
@@ +22,5 @@
> +        Get information about all visibile images, represented as dictionaries
> +        with keys 'id', 'name', 'boot_config_keys', 'can_reuse', 'hidden', and
> +        'has_sut_agent'.  Raises NotFound if no such image exists.
> +        """
> +        stmt = sqlalchemy.select([model.images])

As with devices.py, select is already being imported directly.  May as well also import not_ directly and remove the sqlalchemy import on line 6.

This is present in a few other files so I'll stop pointing it out now. :)

::: mozpool/db/imaging_servers.py
@@ +17,5 @@
> +                            whereclause=(model.imaging_servers.c.fqdn==fqdn)))
> +        row = res.fetchone()
> +        if not row:
> +            raise exceptions.NotFound
> +        res.close()

Is this necessary?  ResultProxy objects aren't closed elsewhere.

::: mozpool/db/inventorysync.py
@@ +11,5 @@
> +
> +    def dump_devices(self):
> +        """
> +        Dump device data.  This returns a list of dictionaries with keys id, name,
> +        fqdn, invenetory_id, mac_address, imaging_server, relay_info, and state.

Misspelled "inventory".

@@ +48,5 @@
> +        """Delete the device with the given ID"""
> +        # foreign keys don't automatically delete log entries, so do it manually.
> +        # This table is partitioned, so there's no need to later optimize these
> +        # deletes - they'll get flushed when their parititon is dropped.
> +        self.facade.logs.device_logs.delete_all(id)

I don't think self.facade is defined... Should this class inherit from base.ObjectLogsMethodsMixin in order to clear the logs?

@@ +78,5 @@
> +            pass # probably already exists
> +
> +        res = self.db.execute(sqlalchemy.select([ model.imaging_servers.c.id ],
> +                            whereclause=(model.imaging_servers.c.fqdn==name)))
> +        return res.fetchall()[0].id

Consistency again dictates return res.fetchall()[0][0] although in this case I can see the appeal of the above.

::: mozpool/db/pxe_configs.py
@@ +14,5 @@
> +        q = select([model.pxe_configs.c.name])
> +        if active_only:
> +            q = q.where(model.pxe_configs.c.active)
> +        res = self.db.execute(q)
> +        return [row[0].encode('utf-8') for row in res]

Noticed you did away with a lot of the UTF-8 business.  Is it required here?

@@ +47,5 @@
> +                [ {'name':name, 'description':description, 'active':active, 'contents':contents} ])
> +
> +    def update(self, name, description=None, active=None, contents=None):
> +        """
> +        Update the given PXE config with teh given parameters.  Unspecified

Typo: "teh"

::: mozpool/lifeguard/inventorysync.py
@@ +6,4 @@
>  import re
>  import argparse
>  import requests
> +from mozpool.db import facade

I think this is supposed to be just "import mozpool.db" (and see below).

@@ +131,4 @@
>                          help='verbose output')
>      args = parser.parse_args()
>  
> +    db = facade.Facade()

Should be "db = mozpool.db.setup()".

::: mozpool/mozpool/handlers.py
@@ +36,4 @@
>          except (KeyError, ValueError):
>              raise web.badrequest()
>  
> +        image = self.db.images.get(image_name)

Think it's worth catching exceptions and returning better errors (like Bad Request or Not Found) rather than defaulting to 500 errors?  I'm certainly not super picky about "correct" pedantic REST interfaces, but I like to avoid 500s if possible.  I see we still do that in request_details below... although not in request_status.  We should probably be consistent here. :)

::: mozpool/statedriver.py
@@ +136,5 @@
> +        # pick the appropriate sub-object of db based on object_type
> +        self.db_methods = {
> +            'request' : db.requests,
> +            'device' : db.devices,
> +        }[self.object_type]

Hm I guess this is one side effect of passing around the db object--the DBHandler class now has to be aware of its subclasses to some degree.  But I can't think of a better way aside from maybe doing an attribute lookup...

::: mozpool/web/server.py
@@ +18,1 @@
>  from mozpool.mozpool import requestmachine, handlers as mozpool_handlers

mozpool.mozpool is entirely imported on line 14 and then pieces imported here directly. Same with mozpool.lifeguard above.  In both cases, only the driver is used from the full import (e.g. mozpool.lifeguard.driver and mozpool.mozpool.driver), so maybe both of those could be imported specifically and the full imports removed.

::: runtests.py
@@ +27,5 @@
>  from mozpool.bmm import relay
>  from mozpool.bmm import pxe
>  from mozpool.bmm import ping
>  from mozpool.bmm import scripts
> +from mozpool.db import model

Again we're importing mozpool.db but then importing model from mozpool.db, which strikes me as weird.  Is it just for convenience?

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

The facade stuff was missed due to the lack of tests - I caught it in the subsequent patch to add tests.

The DBHandler issue is weird, but my justification is that it's a parent class to reduce code duplication, rather than as an abstraction, so this is excusable.  I suppose I could also add a subclass method.

I tend to never import things with dots in them (e.g., import mozpool.db.model).  The exception is where I need a global variable from such a module - driver, in this case.  I don't like those driver globals, but I didn't fix them in this patch.  Runtests goes away in the next patch, so I won't stress about the imports there too much.

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

Oh, and regarding boot_config, the code is pretty consistent about treating it as a blob throughout (except, of course, in the shell scripts on the pandas).  I don't see any strong reason to change that right now.

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: interdiff.patch

Updates as requested.  Rather than do things the same way everywhere, I factored "the same way" into utility methods.

I think we should do a pep8 cleanup when everything's landed.  I have a stack of 6 patches pending now, so this is not a good time.

Comment 7

[Mark Côté [:mcote]]

Comment on attachment 719523 [details] [diff] [review]
interdiff.patch

Nice!