)]}'
{"specs/newton/approved/libvirt-instance-storage.rst":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ebae625fae688eadc076ca40ca7708cea974d3ac","unresolved":false,"context_lines":[{"line_number":17,"context_line":"between compute nodes in the same installation, or over time after"},{"line_number":18,"context_line":"installation. By storing storage metadata of a particular instance explicitly"},{"line_number":19,"context_line":"we make its configuration unambiguous and simple to understand, and therefore"},{"line_number":20,"context_line":"less vulnerability to security vulnerabilities. We also allow multiple"},{"line_number":21,"context_line":"configurations to exist within an installation, and lay a foundation to allow"},{"line_number":22,"context_line":"the robust transition between different storage layouts."},{"line_number":23,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_3c8fba76","line":20,"range":{"start_line":20,"start_character":5,"end_line":20,"end_character":46},"updated":"2016-05-03 14:21:59.000000000","message":"This is a nit, but perhaps:\n\n less vulnerable to security issues","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f55f2083cdbf612961feb9282cc33c868603cb99","unresolved":false,"context_lines":[{"line_number":17,"context_line":"between compute nodes in the same installation, or over time after"},{"line_number":18,"context_line":"installation. By storing storage metadata of a particular instance explicitly"},{"line_number":19,"context_line":"we make its configuration unambiguous and simple to understand, and therefore"},{"line_number":20,"context_line":"less vulnerability to security vulnerabilities. We also allow multiple"},{"line_number":21,"context_line":"configurations to exist within an installation, and lay a foundation to allow"},{"line_number":22,"context_line":"the robust transition between different storage layouts."},{"line_number":23,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_7e942e36","line":20,"range":{"start_line":20,"start_character":5,"end_line":20,"end_character":46},"in_reply_to":"1a122d0e_3c8fba76","updated":"2016-05-05 10:15:31.000000000","message":"Done","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ebae625fae688eadc076ca40ca7708cea974d3ac","unresolved":false,"context_lines":[{"line_number":18,"context_line":"installation. By storing storage metadata of a particular instance explicitly"},{"line_number":19,"context_line":"we make its configuration unambiguous and simple to understand, and therefore"},{"line_number":20,"context_line":"less vulnerability to security vulnerabilities. We also allow multiple"},{"line_number":21,"context_line":"configurations to exist within an installation, and lay a foundation to allow"},{"line_number":22,"context_line":"the robust transition between different storage layouts."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_dc468620","line":21,"updated":"2016-05-03 14:21:59.000000000","message":"Did you mean \"with this change we /can/ allow multiple configurations?","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f55f2083cdbf612961feb9282cc33c868603cb99","unresolved":false,"context_lines":[{"line_number":18,"context_line":"installation. By storing storage metadata of a particular instance explicitly"},{"line_number":19,"context_line":"we make its configuration unambiguous and simple to understand, and therefore"},{"line_number":20,"context_line":"less vulnerability to security vulnerabilities. We also allow multiple"},{"line_number":21,"context_line":"configurations to exist within an installation, and lay a foundation to allow"},{"line_number":22,"context_line":"the robust transition between different storage layouts."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_9e2b0aeb","line":21,"in_reply_to":"1a122d0e_dc468620","updated":"2016-05-05 10:15:31.000000000","message":"Hmm, reading this again it\u0027s correct but very far from clear. I meant to say that multiple could always exist through misconfiguration, but now we can accurately represent them. They\u0027ll still be broken, though.\n\nWill clean this up.","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ebae625fae688eadc076ca40ca7708cea974d3ac","unresolved":false,"context_lines":[{"line_number":36,"context_line":"  another compute node with different values for these config variables, or"},{"line_number":37,"context_line":"  which was created before the values of these config variables was changed,"},{"line_number":38,"context_line":"  will be incorrectly handled. This will lead to failure at best. At worst it"},{"line_number":39,"context_line":"  will create a security vulnerability."},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"- The imagebackend code uses a single method, cache(), to create both disks"},{"line_number":42,"context_line":"  from glance images, and disks from templates (i.e. blank filesystems or swap"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_3c197a23","line":39,"updated":"2016-05-03 14:21:59.000000000","message":"...and has.\n\nJust to be clear, are you saying that if I create a couple instances and then change either or both of these configs on that compute node, that things can break for the existing instance? If so..that sounds bad.","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9bc2ed0164e03b47dcd469697ba0aaca2e57c5fa","unresolved":false,"context_lines":[{"line_number":36,"context_line":"  another compute node with different values for these config variables, or"},{"line_number":37,"context_line":"  which was created before the values of these config variables was changed,"},{"line_number":38,"context_line":"  will be incorrectly handled. This will lead to failure at best. At worst it"},{"line_number":39,"context_line":"  will create a security vulnerability."},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"- The imagebackend code uses a single method, cache(), to create both disks"},{"line_number":42,"context_line":"  from glance images, and disks from templates (i.e. blank filesystems or swap"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_73ff37ab","line":39,"in_reply_to":"1a122d0e_01115f52","updated":"2016-05-05 15:02:03.000000000","message":"In \u0027merica, we say \"bubble gum and duct tape\".","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f55f2083cdbf612961feb9282cc33c868603cb99","unresolved":false,"context_lines":[{"line_number":36,"context_line":"  another compute node with different values for these config variables, or"},{"line_number":37,"context_line":"  which was created before the values of these config variables was changed,"},{"line_number":38,"context_line":"  will be incorrectly handled. This will lead to failure at best. At worst it"},{"line_number":39,"context_line":"  will create a security vulnerability."},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"- The imagebackend code uses a single method, cache(), to create both disks"},{"line_number":42,"context_line":"  from glance images, and disks from templates (i.e. blank filesystems or swap"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_01115f52","line":39,"in_reply_to":"1a122d0e_3c197a23","updated":"2016-05-05 10:15:31.000000000","message":"Yes, that\u0027s exactly the current situation. We have a sticky tape and sealing wax workaround for it which closes a security hole here, for eg:\n\nhttps://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L7390-L7392\n\nHowever, if you change your config to rbd it will just be broken.","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"0faa1dfddfb3a3f5410d788ccbcc38a9b4ebb2ff","unresolved":false,"context_lines":[{"line_number":56,"context_line":"  security bugs. The replacement for cache() must allow the backend to"},{"line_number":57,"context_line":"  determine in advance the format and size of the disk it is importing."},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"- The Raw backend is badly named, as disks using the Raw backend may be either"},{"line_number":60,"context_line":"  raw or qcow2. The Raw backend first inspects the disk it is importing (see"},{"line_number":61,"context_line":"  problem above), then writes its format to a local file called disk.info."},{"line_number":62,"context_line":"  Storing the format of the disk means that there is no need to inspect the"},{"line_number":63,"context_line":"  disk at boot time, which prevents a severe security flaw. However, we can do"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_5bda5cea","line":60,"range":{"start_line":59,"start_character":2,"end_line":60,"end_character":15},"updated":"2016-05-04 09:48:45.000000000","message":"+1 on this. I\u0027ve always found this very confusing","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":16907,"name":"Diana Clarke","email":"diana.joan.clarke@gmail.com","username":"diana-clarke"},"change_message_id":"2116a5f9e7589e0ae4b61bb6cd6fd8ace6c91a98","unresolved":false,"context_lines":[{"line_number":73,"context_line":"canonical, persistent repository for disk metadata."},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"Developer: Enable future backend development work by removing poorly understood"},{"line_number":76,"context_line":"heurstics and tight coupling."},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"Developer: By storing disk layout per instance rather than compute node, enable"},{"line_number":79,"context_line":"the future development of features to:"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_9dcd1e89","line":76,"updated":"2016-05-03 19:19:28.000000000","message":"typo: heuristics","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f55f2083cdbf612961feb9282cc33c868603cb99","unresolved":false,"context_lines":[{"line_number":73,"context_line":"canonical, persistent repository for disk metadata."},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"Developer: Enable future backend development work by removing poorly understood"},{"line_number":76,"context_line":"heurstics and tight coupling."},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"Developer: By storing disk layout per instance rather than compute node, enable"},{"line_number":79,"context_line":"the future development of features to:"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_615f730f","line":76,"in_reply_to":"1a122d0e_9dcd1e89","updated":"2016-05-05 10:15:31.000000000","message":"Done","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ebae625fae688eadc076ca40ca7708cea974d3ac","unresolved":false,"context_lines":[{"line_number":76,"context_line":"heurstics and tight coupling."},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"Developer: By storing disk layout per instance rather than compute node, enable"},{"line_number":79,"context_line":"the future development of features to:"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"- migrate between disk layouts."},{"line_number":82,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_5ce0b6cc","line":79,"updated":"2016-05-03 14:21:59.000000000","message":"I think this work has real benefits for operators, so I\u0027d hate to see this pitched as only for the developers of Nova. At least this third one could be called an operator use-case, right?","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9bc2ed0164e03b47dcd469697ba0aaca2e57c5fa","unresolved":false,"context_lines":[{"line_number":76,"context_line":"heurstics and tight coupling."},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"Developer: By storing disk layout per instance rather than compute node, enable"},{"line_number":79,"context_line":"the future development of features to:"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"- migrate between disk layouts."},{"line_number":82,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_33e77f13","line":79,"in_reply_to":"1a122d0e_2169fbbe","updated":"2016-05-05 15:02:03.000000000","message":"I think I said this before I realized this was only the lead-in to the storage pools part, so that\u0027s fine.","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f55f2083cdbf612961feb9282cc33c868603cb99","unresolved":false,"context_lines":[{"line_number":76,"context_line":"heurstics and tight coupling."},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"Developer: By storing disk layout per instance rather than compute node, enable"},{"line_number":79,"context_line":"the future development of features to:"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"- migrate between disk layouts."},{"line_number":82,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_2169fbbe","line":79,"in_reply_to":"1a122d0e_5ce0b6cc","updated":"2016-05-05 10:15:31.000000000","message":"Well it enables the development of future features which I think operators will like. However, this change on its own doesn\u0027t do that. I don\u0027t want to over-sell it.","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ebae625fae688eadc076ca40ca7708cea974d3ac","unresolved":false,"context_lines":[{"line_number":105,"context_line":"enough context when creating a disk to know how big it is, what format it is,"},{"line_number":106,"context_line":"and where it should go. We will implement library code which persists this"},{"line_number":107,"context_line":"information for the disk, and then calls out to the relevant backend. It will"},{"line_number":108,"context_line":"be stored in a driver-specific field we will add to BlockDeviceMapping:"},{"line_number":109,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":110,"context_line":"driver will treat it as a serialised versioned object: LibbirtDiskMetadata."},{"line_number":111,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_dc84c6b2","line":108,"updated":"2016-05-03 14:21:59.000000000","message":"Just want to be clear that this is \"virt driver specific\" and not \"backend driver specific\" right?","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f55f2083cdbf612961feb9282cc33c868603cb99","unresolved":false,"context_lines":[{"line_number":105,"context_line":"enough context when creating a disk to know how big it is, what format it is,"},{"line_number":106,"context_line":"and where it should go. We will implement library code which persists this"},{"line_number":107,"context_line":"information for the disk, and then calls out to the relevant backend. It will"},{"line_number":108,"context_line":"be stored in a driver-specific field we will add to BlockDeviceMapping:"},{"line_number":109,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":110,"context_line":"driver will treat it as a serialised versioned object: LibbirtDiskMetadata."},{"line_number":111,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_e15423c7","line":108,"in_reply_to":"1a122d0e_dc84c6b2","updated":"2016-05-05 10:15:31.000000000","message":"Yep, will make that clear.","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":6608,"name":"Paul Carlton","email":"paul.carlton2@hpe.com","username":"paul-carlton2"},"change_message_id":"e126b9de22db66ed9401d42efc6187a9bae10c8e","unresolved":false,"context_lines":[{"line_number":107,"context_line":"information for the disk, and then calls out to the relevant backend. It will"},{"line_number":108,"context_line":"be stored in a driver-specific field we will add to BlockDeviceMapping:"},{"line_number":109,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":110,"context_line":"driver will treat it as a serialised versioned object: LibbirtDiskMetadata."},{"line_number":111,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."},{"line_number":112,"context_line":""},{"line_number":113,"context_line":"Alternatives"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_2232aba7","line":110,"updated":"2016-04-26 10:34:08.000000000","message":"typo: libvirtDisk...","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f55f2083cdbf612961feb9282cc33c868603cb99","unresolved":false,"context_lines":[{"line_number":107,"context_line":"information for the disk, and then calls out to the relevant backend. It will"},{"line_number":108,"context_line":"be stored in a driver-specific field we will add to BlockDeviceMapping:"},{"line_number":109,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":110,"context_line":"driver will treat it as a serialised versioned object: LibbirtDiskMetadata."},{"line_number":111,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."},{"line_number":112,"context_line":""},{"line_number":113,"context_line":"Alternatives"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_f4d85ff1","line":110,"in_reply_to":"1a122d0e_2232aba7","updated":"2016-05-05 10:15:31.000000000","message":"Done","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ebae625fae688eadc076ca40ca7708cea974d3ac","unresolved":false,"context_lines":[{"line_number":126,"context_line":""},{"line_number":127,"context_line":"The column will be populated by the driver when performing any operation on a"},{"line_number":128,"context_line":"disk and it is found to be unpopulated. It will initially take values based on"},{"line_number":129,"context_line":"the current behaviour of the driver."},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"REST API impact"},{"line_number":132,"context_line":"---------------"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_1747394c","line":129,"updated":"2016-05-03 14:21:59.000000000","message":"So this is covering the upgrades case, which is good.\n\nI assume this means that existing code that infers details will be maintained and will write the specific information to this new field to avoid having to infer things later.\n\nIn order to be able to delete the inference-based code at some point in the future, we need a way to make sure all existing instances have received updated concrete records. Have you given any thought to how we\u0027d do that?\n\nPerhaps we could have the periodic cache manager runs do one instance per host per run or something?","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f55f2083cdbf612961feb9282cc33c868603cb99","unresolved":false,"context_lines":[{"line_number":126,"context_line":""},{"line_number":127,"context_line":"The column will be populated by the driver when performing any operation on a"},{"line_number":128,"context_line":"disk and it is found to be unpopulated. It will initially take values based on"},{"line_number":129,"context_line":"the current behaviour of the driver."},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"REST API impact"},{"line_number":132,"context_line":"---------------"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_d4a723ba","line":129,"in_reply_to":"1a122d0e_1747394c","updated":"2016-05-05 10:15:31.000000000","message":"Good point, might as well get it out of the way.\n\nIt occurs to me that as this is just updating a db field with some fairly cheap metadata. We could probably do it en masse in init_host and be done with it. I doubt the overhead would be more than a few seconds even for 1000 instances.\n\nThis would mean we\u0027d want to index the field, though, because we\u0027d want to efficiently select on \u0027is not null\u0027. After the first run we\u0027d want this to return no rows extremely quickly without further overhead. The index could be removed along with the upgrade code in Ocata.\n\nWe\u0027d also need to maintain the code which handles the no data case too, though, again for a single cycle. This is because an instance could come to us via migration.\n\nSo:\n\n* We add an index to the db field\n* We bulk update in init_host for NULL fields\n* We also handle null data and write it in other code\n\nIn Ocata, we:\n* Remove the bulk update in init_host\n* Remove the index on the field\n* Make the field not nullable (*)\n* Remove code which handles null data\n\n(*) On second thoughts, can\u0027t do this because it would impact other drivers not yet using this field. Messy, but not fatal.\n\nThe advantage of this is that you can prove that as long as you have brought all compute nodes up on Newton at least once, then all instances on a compute host have been upgraded.\n\nWhat about instances with no compute host? Pretty sure they exist, but can\u0027t remember why.","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9bc2ed0164e03b47dcd469697ba0aaca2e57c5fa","unresolved":false,"context_lines":[{"line_number":126,"context_line":""},{"line_number":127,"context_line":"The column will be populated by the driver when performing any operation on a"},{"line_number":128,"context_line":"disk and it is found to be unpopulated. It will initially take values based on"},{"line_number":129,"context_line":"the current behaviour of the driver."},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"REST API impact"},{"line_number":132,"context_line":"---------------"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_d3828bfe","line":129,"in_reply_to":"1a122d0e_d4a723ba","updated":"2016-05-05 15:02:03.000000000","message":"We already pull all instances assigned to the host on compute manager startup. So yeah, I think you just insert yourself into that process and do the checking you need.","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ebae625fae688eadc076ca40ca7708cea974d3ac","unresolved":false,"context_lines":[{"line_number":138,"context_line":""},{"line_number":139,"context_line":"This change does not directly impact security, but by simplifying an area of"},{"line_number":140,"context_line":"code which has been the source of several severe security bugs it should"},{"line_number":141,"context_line":"indirectly improve security."},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"Notifications impact"},{"line_number":144,"context_line":"--------------------"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_178c597e","line":141,"updated":"2016-05-03 14:21:59.000000000","message":"You could probably make a stronger statement about being demonstrably safer by not ever inferring config from inspection of the disk, but ... under-promise and over-deliver is good too :)","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f55f2083cdbf612961feb9282cc33c868603cb99","unresolved":false,"context_lines":[{"line_number":138,"context_line":""},{"line_number":139,"context_line":"This change does not directly impact security, but by simplifying an area of"},{"line_number":140,"context_line":"code which has been the source of several severe security bugs it should"},{"line_number":141,"context_line":"indirectly improve security."},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"Notifications impact"},{"line_number":144,"context_line":"--------------------"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_b4b1c7e2","line":141,"in_reply_to":"1a122d0e_178c597e","updated":"2016-05-05 10:15:31.000000000","message":"Added another line. But yeah, promising no further security bugs seems like tempting fate :)","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ebae625fae688eadc076ca40ca7708cea974d3ac","unresolved":false,"context_lines":[{"line_number":157,"context_line":"pull back driver_info, which will add some small overhead. More significantly,"},{"line_number":158,"context_line":"with this change the driver will update the BlockDeviceMapping object for each"},{"line_number":159,"context_line":"disk during boot and similar operations, which will result in a round trip to a"},{"line_number":160,"context_line":"conductor for each disk."},{"line_number":161,"context_line":""},{"line_number":162,"context_line":"Other deployer impact"},{"line_number":163,"context_line":"---------------------"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_d7466120","line":160,"updated":"2016-05-03 14:21:59.000000000","message":"Can we batch the updates? If we have a method on BlockDeviceMappingList that takes a bunch of BDMs to save, it can do those all on the conductor side in one go.","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"f55f2083cdbf612961feb9282cc33c868603cb99","unresolved":false,"context_lines":[{"line_number":157,"context_line":"pull back driver_info, which will add some small overhead. More significantly,"},{"line_number":158,"context_line":"with this change the driver will update the BlockDeviceMapping object for each"},{"line_number":159,"context_line":"disk during boot and similar operations, which will result in a round trip to a"},{"line_number":160,"context_line":"conductor for each disk."},{"line_number":161,"context_line":""},{"line_number":162,"context_line":"Other deployer impact"},{"line_number":163,"context_line":"---------------------"}],"source_content_type":"text/x-rst","patch_set":3,"id":"1a122d0e_f43cdf30","line":160,"in_reply_to":"1a122d0e_d7466120","updated":"2016-05-05 10:15:31.000000000","message":"Good idea. I\u0027ll note that here as we don\u0027t go into that level of detail above.","commit_id":"113aed8c5575daf3c5d8af769106e49d5fc23627"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1629ffee3e7813578be4b8ec9073f31730aedc32","unresolved":false,"context_lines":[{"line_number":96,"context_line":"  create_from_image(image_id), and create_from_func(func, size)."},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"2 Create a persistent record of a disk\u0027s layout before creating the disk. This"},{"line_number":99,"context_line":"  includes at least: the backend in use, disk format, and size. This persistent"},{"line_number":100,"context_line":"  record must be extensible so that, for example, in the future we can specify"},{"line_number":101,"context_line":"  multiple local storage locations for qcow2 disks, and choose between them."},{"line_number":102,"context_line":""}],"source_content_type":"text/x-rst","patch_set":4,"id":"1a122d0e_bc6fd75e","line":99,"range":{"start_line":99,"start_character":58,"end_line":99,"end_character":62},"updated":"2016-05-05 17:40:10.000000000","message":"This will probably get confused with BDM.volume_size, but that\u0027s just because we munge volumes and disk and everything storage within the BDM object, but a lot of it is optional depending on how it\u0027s used and what\u0027s backing it up.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"07732ad1dd6eb6a355c6ac3b690510b0d55a8203","unresolved":false,"context_lines":[{"line_number":98,"context_line":"2 Create a persistent record of a disk\u0027s layout before creating the disk. This"},{"line_number":99,"context_line":"  includes at least: the backend in use, disk format, and size. This persistent"},{"line_number":100,"context_line":"  record must be extensible so that, for example, in the future we can specify"},{"line_number":101,"context_line":"  multiple local storage locations for qcow2 disks, and choose between them."},{"line_number":102,"context_line":""},{"line_number":103,"context_line":"The first change involves a substantial refactoring of libvirt\u0027s imagebackend"},{"line_number":104,"context_line":"module. This should be achieved with a minimum of functional change."}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_000f30e2","line":101,"updated":"2016-05-05 23:00:06.000000000","message":"What mriedem says, but I\u0027ll also add that the above description of an \"extensible\" record of a disk layout looks nearly identical to the Image API\u0027s definition of an image, including format, size, and a set of locations where the image can be found. The fact that this looks so similar to the Glance definition of an image makes me even more wary that we are duplicating a bunch of information already storable in Glance...","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"9da4f56352ab324d2a1fc1fa7e4d9b16099e8aa4","unresolved":false,"context_lines":[{"line_number":98,"context_line":"2 Create a persistent record of a disk\u0027s layout before creating the disk. This"},{"line_number":99,"context_line":"  includes at least: the backend in use, disk format, and size. This persistent"},{"line_number":100,"context_line":"  record must be extensible so that, for example, in the future we can specify"},{"line_number":101,"context_line":"  multiple local storage locations for qcow2 disks, and choose between them."},{"line_number":102,"context_line":""},{"line_number":103,"context_line":"The first change involves a substantial refactoring of libvirt\u0027s imagebackend"},{"line_number":104,"context_line":"module. This should be achieved with a minimum of functional change."}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_52ca97a2","line":101,"in_reply_to":"dab17558_000f30e2","updated":"2016-05-06 13:00:07.000000000","message":"The problem with relying on glance is that glance metadata can be arbitrarily changed for existing images. So the data associated image that Nova has cached locally on the node may not match what glance currently reports. IOW we can\u0027t trust glance, except at the very moment we initially download the image. Thereafter we must rely on our own record and ignore what glance says","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4eff4cb8756a6410ecf4faa01cffd67aff0ccfe1","unresolved":false,"context_lines":[{"line_number":98,"context_line":"2 Create a persistent record of a disk\u0027s layout before creating the disk. This"},{"line_number":99,"context_line":"  includes at least: the backend in use, disk format, and size. This persistent"},{"line_number":100,"context_line":"  record must be extensible so that, for example, in the future we can specify"},{"line_number":101,"context_line":"  multiple local storage locations for qcow2 disks, and choose between them."},{"line_number":102,"context_line":""},{"line_number":103,"context_line":"The first change involves a substantial refactoring of libvirt\u0027s imagebackend"},{"line_number":104,"context_line":"module. This should be achieved with a minimum of functional change."}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_8d269e14","line":101,"in_reply_to":"dab17558_000f30e2","updated":"2016-05-06 13:46:51.000000000","message":"There are some superficial parallels with glance in this data, but it\u0027s really quite different. It is entirely driver specific, for example, and refers to ephemeral storage which is not accessible except by the compute host. There is also exactly one location, not a set, although it might change throughout the lifecycle of an instance. The data may also evolve in future versions, for example to include libvirt storage pool name or any data relevant specifically to how the libvirt driver accesses any storage backend we use for ephemeral storage.\n\nThe other critical difference is that the owner of the data is the nova compute host, meaning that we trust it absolutely. It is not possible for a user to specify or even view it.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"2655d9cc2efb827807f9cc53070c329b9f4bb64a","unresolved":false,"context_lines":[{"line_number":98,"context_line":"2 Create a persistent record of a disk\u0027s layout before creating the disk. This"},{"line_number":99,"context_line":"  includes at least: the backend in use, disk format, and size. This persistent"},{"line_number":100,"context_line":"  record must be extensible so that, for example, in the future we can specify"},{"line_number":101,"context_line":"  multiple local storage locations for qcow2 disks, and choose between them."},{"line_number":102,"context_line":""},{"line_number":103,"context_line":"The first change involves a substantial refactoring of libvirt\u0027s imagebackend"},{"line_number":104,"context_line":"module. This should be achieved with a minimum of functional change."}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_10e646b4","line":101,"in_reply_to":"dab17558_52ca97a2","updated":"2016-05-06 14:15:23.000000000","message":"@danpb: Got it. Makes sense.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d60156299d336915c0e2d31cdab2c5ef76f7453c","unresolved":false,"context_lines":[{"line_number":98,"context_line":"2 Create a persistent record of a disk\u0027s layout before creating the disk. This"},{"line_number":99,"context_line":"  includes at least: the backend in use, disk format, and size. This persistent"},{"line_number":100,"context_line":"  record must be extensible so that, for example, in the future we can specify"},{"line_number":101,"context_line":"  multiple local storage locations for qcow2 disks, and choose between them."},{"line_number":102,"context_line":""},{"line_number":103,"context_line":"The first change involves a substantial refactoring of libvirt\u0027s imagebackend"},{"line_number":104,"context_line":"module. This should be achieved with a minimum of functional change."}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_8d585dfc","line":101,"in_reply_to":"dab17558_52ca97a2","updated":"2016-05-06 13:43:56.000000000","message":"Yeah.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1629ffee3e7813578be4b8ec9073f31730aedc32","unresolved":false,"context_lines":[{"line_number":105,"context_line":""},{"line_number":106,"context_line":"The second change depends on the first. With the first change in place we have"},{"line_number":107,"context_line":"enough context when creating a disk to know how big it is, what format it is,"},{"line_number":108,"context_line":"and where it should go. We will implement library code which persists this"},{"line_number":109,"context_line":"information for the disk, and then calls out to the relevant backend. It will"},{"line_number":110,"context_line":"be stored in a virt driver specific field we will add to BlockDeviceMapping:"},{"line_number":111,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"}],"source_content_type":"text/x-rst","patch_set":4,"id":"1a122d0e_3cf467ef","line":108,"range":{"start_line":108,"start_character":42,"end_line":108,"end_character":49},"updated":"2016-05-05 17:40:10.000000000","message":"Are you writing a separate library for this? Or just, new goodness within Nova that handles this.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4eff4cb8756a6410ecf4faa01cffd67aff0ccfe1","unresolved":false,"context_lines":[{"line_number":105,"context_line":""},{"line_number":106,"context_line":"The second change depends on the first. With the first change in place we have"},{"line_number":107,"context_line":"enough context when creating a disk to know how big it is, what format it is,"},{"line_number":108,"context_line":"and where it should go. We will implement library code which persists this"},{"line_number":109,"context_line":"information for the disk, and then calls out to the relevant backend. It will"},{"line_number":110,"context_line":"be stored in a virt driver specific field we will add to BlockDeviceMapping:"},{"line_number":111,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_0dee2e8b","line":108,"range":{"start_line":108,"start_character":42,"end_line":108,"end_character":49},"in_reply_to":"1a122d0e_3cf467ef","updated":"2016-05-06 13:46:51.000000000","message":"\u0027library code\u0027 here is just in the general sense. It\u0027ll probably just be a class in imagebackend. Nothing fancy, just common code amongst all the backends. Haven\u0027t given detailed thought to exactly where I\u0027ll put this, yet. Doesn\u0027t really matter as long as it goes somewhere sensible.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9cc09fa192171fe92f843cdaf42018f08e9c56be","unresolved":false,"context_lines":[{"line_number":109,"context_line":"information for the disk, and then calls out to the relevant backend. It will"},{"line_number":110,"context_line":"be stored in a virt driver specific field we will add to BlockDeviceMapping:"},{"line_number":111,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":112,"context_line":"driver will treat it as a serialised versioned object: LibVirtDiskMetadata."},{"line_number":113,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"Alternatives"}],"source_content_type":"text/x-rst","patch_set":4,"id":"1a122d0e_33549f52","line":112,"range":{"start_line":112,"start_character":58,"end_line":112,"end_character":59},"updated":"2016-05-05 15:04:34.000000000","message":"Everywhere else we don\u0027t capitalize the V. Assuming you won\u0027t do that in the code, I\u0027ll let this slide :)","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4eff4cb8756a6410ecf4faa01cffd67aff0ccfe1","unresolved":false,"context_lines":[{"line_number":109,"context_line":"information for the disk, and then calls out to the relevant backend. It will"},{"line_number":110,"context_line":"be stored in a virt driver specific field we will add to BlockDeviceMapping:"},{"line_number":111,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":112,"context_line":"driver will treat it as a serialised versioned object: LibVirtDiskMetadata."},{"line_number":113,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"Alternatives"}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_2dbb3274","line":112,"range":{"start_line":112,"start_character":58,"end_line":112,"end_character":59},"in_reply_to":"1a122d0e_33549f52","updated":"2016-05-06 13:46:51.000000000","message":"Looks like I\u0027m likely to update anyway, so I\u0027ll fix it on the next push.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1629ffee3e7813578be4b8ec9073f31730aedc32","unresolved":false,"context_lines":[{"line_number":110,"context_line":"be stored in a virt driver specific field we will add to BlockDeviceMapping:"},{"line_number":111,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":112,"context_line":"driver will treat it as a serialised versioned object: LibVirtDiskMetadata."},{"line_number":113,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"Alternatives"},{"line_number":116,"context_line":"------------"}],"source_content_type":"text/x-rst","patch_set":4,"id":"1a122d0e_1c2c0b49","line":113,"range":{"start_line":113,"start_character":0,"end_line":113,"end_character":19},"updated":"2016-05-05 17:40:10.000000000","message":"So is this going to look like LiveMigrateData where we have a base class that\u0027s not a registered object, and then subclasses with the virt-driver specific driver info? Because just throwing a libvirt-specific blob into the BDM object sounds bad to me, but if it\u0027s an abstraction with virt-driver specific details, then I\u0027m more on board with it.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8534a6810fecbdf58e22b9232531e059288b1e6c","unresolved":false,"context_lines":[{"line_number":110,"context_line":"be stored in a virt driver specific field we will add to BlockDeviceMapping:"},{"line_number":111,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":112,"context_line":"driver will treat it as a serialised versioned object: LibVirtDiskMetadata."},{"line_number":113,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"Alternatives"},{"line_number":116,"context_line":"------------"}],"source_content_type":"text/x-rst","patch_set":4,"id":"1a122d0e_3fa6190f","line":113,"range":{"start_line":113,"start_character":0,"end_line":113,"end_character":19},"in_reply_to":"1a122d0e_1c2c0b49","updated":"2016-05-05 17:43:37.000000000","message":"Why does it sound bad to you? We already make the BDMs virt-specific when we choose a virt-specific device name and store connection information, etc al, right?\n\nWe should definitely have an unregistered base class for inheritance checking at least, but I\u0027m not sure what common bits there will be.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4ea8635936cc7b30adacaa0019e5f6bc85aae4aa","unresolved":false,"context_lines":[{"line_number":110,"context_line":"be stored in a virt driver specific field we will add to BlockDeviceMapping:"},{"line_number":111,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":112,"context_line":"driver will treat it as a serialised versioned object: LibVirtDiskMetadata."},{"line_number":113,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"Alternatives"},{"line_number":116,"context_line":"------------"}],"source_content_type":"text/x-rst","patch_set":4,"id":"1a122d0e_6dc74b45","line":113,"range":{"start_line":113,"start_character":0,"end_line":113,"end_character":19},"in_reply_to":"1a122d0e_223722b1","updated":"2016-05-05 18:45:18.000000000","message":"Yeah, FWIW, I\u0027m totally fine with doing this like the LiveMigrateData object structure we have for live migration, with the virt-specific sub-objects/types.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1621845a66edd33fd9768980561e469fbc910c0c","unresolved":false,"context_lines":[{"line_number":110,"context_line":"be stored in a virt driver specific field we will add to BlockDeviceMapping:"},{"line_number":111,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":112,"context_line":"driver will treat it as a serialised versioned object: LibVirtDiskMetadata."},{"line_number":113,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"Alternatives"},{"line_number":116,"context_line":"------------"}],"source_content_type":"text/x-rst","patch_set":4,"id":"1a122d0e_df1c45c4","line":113,"range":{"start_line":113,"start_character":0,"end_line":113,"end_character":19},"in_reply_to":"1a122d0e_3fa6190f","updated":"2016-05-05 17:58:07.000000000","message":"Sure, that doesn\u0027t mean it\u0027s awesome, connection_info is a nasty mess we pass around with Cinder. At least this new thing would be versioned from the start, which is good.\n\nMaybe rather than have an ABC object and virt-driver specific sub-classes, we should just have a versioned object that handles this for all of the virt drivers, i.e. BDMDriverInfo or DriverInfoMetadata or something. If another virt driver wants to add something to it, they do that, but it might not be used by all virt drivers. Over time that could get ugly also because it becomes like the BDM object itself, where it\u0027s a jack of all trades.\n\nAnyway, I\u0027m not -1 on this part, just raising my eww flag.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4eff4cb8756a6410ecf4faa01cffd67aff0ccfe1","unresolved":false,"context_lines":[{"line_number":110,"context_line":"be stored in a virt driver specific field we will add to BlockDeviceMapping:"},{"line_number":111,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":112,"context_line":"driver will treat it as a serialised versioned object: LibVirtDiskMetadata."},{"line_number":113,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"Alternatives"},{"line_number":116,"context_line":"------------"}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_6d81b94a","line":113,"range":{"start_line":113,"start_character":0,"end_line":113,"end_character":19},"in_reply_to":"1a122d0e_6dc74b45","updated":"2016-05-06 13:46:51.000000000","message":"I had in mind that this would be versioned, but driver-specific. That way we don\u0027t end up with weird cross-use of only vaguely similar fields in wildly different contexts, and a proliferation of fields which aren\u0027t relevant in most places.\n\nI intend to follow the LiveMigrateData model.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"9da4f56352ab324d2a1fc1fa7e4d9b16099e8aa4","unresolved":false,"context_lines":[{"line_number":110,"context_line":"be stored in a virt driver specific field we will add to BlockDeviceMapping:"},{"line_number":111,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":112,"context_line":"driver will treat it as a serialised versioned object: LibVirtDiskMetadata."},{"line_number":113,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"Alternatives"},{"line_number":116,"context_line":"------------"}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_b21783ec","line":113,"range":{"start_line":113,"start_character":0,"end_line":113,"end_character":19},"in_reply_to":"1a122d0e_6dc74b45","updated":"2016-05-06 13:00:07.000000000","message":"We already have a nova/virt/image/model.py file defining a pure python Image object. This encodes information about the format, and how to access the image (local file vs block dev vs rbd). This object is used by the various methods in nova/virt/disk/*.py \n\nRather than creating yet another alternative way to describe an image, I think it\u0027d be highly desirable to have the same model used everywhere.\n\nI understand the current class probably doesn\u0027t have sufficient information associated with it.  So either we should extend this existing Image class, or we should define a new set of Nova objects and use them to replace existing usage of this Image class.  We really don\u0027t want to end up having 2 different ways to describe Images used by different bits of the code.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bbce88028a3aebc265a500eea56f5a805af018a7","unresolved":false,"context_lines":[{"line_number":110,"context_line":"be stored in a virt driver specific field we will add to BlockDeviceMapping:"},{"line_number":111,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":112,"context_line":"driver will treat it as a serialised versioned object: LibVirtDiskMetadata."},{"line_number":113,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"Alternatives"},{"line_number":116,"context_line":"------------"}],"source_content_type":"text/x-rst","patch_set":4,"id":"1a122d0e_223722b1","line":113,"range":{"start_line":113,"start_character":0,"end_line":113,"end_character":19},"in_reply_to":"1a122d0e_df1c45c4","updated":"2016-05-05 18:12:40.000000000","message":"Yeah, the reason it\u0027s nasty is because we don\u0027t know what is in it. Well, one of the reasons anyway.\n\nI guess I see a completely generic blob becoming more like ImageMeta, which is a GIANT thing that has replaced a dict of madness with known keys and valid values, but has lost all sense of the contextual relationship between those values. If we do a common object for this, then someone going to work on xen code will have to examine the driver itself to figure out which fields are likely to be set in the xen case vs. the libvirt one.\n\nI don\u0027t think we can support cold migration between hypervisor types today (evidence: the libvirt driver scp\u0027s data around, for one example). Thus, I don\u0027t think this is really any different than live migration having a virt-specific blob. In the future, translating from one to the other will be a lot easier if it\u0027s a known/versioned blob, should that requirement ever be raised.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"5d1c2445a690d18e5505104ef36c88a27f2a7abe","unresolved":false,"context_lines":[{"line_number":110,"context_line":"be stored in a virt driver specific field we will add to BlockDeviceMapping:"},{"line_number":111,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":112,"context_line":"driver will treat it as a serialised versioned object: LibVirtDiskMetadata."},{"line_number":113,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"Alternatives"},{"line_number":116,"context_line":"------------"}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_8cf5f684","line":113,"range":{"start_line":113,"start_character":0,"end_line":113,"end_character":19},"in_reply_to":"dab17558_6d81b94a","updated":"2016-05-06 17:13:33.000000000","message":"@danpb I think the data I want to store is different to the data stored in the image hierarchy, although there is significant overlap there. IIUC the image model was originally created to be a common location representation so that multiple hypervisors can use libguestfs. This is great, but the storage backend is more than just a location: it\u0027s also semantics.\n\nFor example, an image with a backend of \u0027Qcow2\u0027 isn\u0027t just a path and a file format. It also caches images in the global image cache, it always has a backing file, and that backing file is always in the global image cache. The \u0027LibvirtQcow2\u0027 backend, which uses a libvirt storage pool, may also have a path and an image format. However, it\u0027s guaranteed to be in a storage pool, and its path is determined by that storage pool. It may also be able to use a different image cache location. This contextual information is irrelevant to libguestfs, but we need it in the driver.\n\nI think if we tried to shoehorn everything we needed into the Image model hierarchy it would get unnecessarily messy very quickly. I still think the best way forward is separate, driver-specific data.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d60156299d336915c0e2d31cdab2c5ef76f7453c","unresolved":false,"context_lines":[{"line_number":110,"context_line":"be stored in a virt driver specific field we will add to BlockDeviceMapping:"},{"line_number":111,"context_line":"driver_info. Higher level code will treat this as an opaque blob. The libvirt"},{"line_number":112,"context_line":"driver will treat it as a serialised versioned object: LibVirtDiskMetadata."},{"line_number":113,"context_line":"LibvirtDiskMetadata will initially contain the metadata mentioned above."},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"Alternatives"},{"line_number":116,"context_line":"------------"}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_0d562dd2","line":113,"range":{"start_line":113,"start_character":0,"end_line":113,"end_character":19},"in_reply_to":"dab17558_b21783ec","updated":"2016-05-06 13:43:56.000000000","message":"By some quick grepping, that model doesn\u0027t seem to be used many places outside of libvirt. Even still, it has almost no meat to it at all, to the point that I\u0027m not even sure what the point of it is. It looks like it\u0027s a class hierarchy that serves as an enum, with a format field (aside from the rbd case which has more detail). I\u0027m not sure that I\u0027d want to use that model for this case, where we\u0027d end up serializing one of many potential classes into the DB.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"07732ad1dd6eb6a355c6ac3b690510b0d55a8203","unresolved":false,"context_lines":[{"line_number":124,"context_line":""},{"line_number":125,"context_line":"A driver_info column will be added to the block_device_mapping table with type"},{"line_number":126,"context_line":"MediumText. The column will be nullable, and will initially be unpopulated. It"},{"line_number":127,"context_line":"will not be indexed, or have any associated constraints."},{"line_number":128,"context_line":""},{"line_number":129,"context_line":"The column will be populated by the driver when performing any operation on a"},{"line_number":130,"context_line":"disk and it is found to be unpopulated. It will initially take values based on"}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_e0130c0c","line":127,"updated":"2016-05-05 23:00:06.000000000","message":"I\u0027m fine with this. Honestly, you can just use TEXT though. MEDIUMTEXT really doesn\u0027t have much value here, and it\u0027s just a random MySQLism (that should DIAF actually).","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"2655d9cc2efb827807f9cc53070c329b9f4bb64a","unresolved":false,"context_lines":[{"line_number":124,"context_line":""},{"line_number":125,"context_line":"A driver_info column will be added to the block_device_mapping table with type"},{"line_number":126,"context_line":"MediumText. The column will be nullable, and will initially be unpopulated. It"},{"line_number":127,"context_line":"will not be indexed, or have any associated constraints."},{"line_number":128,"context_line":""},{"line_number":129,"context_line":"The column will be populated by the driver when performing any operation on a"},{"line_number":130,"context_line":"disk and it is found to be unpopulated. It will initially take values based on"}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_b0ff7a33","line":127,"in_reply_to":"dab17558_cda3859d","updated":"2016-05-06 14:15:23.000000000","message":"Nah, the storage size for both TEXT and MEDIUMTEXT would be fine for the BDM.connection. I recommend not using MEDIUMTEXT because it\u0027s a MySQL-specific data type and offers no value whatsoever in terms of performance or storage requirements on modern hardware...","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4eff4cb8756a6410ecf4faa01cffd67aff0ccfe1","unresolved":false,"context_lines":[{"line_number":124,"context_line":""},{"line_number":125,"context_line":"A driver_info column will be added to the block_device_mapping table with type"},{"line_number":126,"context_line":"MediumText. The column will be nullable, and will initially be unpopulated. It"},{"line_number":127,"context_line":"will not be indexed, or have any associated constraints."},{"line_number":128,"context_line":""},{"line_number":129,"context_line":"The column will be populated by the driver when performing any operation on a"},{"line_number":130,"context_line":"disk and it is found to be unpopulated. It will initially take values based on"}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_ede4893c","line":127,"in_reply_to":"dab17558_e0130c0c","updated":"2016-05-06 13:46:51.000000000","message":"I think I just copied MEDIUMTEXT from somewhere similar :) Will update.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"19bedda5379394f86b2c56d1311aae7c7e56e94d","unresolved":false,"context_lines":[{"line_number":124,"context_line":""},{"line_number":125,"context_line":"A driver_info column will be added to the block_device_mapping table with type"},{"line_number":126,"context_line":"MediumText. The column will be nullable, and will initially be unpopulated. It"},{"line_number":127,"context_line":"will not be indexed, or have any associated constraints."},{"line_number":128,"context_line":""},{"line_number":129,"context_line":"The column will be populated by the driver when performing any operation on a"},{"line_number":130,"context_line":"disk and it is found to be unpopulated. It will initially take values based on"}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_cda3859d","line":127,"in_reply_to":"dab17558_e0130c0c","updated":"2016-05-06 13:49:35.000000000","message":"I was looking at this yesterday too and we aren\u0027t very consistent with TEXT vs MEDIUMTEXT in our DB model. I assume MEDIUMTEXT was just picked because that\u0027s what BDM.connection_info uses, and we want it bigger because it\u0027s such a random blob of gorp that we get from Cinder and can change at any time by their drivers.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1629ffee3e7813578be4b8ec9073f31730aedc32","unresolved":false,"context_lines":[{"line_number":141,"context_line":"This change does not directly impact security, but by simplifying an area of"},{"line_number":142,"context_line":"code which has been the source of several severe security bugs it should"},{"line_number":143,"context_line":"indirectly improve security. Specifically, by ensuring we always know the"},{"line_number":144,"context_line":"format of a virtual disk we should never have to insecure format inspection."},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"Notifications impact"},{"line_number":147,"context_line":"--------------------"}],"source_content_type":"text/x-rst","patch_set":4,"id":"1a122d0e_bcf51752","line":144,"range":{"start_line":144,"start_character":46,"end_line":144,"end_character":57},"updated":"2016-05-05 17:40:10.000000000","message":"nit: to perform insecure?","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4eff4cb8756a6410ecf4faa01cffd67aff0ccfe1","unresolved":false,"context_lines":[{"line_number":141,"context_line":"This change does not directly impact security, but by simplifying an area of"},{"line_number":142,"context_line":"code which has been the source of several severe security bugs it should"},{"line_number":143,"context_line":"indirectly improve security. Specifically, by ensuring we always know the"},{"line_number":144,"context_line":"format of a virtual disk we should never have to insecure format inspection."},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"Notifications impact"},{"line_number":147,"context_line":"--------------------"}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_edbbe90c","line":144,"range":{"start_line":144,"start_character":46,"end_line":144,"end_character":57},"in_reply_to":"1a122d0e_bcf51752","updated":"2016-05-06 13:46:51.000000000","message":"Done","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"07732ad1dd6eb6a355c6ac3b690510b0d55a8203","unresolved":false,"context_lines":[{"line_number":162,"context_line":"disk during boot and similar operations. We should be able to reduce the impact"},{"line_number":163,"context_line":"of this update for instances with multiple disks by batching them in a single"},{"line_number":164,"context_line":"update to a BlockDeviceMappingList object. This would require only one round"},{"line_number":165,"context_line":"trip to conductor."},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"Other deployer impact"},{"line_number":168,"context_line":"---------------------"}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_40f938c8","line":165,"updated":"2016-05-05 23:00:06.000000000","message":"++ for the batched update approach.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1629ffee3e7813578be4b8ec9073f31730aedc32","unresolved":false,"context_lines":[{"line_number":215,"context_line":"Unit tests will likely have significant churn due to changing internal"},{"line_number":216,"context_line":"interfaces, however the scenarios covered should be at least the same as"},{"line_number":217,"context_line":"previously."},{"line_number":218,"context_line":""},{"line_number":219,"context_line":""},{"line_number":220,"context_line":"Documentation Impact"},{"line_number":221,"context_line":"\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d"}],"source_content_type":"text/x-rst","patch_set":4,"id":"1a122d0e_9f13ed5b","line":218,"updated":"2016-05-05 17:40:10.000000000","message":"ploop compute/storage third party CI should be testing this too hopefully, and we have a ceph job. As noted before when discussing this, we don\u0027t have an LVM job in the gate, and we\u0027re not sure if we\u0027re testing both qcow2 and raw in the gate. I think we can do the LVM job pretty easily. There is an experimental job for LXC + LVM but it\u0027s super unreliable so I wouldn\u0027t want to use that for LVM regression testing here. And I think we can just have a TODO to check on the qcow2/raw question.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"421893a7905ee72f78764fcab31643128a35100c","unresolved":false,"context_lines":[{"line_number":215,"context_line":"Unit tests will likely have significant churn due to changing internal"},{"line_number":216,"context_line":"interfaces, however the scenarios covered should be at least the same as"},{"line_number":217,"context_line":"previously."},{"line_number":218,"context_line":""},{"line_number":219,"context_line":""},{"line_number":220,"context_line":"Documentation Impact"},{"line_number":221,"context_line":"\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d\u003d"}],"source_content_type":"text/x-rst","patch_set":4,"id":"dab17558_22e85810","line":218,"in_reply_to":"1a122d0e_9f13ed5b","updated":"2016-05-06 15:58:45.000000000","message":"I\u0027ll add a note here about current CI coverage.","commit_id":"61e7c461439bbb3fd5693914d393763cf3837270"},{"author":{"_account_id":18602,"name":"Gábor Antal","email":"antal@inf.u-szeged.hu","username":"antal"},"change_message_id":"1f38f5635f3f350cc136eee1d9b8d7c735a726e3","unresolved":false,"context_lines":[{"line_number":16,"context_line":"vulnerabilities. It also makes storage configuration hard or impossible to vary"},{"line_number":17,"context_line":"between compute nodes in the same installation, or over time after"},{"line_number":18,"context_line":"installation. By persisting storage metadata of a particular instance"},{"line_number":19,"context_line":"explicitly we make its configuration unambiguous and simple to understand, and"},{"line_number":20,"context_line":"therefore less vulnerable to security issues. We also make it possible to"},{"line_number":21,"context_line":"unambiguously represent multiple storage configurations within a single"},{"line_number":22,"context_line":"installation. While we don\u0027t make the necessary changes to correctly handle"}],"source_content_type":"text/x-rst","patch_set":5,"id":"dab17558_6f440474","line":19,"range":{"start_line":19,"start_character":53,"end_line":19,"end_character":59},"updated":"2016-05-06 17:59:07.000000000","message":"\"easy\" maybe would a better word for this?","commit_id":"bee69067903761f2adf8e69e999c9baef5d9b6a9"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"c9a40f634f44343e3247fe047d1bdf03b1f9700a","unresolved":false,"context_lines":[{"line_number":16,"context_line":"vulnerabilities. It also makes storage configuration hard or impossible to vary"},{"line_number":17,"context_line":"between compute nodes in the same installation, or over time after"},{"line_number":18,"context_line":"installation. By persisting storage metadata of a particular instance"},{"line_number":19,"context_line":"explicitly we make its configuration unambiguous and simple to understand, and"},{"line_number":20,"context_line":"therefore less vulnerable to security issues. We also make it possible to"},{"line_number":21,"context_line":"unambiguously represent multiple storage configurations within a single"},{"line_number":22,"context_line":"installation. While we don\u0027t make the necessary changes to correctly handle"}],"source_content_type":"text/x-rst","patch_set":5,"id":"dab17558_26dcbee3","line":19,"range":{"start_line":19,"start_character":53,"end_line":19,"end_character":59},"in_reply_to":"dab17558_6f440474","updated":"2016-05-09 08:50:23.000000000","message":"You actually made me think about this. I went to look up what it would be in Simplified English, but before I found the manual it occurred to me that it\u0027s probably ok if it\u0027s good enough for the title ;)","commit_id":"bee69067903761f2adf8e69e999c9baef5d9b6a9"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1dab70d76cf01f50a85d23770b68c8e08f7c956c","unresolved":false,"context_lines":[{"line_number":221,"context_line":"in the gate. All current libvirt tempest jobs run by Jenkins use the"},{"line_number":222,"context_line":"default Qcow2 backend except gate-tempest-dsvm-full-devstack-plugin-ceph, which"},{"line_number":223,"context_line":"uses Rbd. We additionally coverage of the ploop backend in"},{"line_number":224,"context_line":"check-dsvm-tempest-vz7-exe-minimal run by Virtuozzo CI. This means that we"},{"line_number":225,"context_line":"currently have no gate coverage of the Raw and Lvm backends."},{"line_number":226,"context_line":""},{"line_number":227,"context_line":""},{"line_number":228,"context_line":"Documentation Impact"}],"source_content_type":"text/x-rst","patch_set":5,"id":"dab17558_e9d354d3","line":225,"range":{"start_line":224,"start_character":56,"end_line":225,"end_character":60},"updated":"2016-05-10 16:49:50.000000000","message":"But per the live migration meeting this morning, we\u0027re working on that, so yay!","commit_id":"bee69067903761f2adf8e69e999c9baef5d9b6a9"}]}