| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-06 | |||
| 14:29:30 | tssurya | that is what I mean by new | |
| 14:29:47 | mriedem | and those should have queued_for_delete=False in the db by default | |
| 14:29:55 | tssurya | which they don't | |
| 14:30:05 | mriedem | ? | |
| 14:30:20 | tssurya | I thought they should have qfd=False by default too | |
| 14:30:30 | tssurya | but in the new devstack in which I tested they are NULL | |
| 14:30:32 | tssurya | by default | |
| 14:30:33 | mriedem | if they don't, then we have a problem because we'd never be able to complete that data migration | |
| 14:30:54 | tssurya | well I am assuming once we run the migration, we would not run it again | |
| 14:31:20 | mriedem | but if you're saying that new instance mappings records don't have queued_for_delete=false in the db, then we'd have to migrate those as well | |
| 14:31:33 | tssurya | would have to ask dansmith | |
| 14:31:33 | mriedem | b/c the migration is looking for queued_for_delete=None | |
| 14:31:57 | tssurya | but I am thinking for the newer ones None also means not queued_for_delete | |
| 14:32:17 | tssurya | and when they get deleted it would become True | |
| 14:32:32 | dansmith | why ask me? :) | |
| 14:33:08 | dansmith | if we're writing the qfd flag on object save, then new records should have =False, if not then it depends on whether or not we have a default in the schema | |
| 14:33:18 | tssurya | https://review.openstack.org/#/c/584504/5//COMMIT_MSG@16 | |
| 14:34:24 | dansmith | oh ;) | |
| 14:34:39 | dansmith | yeah, I remember this being some weird detail when writing that migration but I don't remember why | |
| 14:35:00 | tssurya | to catch all the None and change them to True/False | |
| 14:35:19 | tssurya | which made sense | |
| 14:35:34 | tssurya | so I am assuming we were ok with have new records that would be None by default right ? | |
| 14:35:38 | mriedem | no | |
| 14:35:38 | tssurya | having* | |
| 14:35:46 | mriedem | because then the data migration will never be complet | |
| 14:35:48 | mriedem | *complete | |
| 14:36:09 | mriedem | new records should have False in the db when we insert the record | |
| 14:36:17 | dansmith | oh right right | |
| 14:36:22 | tssurya | mriedem: yea the data migration will never be complete, but I am assuming it will be removed eventually and the goal of that migration was to update the deleted instances to True | |
| 14:36:28 | dansmith | because they need to be either true or false null means unmigrated | |
| 14:36:55 | dansmith | tssurya: no, I think the intent is to update them to true or false right? | |
| 14:36:56 | tssurya | heh ok I didn't think it was bug, but maybe it is ? :D | |
| 14:37:07 | tssurya | dansmith: yea for the old instances | |
| 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: Set bandwidth limits for generic vHost User interfaces https://review.openstack.org/635170 | |
| 14:41:03 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove get_config_vhostuser https://review.openstack.org/565471 | |
| 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 | |