)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"0f3f5dff7beb5125bd864774b22c3a5c5299ed95","unresolved":true,"context_lines":[{"line_number":12,"context_line":"request_level_params field to None breaking the non nullable invariant"},{"line_number":13,"context_line":"of the field."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"So far this is error is not triggered as the request_level_params field"},{"line_number":16,"context_line":"was only used lazy loading, which is defaulted the field to a new"},{"line_number":17,"context_line":"RequestLevelParam object on the RequestSpec object that was never saved after"},{"line_number":18,"context_line":"such lazy load. A later patch will initialize request_level_params field"},{"line_number":19,"context_line":"in the RequestSpec.from_components() factory method to handle this"},{"line_number":20,"context_line":"parameters from Neutron ports and that triggers a failure when the"},{"line_number":21,"context_line":"RequestSpec is saved."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"8d85fd75_35882430","line":18,"range":{"start_line":15,"start_character":1,"end_line":18,"end_character":15},"updated":"2021-05-17 13:50:20.000000000","message":"Yep, stored as None in the DB, but never loaded normally, and lazy-load just gives us a fresh object. You\u0027re definitely right that storing the None isn\u0027t correct since it\u0027s not nullable, but... I guess something later in the stack changes the loading behavior and becomes a problem? Because, of course, lots of people have RequestSpecs with null\u0027d request_level_params in them, so we still have to handle that going forward.","commit_id":"67c2d86b10aa87e6657e12641e05d0aae0ccff9c"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"968fffaca25c473cadd57cf9816c3a9fa7242192","unresolved":true,"context_lines":[{"line_number":12,"context_line":"request_level_params field to None breaking the non nullable invariant"},{"line_number":13,"context_line":"of the field."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"So far this is error is not triggered as the request_level_params field"},{"line_number":16,"context_line":"was only used lazy loading, which is defaulted the field to a new"},{"line_number":17,"context_line":"RequestLevelParam object on the RequestSpec object that was never saved after"},{"line_number":18,"context_line":"such lazy load. A later patch will initialize request_level_params field"},{"line_number":19,"context_line":"in the RequestSpec.from_components() factory method to handle this"},{"line_number":20,"context_line":"parameters from Neutron ports and that triggers a failure when the"},{"line_number":21,"context_line":"RequestSpec is saved."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"e78aec44_36d27149","line":18,"range":{"start_line":15,"start_character":1,"end_line":18,"end_character":15},"in_reply_to":"8d85fd75_35882430","updated":"2021-05-17 16:05:59.000000000","message":"See my reply to your other comment. I don\u0027t think we have a problem.","commit_id":"67c2d86b10aa87e6657e12641e05d0aae0ccff9c"}],"nova/objects/request_spec.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"0f3f5dff7beb5125bd864774b22c3a5c5299ed95","unresolved":true,"context_lines":[{"line_number":674,"context_line":"                spec.instance_group.hosts \u003d None"},{"line_number":675,"context_line":"            # NOTE(mriedem): Don\u0027t persist these since they are per-request"},{"line_number":676,"context_line":"            for excluded in (\u0027retry\u0027, \u0027requested_destination\u0027,"},{"line_number":677,"context_line":"                             \u0027requested_resources\u0027, \u0027ignore_hosts\u0027):"},{"line_number":678,"context_line":"                if excluded in spec and getattr(spec, excluded):"},{"line_number":679,"context_line":"                    setattr(spec, excluded, None)"},{"line_number":680,"context_line":"            # NOTE(stephenfin): Don\u0027t persist network metadata since we have"}],"source_content_type":"text/x-python","patch_set":2,"id":"f06016c5_f54f46b4","line":677,"updated":"2021-05-17 13:50:20.000000000","message":"I wonder if it\u0027s worth a NOTE() here about how r-l-p *used* to be in this class and thus some people may have a null\u0027d value for a long time?","commit_id":"67c2d86b10aa87e6657e12641e05d0aae0ccff9c"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"860c4ada9fb28f30bc8e56afa5c4fe2b311b6f1f","unresolved":true,"context_lines":[{"line_number":674,"context_line":"                spec.instance_group.hosts \u003d None"},{"line_number":675,"context_line":"            # NOTE(mriedem): Don\u0027t persist these since they are per-request"},{"line_number":676,"context_line":"            for excluded in (\u0027retry\u0027, \u0027requested_destination\u0027,"},{"line_number":677,"context_line":"                             \u0027requested_resources\u0027, \u0027ignore_hosts\u0027):"},{"line_number":678,"context_line":"                if excluded in spec and getattr(spec, excluded):"},{"line_number":679,"context_line":"                    setattr(spec, excluded, None)"},{"line_number":680,"context_line":"            # NOTE(stephenfin): Don\u0027t persist network metadata since we have"}],"source_content_type":"text/x-python","patch_set":2,"id":"ac9ad628_2be2935b","line":677,"in_reply_to":"15cc130a_226ac09a","updated":"2021-05-18 07:34:44.000000000","message":"I think it r-l-p was never set / never been dirty when we saved RequestSpec. As soon as I set it during RequestSpec creation in a later patch[1], the RequestSpec.create() started failing \n\n[1] https://review.opendev.org/c/openstack/nova/+/791506/2/nova/objects/request_spec.py#546","commit_id":"67c2d86b10aa87e6657e12641e05d0aae0ccff9c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"c062b3d9535e4599533b900593f855bfb52a3fb3","unresolved":true,"context_lines":[{"line_number":674,"context_line":"                spec.instance_group.hosts \u003d None"},{"line_number":675,"context_line":"            # NOTE(mriedem): Don\u0027t persist these since they are per-request"},{"line_number":676,"context_line":"            for excluded in (\u0027retry\u0027, \u0027requested_destination\u0027,"},{"line_number":677,"context_line":"                             \u0027requested_resources\u0027, \u0027ignore_hosts\u0027):"},{"line_number":678,"context_line":"                if excluded in spec and getattr(spec, excluded):"},{"line_number":679,"context_line":"                    setattr(spec, excluded, None)"},{"line_number":680,"context_line":"            # NOTE(stephenfin): Don\u0027t persist network metadata since we have"}],"source_content_type":"text/x-python","patch_set":2,"id":"15cc130a_226ac09a","line":677,"in_reply_to":"c59dfa3f_be6a1af2","updated":"2021-05-17 16:30:21.000000000","message":"Ah, I was thinking \u0027spec\u0027 here was the dict we\u0027re building up (which obviously doesn\u0027t make sense with setattr anyway), and thus we were setting None for this value headed to the DB. However, I see that\u0027s done on L693 and we\u0027re operating on a clone of the object here.\n\nI guess I\u0027m not sure why this wasn\u0027t being triggered when we went to save the object, but maybe it wasn\u0027t dirty (or set?) at the time and thus wasn\u0027t in \u0027updates\u0027. Anyway, doesn\u0027t matter because as you say, we\u0027re not actually storing it this way.","commit_id":"67c2d86b10aa87e6657e12641e05d0aae0ccff9c"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"968fffaca25c473cadd57cf9816c3a9fa7242192","unresolved":true,"context_lines":[{"line_number":674,"context_line":"                spec.instance_group.hosts \u003d None"},{"line_number":675,"context_line":"            # NOTE(mriedem): Don\u0027t persist these since they are per-request"},{"line_number":676,"context_line":"            for excluded in (\u0027retry\u0027, \u0027requested_destination\u0027,"},{"line_number":677,"context_line":"                             \u0027requested_resources\u0027, \u0027ignore_hosts\u0027):"},{"line_number":678,"context_line":"                if excluded in spec and getattr(spec, excluded):"},{"line_number":679,"context_line":"                    setattr(spec, excluded, None)"},{"line_number":680,"context_line":"            # NOTE(stephenfin): Don\u0027t persist network metadata since we have"}],"source_content_type":"text/x-python","patch_set":2,"id":"c59dfa3f_be6a1af2","line":677,"in_reply_to":"f06016c5_f54f46b4","updated":"2021-05-17 16:05:59.000000000","message":"The thing is that if r-l-p field was set to any value in the obj and then obj.create() or obj.save() was called then the code blown at L679 as None cannot be set to a non nullable ovo field. You can simulate that by reverting this change and running the new test extended testcase. \n\nSo we know that there is no RequestSpec saved in the DB where the None value is stored. Every RequestSpec instance actually missing the r-l-p field in the DB. And the lazy load behavior hides that it is missing when the field is accessed.\n\nI just looked into my devstack nova_api db for an example: http://paste.openstack.org/show/805436/","commit_id":"67c2d86b10aa87e6657e12641e05d0aae0ccff9c"},{"author":{"_account_id":7634,"name":"Takashi Natsume","email":"takanattie@gmail.com","username":"natsumet"},"change_message_id":"8594c82c4f006345158595c169a44af738d36727","unresolved":true,"context_lines":[{"line_number":685,"context_line":"            # no need for it after scheduling"},{"line_number":686,"context_line":"            if \u0027requested_networks\u0027 in spec and spec.requested_networks:"},{"line_number":687,"context_line":"                del spec.requested_networks"},{"line_number":688,"context_line":"            # NOTE(gibi): Don\u0027t persist requested_networks since we have"},{"line_number":689,"context_line":"            # no need for it after scheduling"},{"line_number":690,"context_line":"            if \u0027request_level_params\u0027 in spec and spec.request_level_params:"},{"line_number":691,"context_line":"                del spec.request_level_params"}],"source_content_type":"text/x-python","patch_set":2,"id":"b810a799_e4df463b","line":688,"range":{"start_line":688,"start_character":40,"end_line":688,"end_character":58},"updated":"2021-05-18 12:37:31.000000000","message":"request_level_params","commit_id":"67c2d86b10aa87e6657e12641e05d0aae0ccff9c"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"c48088954ac677b154d436241ae92cda1ecb2431","unresolved":true,"context_lines":[{"line_number":685,"context_line":"            # no need for it after scheduling"},{"line_number":686,"context_line":"            if \u0027requested_networks\u0027 in spec and spec.requested_networks:"},{"line_number":687,"context_line":"                del spec.requested_networks"},{"line_number":688,"context_line":"            # NOTE(gibi): Don\u0027t persist requested_networks since we have"},{"line_number":689,"context_line":"            # no need for it after scheduling"},{"line_number":690,"context_line":"            if \u0027request_level_params\u0027 in spec and spec.request_level_params:"},{"line_number":691,"context_line":"                del spec.request_level_params"}],"source_content_type":"text/x-python","patch_set":2,"id":"9cc27511_7f93895a","line":688,"range":{"start_line":688,"start_character":40,"end_line":688,"end_character":58},"in_reply_to":"b810a799_e4df463b","updated":"2021-05-19 16:30:32.000000000","message":"When you\u0027re (hopefully) fixing this in a follow-up, let\u0027s maybe take the opportunity to remove all the duplication here since three users of a pattern probably suggests it should be made more generic","commit_id":"67c2d86b10aa87e6657e12641e05d0aae0ccff9c"}]}