Code review comment for lp:lava-dashboard/multinode

On Thu, 22 Aug 2013 01:27:18 -0000
Antonio Terceiro <email address hidden> wrote:

> > + def put_pending(self, content, group_name):
> > + try:
> > + # add this to a list which put_group can use.
> > + sha1 = hashlib.sha1()
> > + sha1.update(content)
> > + hexdigest = sha1.hexdigest()
> > + groupfile = "/tmp/%s" % group_name
> > + with open(groupfile, "a+") as grp_file:
> > + grp_file.write("%s\n" % content)
> > + return hexdigest
> > + except Exception as e:
> > + logging.debug("Dashboard pending submission caused an
> > exception: %s" % e)
>
> Is there a race condition here?

Possibly, only within that one group. The Coordinator already ensures
that put_group waits for a fixed period to allow the last put_pending
for this group to complete but large groups could have a problem here.
(There is a reboot into the master image between the last possible sync
operation and the XMLRPC call too.)

> It's fine for two or more processes
> to append to the same file, but it's possible that depending on the
> size of the bundles and on line buffering issues the contents of
> different bundles might get intermingled. Maybe we should write each
> bundle to its own separate file, then read them all on put_group.

> Also, I miss some sort of authentication to avoid the risk of having
> attackers submitting random crap into bundle streams for multinode
> groups. I guess put_group already handles authentication because it
> uses the underlying bundle stream access control?

put_group uses authentication, yes.

I'll have a look at some of the checks in _put - what we don't want is
to use the full _put function which causes the creation and
deserialization of the pending bundle.

--

Neil Williams
=============
http://www.linux.codehelp.co.uk/