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

465 lines
20 KiB
Plaintext
Raw Blame History

{
"comments": [
{
"unresolved": false,
"key": {
"uuid": "01e103f4_e4e38e57",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 1
},
"lineNbr": 0,
"author": {
"id": 33634
},
"writtenOn": "2022-11-07T15:26:48Z",
"side": 1,
"message": "This is the revised version of the spec once approved for Zed (https://review.opendev.org/c/openstack/nova-specs/+/816542) for updating user data, including the improved design we have discussed.\n\nPlease let me know what you think or if anything is missing.",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "18d4ffed_7ce30da6",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 1
},
"lineNbr": 0,
"author": {
"id": 7166
},
"writtenOn": "2022-11-15T10:28:07Z",
"side": 1,
"message": "I\u0027m pretty OK with the direction but I don\u0027t want the Nova API to present a specific state that we would need to keep forever while we already have an existing API that helps to know the right status.",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "17a55e7b_63ebf12f",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 1
},
"lineNbr": 0,
"author": {
"id": 11604
},
"writtenOn": "2022-11-15T14:23:52Z",
"side": 1,
"message": "this is not inline with the direction we discussed after FeatureFreeze.\n\n",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "89b7ce4f_fd71356c",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 1
},
"lineNbr": 0,
"author": {
"id": 32755
},
"writtenOn": "2022-11-18T14:35:15Z",
"side": 1,
"message": "Hey all. Jan and I dove into the spec and relevant sections of existing code and replied inline to your feedback.\n\nPlease kindly take another look, so we can update the spec again accordingly.",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "119d48b1_5bec5c4e",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 55,
"author": {
"id": 7166
},
"writtenOn": "2022-11-15T10:28:07Z",
"side": 1,
"message": "fwiw, this should also be seen in the os-instance-actions API as another action.",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "e5821286_6b7cb50c",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 55,
"author": {
"id": 32755
},
"writtenOn": "2022-11-18T14:35:15Z",
"side": 1,
"message": "Yes. Does this need particular mentioning in the spec that this new server action is then accessible via the os-instance-actions API? So I am asking if there is a required change / extention of that API or if that is just working for any type of action anyways.",
"parentUuid": "119d48b1_5bec5c4e",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "5ababa78_d7ba0a97",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 55,
"author": {
"id": 11604
},
"writtenOn": "2022-11-18T14:50:19Z",
"side": 1,
"message": "there is no requried change to os-instance-actions enpoint but we need to ensure that the user data update is recoded properly there.\nso that there is an audit log and you can check the status/error or the action.\n\nthis is just perserving the same behaivor we have for other instance actions.",
"parentUuid": "e5821286_6b7cb50c",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "124d0f81_ed23b983",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 55,
"author": {
"id": 7166
},
"writtenOn": "2022-12-14T14:45:38Z",
"side": 1,
"message": "My point was to make sure you will add this when implementing, which is not what I see in the spec.\n\nCould you please modify the spec to explain about this please ?",
"parentUuid": "5ababa78_d7ba0a97",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "457560e3_c99886ef",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 55,
"author": {
"id": 11604
},
"writtenOn": "2022-12-14T15:33:03Z",
"side": 1,
"message": "basically sylvain is asking you to add this either to the work itmes or explcitly say a new instance cation event will be recoreded in the db.\n\nthis will also have an impact on the notifactions since we will have a new action start and end notifcation",
"parentUuid": "124d0f81_ed23b983",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "554dae3e_d306c09a",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 55,
"author": {
"id": 33634
},
"writtenOn": "2022-12-19T12:42:30Z",
"side": 1,
"message": "Done",
"parentUuid": "457560e3_c99886ef",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "0321c63d_a22d22f5",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 73,
"author": {
"id": 7166
},
"writtenOn": "2022-11-15T10:28:07Z",
"side": 1,
"message": "I\u0027m not really happy with this. I\u0027d rather prefer to continue having the same behaviour like for other asynchronous actions we have : for each action we have, we can have a list of events.\n\n\nIn the case of the action named \u0027userdata_update\", I\u0027d then prefer to have an event compute_userdata_updated to only be sent once the reboot is made in the case of a configdrive, and straight emitted in the case of a metadata.\n\nThe user would then see whether the userdata is eventually updated by looking at the Nova os-instance-actions API for that specific action and verify whether the event was emitted or not.",
"range": {
"startLine": 72,
"startChar": 0,
"endLine": 73,
"endChar": 36
},
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "6b5a00a7_2ba63734",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 73,
"author": {
"id": 11604
},
"writtenOn": "2022-11-15T14:23:52Z",
"side": 1,
"message": "we agreed to not use the varible in system metadata approch.\n\ninstead what we discussed was changing the precondtion to require the server to be stopped/shelved and then having the new instance action regenerate the config drive .",
"parentUuid": "0321c63d_a22d22f5",
"range": {
"startLine": 72,
"startChar": 0,
"endLine": 73,
"endChar": 36
},
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "27f13543_dff9f939",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 73,
"author": {
"id": 32755
},
"writtenOn": "2022-11-18T14:35:15Z",
"side": 1,
"message": "1. If an instance must be \"stopped\" in order for userdata updates while a config_drive is used, there is no \"pending\" state the likes of USERDATA_UPDATE_REQUIRES_REBOOT necessary, I agree.\n\n2. If the instance is not in a suitable state the action will just be rejected immediately.\n\n3. If accepted the action emits an event of either \"compute_userdata_updated\" or \"failed\" for that action.\n\n4. May I argue that \"shelved\" instances would not even require an active rebuild of the config drive. While shelved the instance is not currently \"living\" on a compute node and actually does not have a config drive? Or am I mistaken? \n\nIf that was the case, shelved instances with config drive would also allow for immediate updates of the user_data in the DB (like with no config drive).",
"parentUuid": "6b5a00a7_2ba63734",
"range": {
"startLine": 72,
"startChar": 0,
"endLine": 73,
"endChar": 36
},
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "5b084f56_e16725f0",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 73,
"author": {
"id": 11604
},
"writtenOn": "2022-11-18T14:50:19Z",
"side": 1,
"message": "correct shelve_offloaded instance do not need the config drive to be rebuilt at all becasue it will be recreated when it unshelved wiht the most up to date data.\n\n\nand yes if the instance is not in either the stopped or shelve_offloaded state teh instace actuion shoudl return a 409 conclict since the preconditon is not met.\n\nthere is no need to emit an event, the instance action will etiher be accpected in or rejected based on the state of the vm. the instance action event log will contain the result.\n\nim debating if this should be an async operation\n\ni think it can by syncornos in which case the api responce will contain the succes but we could make it async in which case the api would return a 202 accpted and then you would check the instace action list api to see if it succeded",
"parentUuid": "27f13543_dff9f939",
"range": {
"startLine": 72,
"startChar": 0,
"endLine": 73,
"endChar": 36
},
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "b3958fb5_3863d5c0",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 73,
"author": {
"id": 32755
},
"writtenOn": "2022-11-22T16:13:30Z",
"side": 1,
"message": "\u003e im debating if this should be an async operation\n\u003e \n\u003e i think it can by syncornos in which case the api responce will contain the succes but we could make it async in which case the api would return a 202 accpted and then you would check the instace action list api to see if it succeded\n\nCertainly giving the user instant feedback on a successful API call is nice.\nBut are not ALL actions async by definition? So the request is responded with 202 and then the result is fetched via the instance action API as you said?",
"parentUuid": "5b084f56_e16725f0",
"range": {
"startLine": 72,
"startChar": 0,
"endLine": 73,
"endChar": 36
},
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "9eff24d0_f9b97a09",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 73,
"author": {
"id": 11604
},
"writtenOn": "2022-12-13T18:42:21Z",
"side": 1,
"message": "we server operations that synconous like\nhttps://docs.openstack.org/api-ref/compute/?expanded\u003d#create-console\n\nServer Diagnostics or server metadata\n\nbut you are right ath other then evacuate the rest all return 202.\nthe fact evacuate returns 200 is proably a bug too.\n\nreturning a 202 is likely more consitent so lets stick with that pattern.",
"parentUuid": "b3958fb5_3863d5c0",
"range": {
"startLine": 72,
"startChar": 0,
"endLine": 73,
"endChar": 36
},
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "7add7951_d21d59ae",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 81,
"author": {
"id": 9708
},
"writtenOn": "2022-11-15T14:32:49Z",
"side": 1,
"message": "I might missing some context here but I thought the main feedback from Dan was to bump the RPC version and send the regeneration flag as an RPC parameter. https://review.opendev.org/c/openstack/nova/+/816157/16#message-500f190a16005ff87099d5c6ba35b3e0b31f4737",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "4fbc585f_dc1937c3",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 81,
"author": {
"id": 4690
},
"writtenOn": "2022-11-16T04:10:32Z",
"side": 1,
"message": "That was my understanding as well, to not create a \"shadow RPC\" call in this manner.",
"parentUuid": "7add7951_d21d59ae",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "a2057979_ed4efb1f",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 81,
"author": {
"id": 32755
},
"writtenOn": "2022-11-18T14:35:15Z",
"side": 1,
"message": "Our bad. I suppose this RPC bump would be similar to the recently added \"support for volume backed server rebuild\" (https://opendev.org/openstack/nova/commit/30aab9c234035b49c7e2cdc940f624a63eeffc1b), right?\n\nSo the idea is to add a \"rebuild_configdrive\" RPC-method that requires a certain RPC version. That\u0027s much cleaner, I totally agree.\n\nJust one question remains: Where to store / how to handle the new user_data while the \"update_userdata\" action is not done? (see previous discussion: https://review.opendev.org/c/openstack/nova/+/816157/comments/033c59ff_53cd6b34).\n\n1. Should we send it via RPC to the agent then rebuilding the config_drive and on success update the user_data in the database and set the action to \"successful\"?\n\n2. Or should we just attach the action ID via RPC to the agent and rather have it \"fetch\" the userdata from the action?",
"parentUuid": "4fbc585f_dc1937c3",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "2f064a40_18e15037",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 81,
"author": {
"id": 11604
},
"writtenOn": "2022-12-13T18:42:21Z",
"side": 1,
"message": "we have 2 options\n\neither the payload of the new instance action can be the content of the new userdtata which we can pass via rpc to the comptue which will update it in the db and regenerate teh confgi drive.\n\n\nor the new action should take no payload adn just triger the config drive to be regenerated from the current metadata.\n\nso i think option 1 include the data in the rpc and commit it to the db from the compute.\n\nthat a bit inefficent but it proably is the more consitent way to do this.\n\nyour option 2 will not work as there is no way for the agent to \"fetch\" the userdata form the action.",
"parentUuid": "a2057979_ed4efb1f",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "a1c017f0_a80d12f0",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 81,
"author": {
"id": 9708
},
"writtenOn": "2022-12-14T14:12:49Z",
"side": 1,
"message": "as far as I see user_data is part of the Instance object we send in most of our RPC calls anyway. So if the API loads the instance from the DB updates the user_data in it in memory and sends down that instance object to the compute via the new RPC then 1) the compute will see the new user_data in the instance, 2) it can call instance.save() to persist the new user_data after the config driver regeneration succeeded. So option 1 looks OK to me.",
"parentUuid": "2f064a40_18e15037",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "65d51852_831c6c21",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 81,
"author": {
"id": 7166
},
"writtenOn": "2022-12-14T14:45:38Z",
"side": 1,
"message": "Here the context from Dan is to make sure we can ping the nova-compute service by RPC for knowing whether it can recreate the configdrive.\n\nThis is not really a concern on how to save the modified userdata. So, while I agree with gibi here on the fact an instance.save() is enough to persist the modification, we still need to make sure we can have a specific RPC call for it.",
"parentUuid": "a1c017f0_a80d12f0",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "4c7c9b0d_6d275c01",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 81,
"author": {
"id": 11604
},
"writtenOn": "2022-12-14T15:33:03Z",
"side": 1,
"message": "well the specific RPC call is the new one that is intoduced for this instance action so i think we are all in agreement that option 1 is the path to take.\n\non the api side fi the compute node is two old the rpc call will fail so that provides the protection we require to mark the instance action as failed and ensure the state in the db is not modifed.",
"parentUuid": "65d51852_831c6c21",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "9a8f7914_d101e239",
"filename": "specs/2023.1/approved/update-userdata.rst",
"patchSetId": 1
},
"lineNbr": 81,
"author": {
"id": 33634
},
"writtenOn": "2022-12-19T12:42:30Z",
"side": 1,
"message": "Done",
"parentUuid": "4c7c9b0d_6d275c01",
"revId": "a5d8feafebbee34b4b3f10789c9923e6adbabe63",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
}
]
}
View Git Blame Copy Permalink