Description of problem: Key value import gives 500 error for undefined systems Version-Release number of selected component (if applicable): develop How reproducible: easy to reproduce Steps to Reproduce: 1. Setup a new beaker server, and add one lab-controller to this server. Now there's no machine. 2. Import CSV file which contains key/value. Actual results: It shows ISE500. Jan 26 21:01:11 beaker beaker-server[22402]: cherrypy.msg INFO HTTP: Page handler: <bound method CSV.action_import of <bkr.server.CSV_import_export.CSV object at 0x7fee6f2c4e50>> Jan 26 21:01:11 beaker beaker-server[22402]: Traceback (most recent call last): Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib/python2.6/site-packages/CherryPy-2.3.0-py2.6.egg/cherrypy/_cphttptools.py", line 121, in _run Jan 26 21:01:11 beaker beaker-server[22402]: self.main() Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib/python2.6/site-packages/CherryPy-2.3.0-py2.6.egg/cherrypy/_cphttptools.py", line 264, in main Jan 26 21:01:11 beaker beaker-server[22402]: body = page_handler(*virtual_path, **self.params) Jan 26 21:01:11 beaker beaker-server[22402]: File "<string>", line 3, in action_import Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib/python2.6/site-packages/turbogears/controllers.py", line 361, in expose Jan 26 21:01:11 beaker beaker-server[22402]: *args, **kw) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib/python2.6/site-packages/bkr/server/wsgi.py", line 54, in run_with_transaction_noop Jan 26 21:01:11 beaker beaker-server[22402]: return func(*args, **kwargs) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib/python2.6/site-packages/turbogears/controllers.py", line 244, in _expose Jan 26 21:01:11 beaker beaker-server[22402]: @abstract() Jan 26 21:01:11 beaker beaker-server[22402]: File "<generated code>", line 0, in _expose Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib/python2.6/site-packages/peak/rules/core.py", line 153, in __call__ Jan 26 21:01:11 beaker beaker-server[22402]: return self.body(*args, **kw) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib/python2.6/site-packages/turbogears/controllers.py", line 390, in <lambda> Jan 26 21:01:11 beaker beaker-server[22402]: fragment, options, args, kw))) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib/python2.6/site-packages/turbogears/controllers.py", line 425, in _execute_func Jan 26 21:01:11 beaker beaker-server[22402]: output = errorhandling.try_call(func, *args, **kw) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib/python2.6/site-packages/turbogears/errorhandling.py", line 77, in try_call Jan 26 21:01:11 beaker beaker-server[22402]: return func(self, *args, **kw) Jan 26 21:01:11 beaker beaker-server[22402]: File "<string>", line 2, in action_import Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib/python2.6/site-packages/bkr/server/identity.py", line 242, in require Jan 26 21:01:11 beaker beaker-server[22402]: return func(*args, **kwargs) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib/python2.6/site-packages/bkr/server/CSV_import_export.py", line 161, in action_import Jan 26 21:01:11 beaker beaker-server[22402]: self.from_csv(system, data, log) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib/python2.6/site-packages/bkr/server/CSV_import_export.py", line 230, in from_csv Jan 26 21:01:11 beaker beaker-server[22402]: csv_types[csv_type]._from_csv(system, data, csv_type, log) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib/python2.6/site-packages/bkr/server/CSV_import_export.py", line 814, in _from_csv Jan 26 21:01:11 beaker beaker-server[22402]: session.flush([key_value]) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib64/python2.6/site-packages/sqlalchemy/orm/scoping.py", line 139, in do Jan 26 21:01:11 beaker beaker-server[22402]: return getattr(self.registry(), name)(*args, **kwargs) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib64/python2.6/site-packages/sqlalchemy/orm/session.py", line 1400, in flush Jan 26 21:01:11 beaker beaker-server[22402]: self._flush(objects) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib64/python2.6/site-packages/sqlalchemy/orm/session.py", line 1481, in _flush Jan 26 21:01:11 beaker beaker-server[22402]: flush_context.execute() Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib64/python2.6/site-packages/sqlalchemy/orm/unitofwork.py", line 302, in execute Jan 26 21:01:11 beaker beaker-server[22402]: rec.execute(self) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib64/python2.6/site-packages/sqlalchemy/orm/unitofwork.py", line 446, in execute Jan 26 21:01:11 beaker beaker-server[22402]: uow Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib64/python2.6/site-packages/sqlalchemy/orm/mapper.py", line 1887, in _save_obj Jan 26 21:01:11 beaker beaker-server[22402]: execute(statement, params) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib64/python2.6/site-packages/sqlalchemy/engine/base.py", line 1191, in execute Jan 26 21:01:11 beaker beaker-server[22402]: params) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib64/python2.6/site-packages/sqlalchemy/engine/base.py", line 1271, in _execute_clauseelement Jan 26 21:01:11 beaker beaker-server[22402]: return self.__execute_context(context) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib64/python2.6/site-packages/sqlalchemy/engine/base.py", line 1302, in __execute_context Jan 26 21:01:11 beaker beaker-server[22402]: context.parameters[0], context=context) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib64/python2.6/site-packages/sqlalchemy/engine/base.py", line 1401, in _cursor_execute Jan 26 21:01:11 beaker beaker-server[22402]: context) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib64/python2.6/site-packages/sqlalchemy/engine/base.py", line 1394, in _cursor_execute Jan 26 21:01:11 beaker beaker-server[22402]: context) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib64/python2.6/site-packages/sqlalchemy/engine/default.py", line 299, in do_execute Jan 26 21:01:11 beaker beaker-server[22402]: cursor.execute(statement, parameters) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib64/python2.6/site-packages/MySQLdb/cursors.py", line 173, in execute Jan 26 21:01:11 beaker beaker-server[22402]: self.errorhandler(self, exc, value) Jan 26 21:01:11 beaker beaker-server[22402]: File "/usr/lib64/python2.6/site-packages/MySQLdb/connections.py", line 36, in defaulterrorhandler Jan 26 21:01:11 beaker beaker-server[22402]: raise errorclass, errorvalue Jan 26 21:01:11 beaker beaker-server[22402]: OperationalError: (OperationalError) (1048, "Column 'system_id' cannot be null") 'INSERT INTO key_value_int (system_id, key_id, key_value) VALUES (%s, %s, %s)' (None, 3L, '6') Jan 26 21:01:11 beaker beaker-server[22402]: bkr.server.wsgi DEBUG Rolling back for 500 response Expected results: Additional info:
I am wondering about the correct fix for this. I see two options: - Fail to import with an error that the system doesn't exit - Create the system like we do for others and populate it with the key value.
For all of the CSV types other than "Systems" and "Systems (for modification)", failing with a 422 Unprocessable Entity for unknown FQDN entries would be ideal (and the same goes for unknown non-empty system IDs in the latter case). However, due to the current limitations of the CSV import page, a 200-with-an-error-message response may be the best we can do for now (that's what is currently done to report CSV import failures and it isn't worth investing in changing that just to resolve this bug). Also, ensure that the handling of missing FQDNs is tested for all of the other CSV import types. Implicitly creating missing systems is acceptable (it isn't worth the hassle of deprecating that behaviour in cases where it already happens), but any other cases where it currently triggers 500 errors should also be updated to report a proper error message.
It turns out that the CSV import for non-existent systems creates the system if it doesn't exist for all the other import types: labinfo, power, install, exclude, groups barring keyvalue. It seems to me that there may have been an oversight in the key value import code. So, I have taken the route of fixing this oversight rather than barring it, so that it is consistent with the others. Preliminary patch: http://gerrit.beaker-project.org/#/c/2759/1
OK, after reviewing Amit's patch, it's (much) clearer that the original intent of the CSV import process was to always implicitly create the system if it wasn't already there, regardless of the kind of CSV record involved. While I still have reservations about that design choice (although it does have the benefit of making a full import of a particular system independent of the order of the rows), I agree with Amit that the correct solution at this point in time is to simply bring the Key/Value import into line with the handling of other system rows, rather than moving to an approach where there is a "primary" system row type that must be provided first in order to create a new system.
Beaker 0.15.4 has been released.