797826 - Write script to scrape pandaboard data from inventory into imaging service DB

Status: RESOLVED FIXED

(Infrastructure & Operations :: RelOps: General, task)

Description

[(not currently active) Ted Mielczarek]

Some of the database data for the imaging service needs to come from inventory. Specifically we'll be populating the boards table with the following fields from inventory:
  name varchar(32) not null,
  fqdn varchar(256) not null,
  inventory_id integer not null,
  mac_address varchar(10) not null,
  imaging_server varchar(256) not null,
  relay_info varchar(64)

Presumably we'll also need to populate the imaging_servers table from inventory, which is just:
  fqdn varchar(256) not null primary key

I'm pretty sure this info isn't actually complete in inventory yet. Jake: if there's a bug on getting that data into inventory that should block this one.

Comment 1

[Amy Rich [:arr] [:arich]]

That data is now in inventory for all the pandas in the production chassis.

Comment 2

[Amy Rich [:arr] [:arich]]

ted: if you don't get to this by monday, dustin can work on it (and has some experience with the inventory API).

Comment 3

[(not currently active) Ted Mielczarek]

I'm probably still going to be tied up with the REST api bits, so if he knows the inventory API then I'm happy to let him take care of it.

Comment 4

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

Attachment: bug797826-part1.patch

Here's the first part:
 - downloading inventory results (use pattern 'hostname__startswith=panda-')
 - merging inventory results between db and inventory

I need to figure out the sqlalchemy stuff to do the last bit - actually inserting into the DB.  So I'll put that in a second patch.

Comment 5

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

Attachment: bug797826-crud.patch

part two: crud methods.

Comment 6

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

Attachment: bug797826-actually-sync.patch

This should do it.  I'll take care of merging these when necessary (we've diverged a good bit).

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 669272 [details] [diff] [review]
bug797826-part1.patch

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

Looks good, just a couple of nits.

::: bmm/invsync.py
@@ +1,1 @@
> +#!/usr/bin/env python

I'm a sucker for more descriptive names, can we name this "inventorysync" instead?

@@ +46,5 @@
> +                name=name,
> +                fqdn=hostname,
> +                inventory_id=o['id'],
> +                mac_address=mac_address,
> +                imaging_server=kv.get('system.imaging_server.0', 'UNKNOWN'),

Seems like we might want to require this, otherwise the API will refuse to actually let you boot this board.

@@ +50,5 @@
> +                imaging_server=kv.get('system.imaging_server.0', 'UNKNOWN'),
> +                relay_info=kv['system.relay.0'])
> +
> +            if verbose: print hostname, 'done.'
> +        

nit: Trailing whitespace

@@ +58,5 @@
> +def merge_hosts(from_db, from_inv):
> +    """
> +    Merge a list of hosts in the DB with those in inventory.  This yields a
> +    list of instructions of the form ('insert', dict), ('delete', id), or
> +    (update, id, dict).

Nit: missing quotes: 'update'.

::: runtests.py
@@ +269,5 @@
> +        inventory_id = hash(fqdn) % 100
> +        kv = []
> +        if mac_address:
> +            mac_address = hashlib.md5(fqdn).digest()[:6]
> +            mac_address = ':'.join([ '%02x' % ord(b) for b in mac_address ])

Reusing the boolean parameters as string values here confused me a little, FWIW.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 669574 [details] [diff] [review]
bug797826-crud.patch

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

::: bmm/data.py
@@ +54,5 @@
>                                from_obj=[model.boards.join(model.imaging_servers, model.imaging_servers.c.fqdn == server)]))
>      return {'boards': [row[0].encode('utf-8') for row in res]}
>  
> +def dump_boards():
> +    """Dump all boards for inventory sync."""

I guess my docstring formatting is inconsistent and I should be putting one-line docstrings with the quotes on the same line like this.

@@ +61,5 @@
> +    res = conn.execute(j.select(use_labels=True))
> +    return [ dict(id=row.boards_id, name=row.boards_name, fqdn=row.boards_fqdn,
> +                  inventory_id=row.boards_inventory_id, mac_address=row.boards_mac_address,
> +                  imaging_server=row.imaging_servers_fqdn, relay_info=row.boards_relay_info)
> +             for row in res ]

You probably want to use row_to_dict here (although you might have to tweak it to handle >1 table).

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

I think elsewhere I just use .fetchone() when I'm expecting one result.

@@ +78,5 @@
> +    res = conn.execute(sqlalchemy.select([ model.imaging_servers.c.id ],
> +                        whereclause=(model.imaging_servers.c.fqdn==name)))
> +    return res.fetchall()[0].id
> +
> +def insert_board(values):

Can you adjust testing.py's add_server/add_board to use these methods?

@@ +88,5 @@
> +    values['imaging_server_id'] = find_imaging_server_id(values.pop('imaging_server'))
> +    values['status'] = 'new'
> +
> +    conn = get_conn()
> +    conn.execute(model.boards.insert(), [ values ])

I've just been doing get_conn().execute for when I don't need to execute multiple statements.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 669639 [details] [diff] [review]
bug797826-actually-sync.patch

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

::: bmm/config.py
@@ +14,5 @@
>  config_read = False
>  _db_engine = None
> +_inventory_url = None
> +_inventory_username = None
> +_inventory_password = None

Sorry, this file got real ugly real fast. Should have built a better API in the first place.

::: bmm/data.py
@@ +21,5 @@
>      """
>      global engine
>      if engine is None:
>          engine = sqlalchemy.create_engine(config.db_engine())
> +        model.metadata.create_all(bind=engine)

