openstack
/
nova-specs
Code Issues Proposed changes
nova-specs/befeb2b945cd58663433afb9b0c...

410 lines
17 KiB
Plaintext
Raw Blame History

{
"comments": [
{
"unresolved": false,
"key": {
"uuid": "88a1e2a1_14fc3ff3",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 12
},
"lineNbr": 0,
"author": {
"id": 4393
},
"writtenOn": "2023-05-24T16:41:01Z",
"side": 1,
"message": "I think we need to be pretty specific about ho detailed we\u0027re going to be here, in light of CVE-2023-2088.",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "cd40ddfa_bcd22ec9",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 11,
"author": {
"id": 7166
},
"writtenOn": "2023-05-22T15:09:01Z",
"side": 1,
"message": "nit: change the link please",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "503cf8b0_553d04ec",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 27,
"author": {
"id": 7166
},
"writtenOn": "2023-05-22T15:09:01Z",
"side": 1,
"message": "in general, you don\u0027t need to provide the reproduction steps but here I\u0027m OK",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "a796f9dd_53e89823",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 35,
"author": {
"id": 34860
},
"writtenOn": "2023-05-26T06:21:29Z",
"side": 1,
"message": "this is fixed on cider side, with patch https://review.opendev.org/c/openstack/cinder/+/882835\nwill remove these steps.",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "495f4f21_7484c1fd",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 54,
"author": {
"id": 4393
},
"writtenOn": "2023-05-24T16:41:01Z",
"side": 1,
"message": "I think we might want to be more specific here and say that we need to specifically correlate our attachments (by id) to those in cinder and remove any that are no longer valid. We also want to be specific that creating an attachment in cinder should not be adopted or integrated into nova, effectively allowing someone to bypass nova\u0027s attachment api by jamming the attachment into cinder and then rebooting the instance.\n\nSo this should be based on attachment_id, as stored in our connector, and we should purge any that don\u0027t exist on the cinder side *and* not import any that we find when listing attachments on the volume.",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "8e29b51b_ead2aa2f",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 54,
"author": {
"id": 4393
},
"writtenOn": "2023-05-24T18:25:17Z",
"side": 1,
"message": "Let\u0027s also add that we should delete cinder attachments that we don\u0027t know anything about that are claiming to be for this instance. Meaning, we should list attachments for the volume and then (in pseudocode):\n```\nfor attachment in volume[\u0027attachments\u0027]:\n if attachment.instance_uuid \u003d\u003d instance.uuid and\n attachment.id not in bdms:\n delete_attachment(attachment)\nfor bdm in bdms:\n if bdm.attachment_id not in volume[\u0027attachments\u0027]:\n delete_bdm(bdm)\n```\n\nWe very specifically need to be clear that you can\u0027t attach or detach volumes directly with cinder without going through nova\u0027s API.",
"parentUuid": "495f4f21_7484c1fd",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "4b6adb3a_07c7d568",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 54,
"author": {
"id": 11655
},
"writtenOn": "2023-05-25T17:22:54Z",
"side": 1,
"message": "So deleting any attachments Nova doesn\u0027t explicitly know about will break Ironic\u0027s Boot from Volume support, at least as it stands today and has been used by operators over the past 7 years.\n\nSo, you could just ignore it for nova.virt.ironic users, or we could move record synchronization to be Nova\u0027s responsibility under this proposed model of interaction which is rather similar to what Ironic does already for it\u0027s own data copies of attachment data. We would likely need to have an additional flag or indicator to shift responsibility, and tell Ironic to only go hunt down and ensure all attachments are removed upon the tear down of a physical machine (the instance) in order to prevent another CVE or end-user data loss.\n\nI think the preference on the path forward is a question for the nova contributors on how they want to proceed in this specific case. While the bulk of Bare Metal as a Service users utilize local storage, a portion of BMaaS operators do use Boot from Volume, particularly in larger scale shared resource environments.\n\nRegardless, the overall behavior will need to figured out in terms of upgrade so existing users are not horribly broken upon upgrade if they haven\u0027t had a chance to perform whatever the necessary determined steps are.\n\nTo be clear, I\u0027m not saying \"no\" to the idea. I actually kind of like the idea, but I\u0027m saying \"there be dragons here, lets navigate them!\"",
"parentUuid": "8e29b51b_ead2aa2f",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "bf03f91b_ccbd2272",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 83,
"author": {
"id": 7166
},
"writtenOn": "2023-05-22T15:09:01Z",
"side": 1,
"message": "the two above paragraphs are not the proposed change, just explaining why it won\u0027t work.",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "64fdb0a2_77fb418d",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 85,
"author": {
"id": 7166
},
"writtenOn": "2023-05-22T15:09:01Z",
"side": 1,
"message": "only when restarting the instances, or should we also verify that when restarting the nova-compute ?",
"range": {
"startLine": 85,
"startChar": 0,
"endLine": 85,
"endChar": 30
},
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "31a0cfef_02182f76",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 92,
"author": {
"id": 7166
},
"writtenOn": "2023-05-22T15:09:01Z",
"side": 1,
"message": "what if the Cinder API isn\u0027t running? Could you explain that then we would continue to support the BDM but providing a log.",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "e83c6011_a49eed7f",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 107,
"author": {
"id": 11655
},
"writtenOn": "2023-05-25T17:22:54Z",
"side": 1,
"message": "From an Ironic POV, the BDM data doesn\u0027t really need to be \"updated\". We\u0027ll update our representation of the data upon the next power cycle directly from cinder. Basically Ironic already does this, itself, actually. :) It is just not an XML file on our side, but cross syncing volume target and volume connector information (connectors being information about initiators, and targets being BDM records).\n\nTo make it work, the Ironic bare metal node will need to be powered down for the block device mapping (volume target). We might need to add some sort of indicator to say \"Hey, this is the authority for syncing\" indicator, but that is much more an implementation detail.",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "3f4e20d7_8be5fafe",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 131,
"author": {
"id": 7166
},
"writtenOn": "2023-05-22T15:09:01Z",
"side": 1,
"message": "not sure you really need to explain your code changes, but OK.",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "e805f7aa_c2b537ee",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 131,
"author": {
"id": 34860
},
"writtenOn": "2023-05-26T06:21:29Z",
"side": 1,
"message": "this is not valid anymore, will remove exception traceback.",
"parentUuid": "3f4e20d7_8be5fafe",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "ff344a9d_7560317c",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 174,
"author": {
"id": 7166
},
"writtenOn": "2023-05-22T15:09:01Z",
"side": 1,
"message": "really ? \n\nI think that we would have some impact given we need to call Cinder everytime we restart an instance, even when restarting nova-compute if by default we restart the instances.\n\nThat\u0027s why I\u0027m a bit afraid. Should we like have an opt-in for that then ?",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "7e73f4d5_21cd88cc",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 174,
"author": {
"id": 11604
},
"writtenOn": "2023-05-24T17:02:30Z",
"side": 1,
"message": "it would have a perfroamce impact yes which is why it was orgianlly an oflien oepration done by the admin vai nova-manage.",
"parentUuid": "ff344a9d_7560317c",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "a1844e47_60fd4422",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 174,
"author": {
"id": 4393
},
"writtenOn": "2023-05-24T17:10:45Z",
"side": 1,
"message": "It\u0027s an extremely tiny impact.. A GET of an attachment by ID. In the scheme of a hard reboot, it\u0027s extremely minor. Considering the fallout from the recent CVE, it\u0027s also very easy to justify such things for safety :)",
"parentUuid": "7e73f4d5_21cd88cc",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "27f2f2be_7b13e69f",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 179,
"author": {
"id": 7166
},
"writtenOn": "2023-05-22T15:09:01Z",
"side": 1,
"message": "here, we need to make sure that Cinder-API service runs before, right?",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "6cff9dd0_fc8e3234",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 179,
"author": {
"id": 11604
},
"writtenOn": "2023-05-24T17:02:30Z",
"side": 1,
"message": "not nessisarly\nthe expectaation here is that if the cinder api is down we will still be able to boot vms using our exisitng bdms form the db\nwe jsut cant heal any deviation",
"parentUuid": "27f2f2be_7b13e69f",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "f0aa66fb_02add270",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 179,
"author": {
"id": 4393
},
"writtenOn": "2023-05-24T17:10:45Z",
"side": 1,
"message": "Yeah, when we discussed this we said that if we can\u0027t refresh the cache because cinder is down, just re-use what we have. It might be worth calling that out specifically just to avoid further confusion.",
"parentUuid": "6cff9dd0_fc8e3234",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "34072f9e_e10557af",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 179,
"author": {
"id": 11655
},
"writtenOn": "2023-05-25T17:22:54Z",
"side": 1,
"message": "++ This is the same approach we took in Ironic. Ensure ability to recover, but try and have the latest data possible in the event it is also needed to recover.",
"parentUuid": "f0aa66fb_02add270",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "9d68d92d_6a5accb4",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 209,
"author": {
"id": 11655
},
"writtenOn": "2023-05-25T17:22:54Z",
"side": 1,
"message": "Depending on the decision on how attachments are handled in other virt drivers, another item may be needed here since focusing only on the virtualization use case is fine, but we need to explicitly then ignore things for Bare Metal users, or encode the same basic model of interaction into place.",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "02331c06_a9d3beb7",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 229,
"author": {
"id": 7166
},
"writtenOn": "2023-05-22T15:09:01Z",
"side": 1,
"message": "which ones specifically ?",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "b3a2c853_fd4569c2",
"filename": "specs/2023.2/approved/cleanup-dangling-volume-attachments.rst",
"patchSetId": 12
},
"lineNbr": 229,
"author": {
"id": 34860
},
"writtenOn": "2023-05-26T06:21:29Z",
"side": 1,
"message": "will be adding releasenotes.\nplease suggest if any other doc required update.",
"parentUuid": "02331c06_a9d3beb7",
"revId": "befeb2b945cd58663433afb9b0cf50ba54c64c30",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
}
]
}
View Git Blame Copy Permalink