| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-03-23 | |||
| 00:50:37 | sean-k-mooney[m] | nova is not a collection of microservice by the way | |
| 00:50:57 | sean-k-mooney[m] | its a single distbuted service with multiple components that work together | |
| 00:52:50 | EugenMayer | why did the databases have been designed this way? AFAICS there are relations from one entity in one database to a different database - this really is not how this should be designed right. so nova::instance_extra.flavor<->nova_api::flavor.flavorid | |
| 00:53:20 | sean-k-mooney[m] | that is not how it works | |
| 00:53:42 | sean-k-mooney[m] | when you boot an instance we make a copy of the flavor and store a copy in the instance table | |
| 00:53:55 | EugenMayer | in which instance table: | |
| 00:53:57 | sean-k-mooney[m] | flavor are imuntable and cant be change once created | |
| 00:54:05 | sean-k-mooney[m] | however flavor extra specs are mutable | |
| 00:54:06 | EugenMayer | nova::instances ? | |
| 00:54:28 | sean-k-mooney[m] | so we have to copy the flavor to make sure existing instnace are not change if you modify the extra specs | |
| 00:55:26 | EugenMayer | i guess you the then reference nova::instances.vcpus/memory_mb/disk_db and so on | |
| 00:56:20 | sean-k-mooney[m] | yes those are fixed and cannot be modified. | |
| 00:56:35 | EugenMayer | while i understand that flavors are immutable (and should be) and the actual fields are copied / flatted into nova::instances -there is still a relation from nova::instance_extra to nova_api:falvor | |
| 00:57:00 | sean-k-mooney[m] | yes and no | |
| 00:57:16 | EugenMayer | and if this relation is broken, which very well can happen since there is no constraint possible - you can no longer do anything with the instance. You can no longer apply any new flavor or resize it | |
| 00:57:49 | EugenMayer | so entire decoupling has been planned maybe, but it seems not have been implemented (yet) | |
| 00:57:52 | sean-k-mooney[m] | you shoudl be able to today | |
| 00:57:55 | sean-k-mooney[m] | if you cant its a bug | |
| 00:58:17 | EugenMayer | well then there is a bug in xena (not sure what today standard is, yoga or xena?) | |
| 00:58:53 | sean-k-mooney[m] | so you should be aware that flavors used to be defiend at the cell level with cells v1 | |
| 00:59:31 | sean-k-mooney[m] | and when we moved to cells v2 we change the relation ships such that flavor were defiend gloablly in the api db | |
| 00:59:41 | sean-k-mooney[m] | but we conintued to keep the copy in the cell db | |
| 01:00:28 | sean-k-mooney[m] | but the intnace has used the copy in the request spec for schduling and instance extra as the source of truth for a vm for many many years | |
| 01:01:09 | EugenMayer | i understand, but there seems to be some usage of the back-reference anyway - maybe unintended | |
| 01:02:33 | sean-k-mooney[m] | so when we do a resize this is where we get the current flavor https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4127 | |
| 01:04:48 | EugenMayer | it seems, nevertheless, not the right design. nova should never include the actual flavorid for an instance, rathere nova_api should hold the reference from instance_uuid to flavorid - the relation owner here is nova_api. An instance is (as you explain) able to live without it's flavor - sureley a flavor can live without an instance too. But not | |
| 01:04:48 | EugenMayer | the relation. Sure the relation should be deleted in either way (deletion of instance or deletion of flavor). Anyway | |
| 01:05:32 | sean-k-mooney[m] | well you talking about something that has been in place for the better part of the last decade | |
| 01:05:36 | EugenMayer | looking at the code, nothing check for current_flavor to be null or not, it is used below right away. So if it is null / cannot be loaded (and not loaded does not trigger an exception already) this code will go bananas anway | |
| 01:05:52 | sean-k-mooney[m] | we have desgin constratint the mean we cant eaisly change the db schema | |
| 01:06:09 | sean-k-mooney[m] | espically for some of our larger deploymnets | |
| 01:07:48 | sean-k-mooney[m] | well the instance flavor should always be loadable | |
| 01:08:17 | sean-k-mooney[m] | the flavor is not nullable | |
| 01:09:24 | sean-k-mooney[m] | https://github.com/openstack/nova/blob/master/nova/objects/instance.py#L336 we load the flaovor form the copy in instnace extra | |
| 01:09:26 | EugenMayer | since github does not let me resolve those symbols i cannot see what get_flavor will do if the referenced flavor is not present in nova_api::flavor but i can tell you that this will not work for xena at least | |
| 01:10:43 | EugenMayer | this cannot be rigbth | |
| 01:10:54 | EugenMayer | current_flavor['name'] is accessed, this is not part of instance_extras | |
| 01:11:22 | EugenMayer | most probably the data is expanded using the nova_api::flavor table? | |
| 01:12:02 | sean-k-mooney[m] | where are you seeing current_flaovr[name] | |
| 01:12:28 | sean-k-mooney[m] | and no once the instance object is created and save to the db we dont use the flaovr form the api db any more | |
| 01:12:48 | sean-k-mooney[m] | we should be either using the one in the request spec or the one in instance extra | |
| 01:13:08 | EugenMayer | https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4162 | |
| 01:13:51 | sean-k-mooney[m] | current_flavor is instance.get_flavor | |
| 01:13:58 | EugenMayer | :) | |
| 01:14:09 | sean-k-mooney[m] | which get the filed form the instnace object | |
| 01:14:35 | sean-k-mooney[m] | the instnace object as i pointed too loads the flavor form the instance_extra table in the cell db | |
| 01:15:25 | EugenMayer | https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4142 proves that new_flavor must match the interface of current_flavor, while new_flavor https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4144 is a flavor entity .. and we can see | |
| 01:15:25 | EugenMayer | https://github.com/openstack/nova/blob/0d1dd103d1431400b04f5f3edcb0d48453a79151/nova/compute/flavors.py#L131 .. the comment of that message already tells at, it will use nova_apis database | |
| 01:16:08 | sean-k-mooney[m] | that method will yes but that is not what we are calling | |
| 01:16:39 | sean-k-mooney[m] | that is in the flavor api code | |
| 01:16:49 | sean-k-mooney[m] | that is just doing a lookup in the api db directly | |
| 01:16:54 | sean-k-mooney[m] | resize is not calling that | |
| 01:17:24 | sean-k-mooney[m] | well for the current_flavor | |
| 01:17:35 | sean-k-mooney[m] | it calls that for the new flavor that you are resizeing too | |
| 01:17:38 | EugenMayer | https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4142 is telling us that whatever current_flavor is, it must match the same interface we load in https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4144 | |
| 01:18:24 | sean-k-mooney[m] | that is cold migrate | |
| 01:18:35 | sean-k-mooney[m] | resize is used for both flavor resize and cold migration | |
| 01:19:06 | EugenMayer | and the latter is a fully loaded flavor from the nova_api::flavor table. And this explains why https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4162 is the assumed, since 'name' is the field nova_api::flavor.name | |
| 01:19:10 | sean-k-mooney[m] | line 4142 is the cold migrate path | |
| 01:19:33 | sean-k-mooney[m] | 4144 is the resize and its loading the new flavor from the api db | |
| 01:19:38 | sean-k-mooney[m] | not the current flavor | |
| 01:19:44 | sean-k-mooney[m] | the new flavor must exist | |
| 01:19:56 | EugenMayer | maybe, but https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4162 accesses a field that could never have been loaded from nova::instance_extra | |
| 01:20:23 | sean-k-mooney[m] | why? | |
| 01:20:32 | EugenMayer | there is no such field? | |
| 01:20:59 | sean-k-mooney[m] | the flaovr is stored as a json blob in the instance extra table | |
| 01:21:13 | sean-k-mooney[m] | so name would be in the blob not a colum in that table | |
| 01:22:19 | EugenMayer | i see that instance_extra::flavor is a json serialized model of the flavor | |
| 01:22:32 | sean-k-mooney[m] | yes | |
| 01:22:38 | sean-k-mooney[m] | we serialse the ovo | |
| 01:22:50 | sean-k-mooney[m] | and store it in the flaovr column | |
| 01:22:51 | EugenMayer | and there is the name field indeed | |
| 01:23:34 | sean-k-mooney[m] | so other then the new flavor we shoudl not have a depency on the old flavor exising | |
| 01:23:47 | EugenMayer | so this is deserialized. I understand. Then there is something else trying to access the 'flavor' of the 'old flavor' which leads to an exception | |
| 01:23:48 | sean-k-mooney[m] | in the api db | |
| 01:24:10 | EugenMayer | i see the idea here, got you. But there must be still a expectation left | |
| 01:24:39 | sean-k-mooney[m] | there may be an in correct lookup using the id or some other reference | |
| 01:24:53 | EugenMayer | i guess you can easily try that. Create an instance with a flavor, then change the flavor id in nova_api::flavor and you should no longer be able to resize the instance | |
| 01:25:14 | sean-k-mooney[m] | we could test it in a functional test | |
| 01:25:37 | sean-k-mooney[m] | create a flavor , boot an instnace ,delete the flavor then try to resize | |
| 01:25:42 | EugenMayer | you should see, when using `openstack server resize` - 409 | |
| 01:26:17 | sean-k-mooney[m] | well we should see a 200 :) but we might see a 409 indeed | |
| 01:26:26 | sean-k-mooney[m] | that would be a bug which we should fix | |
| 01:26:42 | EugenMayer | I would have loved to see a 200 :) | |
| 01:29:30 | sean-k-mooney[m] | so looking at the functional test quickly im not seeing a test for resizing after flavor delete in test_server.py | |
| 01:30:07 | sean-k-mooney[m] | we can try creating one tomorrow/later today and see if we can repoduce | |
| 01:31:26 | sean-k-mooney[m] | do you have teh stack trace by the way | |
| 01:32:20 | sean-k-mooney[m] | i can try and take a look at it in the morning its 01:30 here so i was just heading to sleep but i can take another look once im online tomorrow | |
| 01:32:39 | EugenMayer | well that's the funny thing, noboday on the 'user side' ever knows which sub-system is responsible here | |
| 01:32:52 | EugenMayer | so grabbing a stacktrace is not as easy as you might think :) | |
| 01:33:11 | EugenMayer | it's 2:30 here, so i'am with you | |
| 01:33:35 | EugenMayer | sure, i'll be online tomorrow, just mention me. Happy to help if i can. Thank you for your effort! | |
| 01:34:10 | sean-k-mooney[m] | EugenMayer: ah your in europe then i tought you were across the pond. ok ya ill see if i can put up a DNM patch with a simple resize test tomorrow and we will see if i can trigger it | |
| 01:34:46 | EugenMayer | sure! Have a good night then, i'am off to bed :) | |
| 02:51:36 | opendevreview | anguoming proposed openstack/nova master: fix the bug of the log line has no request_id info at source host when live migration https://review.opendev.org/c/openstack/nova/+/834677 | |
| 06:57:00 | opendevreview | melanie witt proposed openstack/nova master: block_device_info: Add swap to inline https://review.opendev.org/c/openstack/nova/+/826523 | |
| 06:57:00 | opendevreview | melanie witt proposed openstack/nova master: libvirt: Improve creating images INFO log https://review.opendev.org/c/openstack/nova/+/826524 | |
| 06:57:01 | opendevreview | melanie witt proposed openstack/nova master: libvirt: Remove defunct comment https://review.opendev.org/c/openstack/nova/+/826525 | |
| 06:57:01 | opendevreview | melanie witt proposed openstack/nova master: imagebackend: default by_name image_type to config correctly https://review.opendev.org/c/openstack/nova/+/826526 | |
| 06:57:02 | opendevreview | melanie witt proposed openstack/nova master: image_meta: Add ephemeral encryption properties https://review.opendev.org/c/openstack/nova/+/760454 | |