This obviates testing.create_db_schema. I didn't realize this was safe to do if the schema already existed.

::: bmm/invsync.py
@@ +68,5 @@
>      from_inv = dict([ (r['inventory_id'], r) for r in from_inv ])
>  
>      # get the insert and deletes out of the way
>      for invid in set(from_db) - set(from_inv):
> +        yield ('delete', from_db[invid]['id'], from_db[invid])

You changed the format of the 'delete' results but didn't update the docstring.

@@ +84,5 @@
> +def sync(verbose=False):
> +    from_db = data.dump_boards()
> +    from_inv = get_boards(
> +            config.inventory_url(),
> +            'hostname__startswith=panda-',

Probably put a TODO here for improving this.

@@ +105,5 @@
> +
> +def main():
> +    parser = argparse.ArgumentParser(description='Sync BMM with inventory.')
> +    parser.add_argument('--verbose', action='store_true',
> +                               default=False,

Indentation is a little wonky here.

Comment 10

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

(In reply to Ted Mielczarek [:ted] from comment #7)
> Review of attachment 669272 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just a couple of nits.

All accounted for.

(In reply to Ted Mielczarek [:ted] from comment #8)
> Review of attachment 669574 [details] [diff] [review]:
> -----------------------------------------------------------------
> I guess my docstring formatting is inconsistent and I should be putting
> one-line docstrings with the quotes on the same line like this.

I honestly wasn't paying much attention..

> You probably want to use row_to_dict here (although you might have to tweak
> it to handle >1 table).

That actually adds quite a bit of complexity to row_to_dict.  But I made is suck less.

> I think elsewhere I just use .fetchone() when I'm expecting one result.

This leaves the ResultSet (and underlying cursor) open, which can cause unexpected behavior.  fetchall has the advantage of automatically closing the ResultSet.

> Can you adjust testing.py's add_server/add_board to use these methods?

insert_board doesn't insert into all of the columns, so I don't think that necessarily makes sense.  Also, it confuses the system under test with the test harness, IMHO.

> I've just been doing get_conn().execute for when I don't need to execute
> multiple statements.

fixed.

(In reply to Ted Mielczarek [:ted] from comment #9)
> Review of attachment 669639 [details] [diff] [review]:
> -----------------------------------------------------------------
> This obviates testing.create_db_schema. I didn't realize this was safe to do
> if the schema already existed.

Ack, that was testing code, and should have been removed!

(remainder fixed)

Comment 11

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

All checked in, merged, and pushed to ted's user repo.

Comment 12

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

Ted, I also landed the config refactor on your repo, FYI.

Comment 13

[(not currently active) Ted Mielczarek]

(In reply to Dustin J. Mitchell [:dustin] from comment #10)
> > I think elsewhere I just use .fetchone() when I'm expecting one result.
> 
> This leaves the ResultSet (and underlying cursor) open, which can cause
> unexpected behavior.  fetchall has the advantage of automatically closing
> the ResultSet.

Ah, we should probably be consistent about that usage then (even if it's not a problem in practice).

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 669574 [details] [diff] [review]
bug797826-crud.patch

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

::: bmm/data.py
@@ +94,5 @@
> +def delete_board(id):
> +    """Delete the board with the given ID"""
> +    conn = get_conn()
> +
> +    conn.execute(model.boards.delete(), whereclause=(model.boards.c.id==id))

I missed this on the first pass, but you probably want to delete the data from the logs table as well, right? (Or do we get cascading delete for free?)

Comment 15

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

Good point.  We don't get that for free because MySQL can't do foreign keys on partitioned tables.

Comment 16

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

Attachment: delete-logs.patch

In my branch at http://hg.mozilla.org/users/dmitchell_mozilla.com/reimaging-service/rev/800cfb608649

Merge it unless you see a problem?

Comment 17

[(not currently active) Ted Mielczarek]

http://hg.mozilla.org/users/tmielczarek_mozilla.com/reimaging-service/rev/800cfb608649