| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-06 | |||
| 14:37:21 | mriedem | the data migration also sets queued_for_delete=False on instance mappings for non-deleted instances | |
| 14:37:28 | mriedem | https://review.openstack.org/#/c/584504/5/nova/objects/instance_mapping.py@179 | |
| 14:37:41 | tssurya | but we don't care for the new ones right ? for them None = False () sort of and when they get deleted it would become True() from None that's all | |
| 14:38:06 | tssurya | mriedem: yea I understand | |
| 14:38:16 | tssurya | if we feel its a bug for the migration to be always incomplete | |
| 14:38:53 | mriedem | well generally data migrations are something you'd like to have an end state | |
| 14:39:01 | tssurya | then we can fix. | |
| 14:39:06 | tssurya | (anu suggestions ?) | |
| 14:39:08 | tssurya | any* | |
| 14:39:59 | dansmith | I'm confused about what the problem is | |
| 14:40:44 | mriedem | started with my confusion over the commit message and comments in https://review.openstack.org/#/c/635120/ | |
| 14:41:03 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove get_config_vhostuser https://review.openstack.org/565471 | |
| 14:41:03 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Set bandwidth limits for generic vHost User interfaces https://review.openstack.org/635170 | |
| 14:41:04 | mriedem | and tssurya said that with a new instance mapping in devstack today, the queued_for_delete in the DB is NULL even though we default to False in the object and sqla model code | |
| 14:41:12 | mriedem | which ^ doesn't make sense to me | |
| 14:41:24 | mriedem | i get that we don't set False for the server_default | |
| 14:41:36 | mriedem | but inserting new records should have a default value of False in the DB i'd think | |
| 14:41:41 | stephenfin | mriedem, jaypipes: I've split out that bandwidth limits fix to a separate patch, if you fancy taking a quick look over it again https://review.openstack.org/565471 https://review.openstack.org/635170 | |
| 14:42:03 | mriedem | stephenfin: only if i can raise my pinky while reviewing it | |
| 14:42:04 | tssurya | mriedem: we don't default to false in the api_models code right ? that was changed for the migration | |
| 14:42:16 | tssurya | which is why this is happening I think | |
| 14:42:22 | dansmith | tssurya: right but we should be passing it from the object always | |
| 14:42:29 | dansmith | if not, then that's a problem | |
| 14:42:32 | mriedem | ok then i missed https://review.openstack.org/#/c/584504/5/nova/db/sqlalchemy/api_models.py | |
| 14:43:11 | mriedem | we're not passing it from the object b/c the field isn't set | |
| 14:43:18 | mriedem | https://review.openstack.org/#/c/584504/5/nova/objects/instance_mapping.py@102 | |
| 14:43:59 | mriedem | https://review.openstack.org/#/c/584504/5/nova/compute/api.py@938 | |
| 14:44:00 | dansmith | we have a default on the field, but that doesn't mean it gets set, | |
| 14:44:05 | mriedem | right | |
| 14:44:06 | dansmith | so maybe whoever wrote that code thought it would be | |
| 14:44:07 | mriedem | so it's not in the changes | |
| 14:44:30 | mriedem | anyway it's a simple fix | |
| 14:44:51 | dansmith | yeah, so create() should always set that =False and destroy should set it =True | |
| 14:44:53 | mriedem | changes.setdefault('queued_for_delete', False) | |
| 14:45:00 | dansmith | I'll work up a patch | |
| 14:45:13 | dansmith | mriedem: I think it'd be better to be explicit about it | |
| 14:45:18 | dansmith | because we have to be on delete anyway | |
| 14:45:32 | tssurya | destroy removes the mapping right ? | |
| 14:45:40 | dansmith | tssurya: got a bug for this? | |
| 14:45:48 | tssurya | dansmith: thanks I will open a bug now | |
| 14:45:54 | dansmith | oh right, no soft delete here | |
| 14:45:55 | tssurya | and assign it to you | |
| 14:46:05 | dansmith | when does it get set =True then? | |
| 14:46:07 | mriedem | right, we set qfd = True in the API https://github.com/openstack/nova/blob/master/nova/compute/api.py#L2192 | |
| 14:46:11 | dansmith | I clearly don't remember this | |
| 14:46:15 | dansmith | oh righ on instance deete duh | |
| 14:46:20 | tssurya | during nova delete's | |
| 14:49:05 | dansmith | ah and we have it in our tests | |
| 14:49:19 | dansmith | like, explicitly setting it =False | |
| 14:52:40 | tssurya | damsith: https://bugs.launchpad.net/nova/+bug/1814913 | |
| 14:52:41 | openstack | Launchpad bug 1814913 in OpenStack Compute (nova) "A new instance_mapping record will have queued_for_delete set to NULL" [Undecided,New] - Assigned to Dan Smith (dansmith) | |
| 14:53:00 | tssurya | dansmith * sorry | |
| 14:53:04 | dansmith | thanks, patch shortly | |
| 14:53:16 | mriedem | high fives for team work | |
| 14:54:00 | tssurya | I will change the patch then to no longer have to consider "None" values | |
| 14:54:38 | bauzas | mmm, we don't document the reasons why flavors are immutable, neither in https://docs.openstack.org/nova/latest/admin/flavors.html or https://docs.openstack.org/nova/latest/user/flavors.html | |
| 14:54:40 | dansmith | tssurya: your patch? you still need to handle none values | |
| 14:54:49 | dansmith | tssurya: meaning you can't fail if they're there | |
| 14:55:15 | tssurya | dansmith: I mean this one: https://review.openstack.org/#/c/635120/ | |
| 14:55:19 | bauzas | I have a downstream request about modifying the nested flavor by not resizing, so I'd like to explain to the customer why I don't like it | |
| 14:55:23 | sean-k-mooney | bauzas: they are immutable as if they were not existing instaces may exceed available resouces | |
| 14:55:33 | dansmith | tssurya: ah okay | |
| 14:55:39 | tssurya | to get non-deleted I only need to ask for "False()" right ? assuming people ran the migration | |
| 14:55:54 | bauzas | yeah, I know *why*, but I'd like to see the fact that you can't modify a nested flavor to be documented | |
| 14:55:58 | dansmith | tssurya: no | |
| 14:56:05 | openstackgerrit | Yury Kulazhenkov proposed openstack/nova master: rename ScaleIO driver to VxFlex Os https://review.openstack.org/634866 | |
| 14:56:09 | dansmith | tssurya: you can't assume the migration was run until it's forced really | |
| 14:56:17 | sean-k-mooney | bauzas: the downstream issue is related to the flavor extra specs not the flaovr | |
| 14:56:27 | bauzas | yeah | |
| 14:56:38 | sean-k-mooney | bauzas: but yes it would be nice to have that documented | |
| 14:57:09 | tssurya | dansmith: even then now that you are fixing it, it would be wrong for me to have a logic where I am also querying for the None ones thinking they are non-deleted | |
| 14:57:11 | bauzas | tbh, I don't like to be able to modify the extra specs but... | |
| 14:57:28 | tssurya | because it could just be that the user has not run the migration | |
| 14:57:57 | dansmith | tssurya: if it's null it means we don't know if it's queued for delete or not, so pick one.. should the user see their instance go away or come back from the dead? it has to be one or the other | |
| 14:59:00 | tssurya | dansmith: hah I don't have a strong opinion on that probably "come back from dead" is better | |
| 14:59:08 | tssurya | so I will just leave my change as it is then | |
| 14:59:28 | tssurya | and just edit the commit/comment | |
| 15:01:58 | sean-k-mooney | bauzas: we proably should update https://github.com/openstack/nova/blob/master/doc/source/admin/flavors.rst#modify-a-flavor | |
| 15:02:32 | bauzas | I'm out of steam for that but yeah | |
| 15:02:46 | mriedem | bauzas: so add something about that here https://docs.openstack.org/nova/latest/admin/flavors.html#modify-a-flavor | |
| 15:03:55 | sean-k-mooney | bauzas: i can add a note if you like | |
| 15:04:08 | mriedem | you can copy what's in https://specs.openstack.org/openstack/nova-specs/specs/queens/implemented/flavor-description.html if you want | |
| 15:04:15 | mriedem | " Note The only field that can be updated is the description field. Nova has historically intentionally not included an API to update a flavor because that would be confusing for instances already created with that flavor. Needing to change any other aspect of a flavor requires deleting and/or creating a new flavor. " | |
| 15:05:29 | sean-k-mooney | mriedem: ya that makes sense. bauzas want me to do it since your busy or will i leave it to you. | |
| 15:05:59 | mriedem | we can also update the osc command note in https://docs.openstack.org/nova/latest/admin/flavors.html#modify-a-flavor to mention that support was added in openstack CLI in 3.16.0: https://review.openstack.org/#/c/575390/ | |
| 15:06:17 | mriedem | or just change the example to use osc | |
| 15:06:20 | mriedem | and drop the nonte | |
| 15:06:22 | mriedem | *note | |
| 15:06:30 | sean-k-mooney | sure | |
| 15:06:37 | bauzas | actually, the spec is a good example | |
| 15:06:43 | sean-k-mooney | ill go do that now | |
| 15:06:54 | bauzas | thanks | |
| 15:07:12 | sean-k-mooney | bauzas: ya you can use the spec as a reference for the customer while i update this | |
| 15:07:25 | bauzas | yeah | |
| 15:07:35 | bauzas | but anyway, I need to provide a hack :( | |
| 15:07:40 | bauzas | and I hate this | |
| 15:07:46 | sean-k-mooney | well you dont | |
| 15:07:56 | sean-k-mooney | we shoud discuss that downstream | |
| 15:07:59 | bauzas | oh yeah, I have to do it | |
| 15:08:03 | bauzas | trust me | |