| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-10-09 | |||
| 16:25:48 | dansmith | repeat on 1, done on 0, 2 means we hit terminal fail state | |
| 16:26:24 | dansmith | either way people that loop on nonzero will break with anything you're going to do, which is why reno and doc is super important | |
| 16:26:39 | imacdonn | right | |
| 16:27:37 | dansmith | not sure how I feel about changing behavior in a backport with a retroactive reno, but mriedem is the authority here, so I'd do whatever he says | |
| 16:28:21 | imacdonn | I'm thinking that most people only read release notes for a new release, not for errata updates | |
| 16:28:43 | dansmith | unfortunately I don't think they even read them for new releases, but.. yeah | |
| 16:28:49 | imacdonn | heh yeah, there is that | |
| 16:30:25 | mriedem | i'll defer to tonyb | |
| 16:31:21 | dansmith | the AUD stops with tonyb | |
| 16:36:50 | melwitt | . | |
| 16:43:21 | mriedem | i get it | |
| 16:43:24 | mriedem | took me awhile | |
| 16:48:42 | imacdonn | dansmith: I'm on the fence about your last suggestion .... I think I tend towards exceptions being bad, requiring some problem to be addressed immediately ... but then there was a suggestion that some migrations may raise exceptions "by design" if some other migration had not yet been completed | |
| 16:49:26 | dansmith | imacdonn: well, by design or not, we've had some that won't complete until others do | |
| 16:55:09 | imacdonn | dansmith: is it defined somewhere that a migration should raise an exception in such a case? (as opposed to just not doing any work) Seems like ideally there should be a way for a migration to explicitly state that "I can't do this *yet*" | |
| 16:55:39 | dansmith | the way to do that is to return nonzero found, with zero done | |
| 16:56:02 | imacdonn | so how do you distinguish that from "I can't do this *all all, ever*" ? | |
| 16:56:20 | dansmith | regardless, because of the complexity of hitting all the cases of live data, which we're historically bad at doing, making the process stop on exception is just practically not the best plan, IMHO | |
| 16:56:52 | dansmith | there's no case where found is nonzero where done is expected to remain zero forever | |
| 16:56:58 | dansmith | found is items that should be migratable | |
| 16:59:51 | imacdonn | OK | |
| 17:00:05 | imacdonn | I'll try to implement that and see what falls out | |
| 18:30:39 | sean-k-mooney | melwitt: i left some feedback in https://review.openstack.org/#/c/575735/2 fyi. hope that helps. the code should work but its duplicating logic that is not needed. | |
| 18:31:46 | melwitt | sean-k-mooney: cool thanks | |
| 18:43:52 | mriedem | dansmith: off the top of your head, do you know much about the migration_context we stash on the instance during cold migration / resize (created by the RT move claim) and what we need it for besides routing neutron events to the source and dest host? looks like it's otherwise for tracking numa/pci on the source and dest host, | |
| 18:44:21 | mriedem | reason i ask is because i'm currently using a move claim for the cross-cell resize but i have alternatives to using a resize_claim, | |
| 18:44:29 | mriedem | both claim ways in the RT are kind of weird for how i'm doing this | |
| 18:46:31 | mriedem | re: https://review.openstack.org/#/c/603930/9/nova/compute/resource_tracker.py and https://review.openstack.org/#/c/603930/9/nova/compute/manager.py@5138 | |
| 18:49:49 | mriedem | i don't think i need to care about the migration_context for the same reasons as normal resize because for cross-cell, we'll have shelved offloaded from the source by the time we claim on the dest, so meh | |
| 18:50:07 | dansmith | mriedem: well, the point of it was to avoid doing things like looking up the most recent unfinished migration for an instance in order to get at things we were going to stash on there | |
| 18:50:19 | dansmith | probably for things that aren't covered by the old/new flavor, so yeah probably numaish things | |
| 18:50:42 | dansmith | maybe, but I guess I'd hope that we could make it as similar of a process as possible, | |
| 18:50:58 | dansmith | and the fact that we only have migration-context for cold moves right now is unfortunate I think | |
| 18:51:22 | mriedem | well, we can make it similar, but things get weird if we do, as noted in those links above | |
| 18:52:10 | mriedem | i just haven't ran into anything with this that requires needing the migration_context being set on the instance | |
| 18:52:52 | dansmith | we use it for directing notifications to both computes right? | |
| 18:53:32 | mriedem | yes, but i don't really need that with cross-cell resize, | |
| 18:53:44 | mriedem | because sending an event to the source is useless b/c we've shelved offloaded from the source | |
| 18:54:05 | mriedem | when we unshelve on the target, the instance.host gets set and the API will route the event there | |
| 18:54:05 | dansmith | sure, I was just responding to "we don't use it anywhere" | |
| 18:54:49 | mriedem | i also have to do cludgy shit like this https://review.openstack.org/#/c/603930/9/nova/compute/manager.py@5189 | |
| 18:54:52 | mriedem | if not using an instance_claim | |
| 18:54:54 | dansmith | so, one question I had but was saving was.. are you going to have a resize operation end up going through SHELVED_OFFLOADED from the user's perspective? | |
| 18:55:16 | mriedem | the terminal state is VERIFY_RESIZE for the user | |
| 18:55:58 | mriedem | i have a TODO related to that https://review.openstack.org/#/c/603930/9/nova/compute/manager.py@5024 | |
| 18:56:47 | dansmith | because the context would be one place to stash what we're doing to hide that | |
| 18:56:51 | mriedem | i think i need to leave the task_state set there, | |
| 18:57:08 | mriedem | and then on unshelve on the target host, we set the vm_state to RESIZED rather than ACTIVE: https://review.openstack.org/#/c/603930/9/nova/compute/manager.py@5195 | |
| 18:57:31 | mriedem | so the functional test is like a normal resize where the caller issues the resize and then polls for VERIFY_RESIZE status | |
| 18:58:53 | dansmith | aight, well, whatever.. point being, it seems bad to create more places where we don't have that set.. more places where if we needed to know which kind of migration we're doing we have to look at the list an guess | |
| 18:58:55 | dansmith | for example, | |
| 18:59:18 | dansmith | we have this long standing bug where we don't properly consider live migrations with pinned cpus as needing to match 1:1 for the destination host, | |
| 18:59:23 | dansmith | which is solvable in other ways, | |
| 18:59:43 | dansmith | but one downstream hack tries to find the migration, determine if it's a live one, so it can make better choices | |
| 19:00:09 | dansmith | which is icky for other reasons, but.. finding out what kind of migration is currently going on and what those details are seems like a good thing to me | |
| 19:00:45 | dansmith | so, not a real helpful opinion, but....there it is | |
| 19:01:17 | mriedem | i can do it either way, it's mostly just a question of how gorby we want the RT resize_claim flow to become when we're unshelving during a cross-cell resize....since as noted we have to do some things manually if we're not doing an instance_claim | |
| 19:01:23 | mriedem | *gorpy | |
| 19:01:34 | dansmith | do we not have any places where we might need to look at a late-stage migration and know if it was cross-cell before we allow or disallow something? | |
| 19:02:18 | dansmith | well, fwiw, the migration context setting via instance_claim never made sense to me, and continues to confuse me and others when we try to remember where it gets set (and why not for things like live migration) | |
| 19:02:34 | mriedem | it's not set in instance_claim, it's in resize_claim | |
| 19:02:43 | dansmith | you know what I mean gdi :) | |
| 19:02:46 | dansmith | in the claim process | |
| 19:03:27 | mriedem | i'm not aware of late stage thingies that rely on the migration_context, maybe there are in the actual finish_resize/finish_revert_resize/confirm_resize flows...which i'm not using | |
| 19:03:49 | dansmith | I'm saying things we'd need to handle in this process, not existing ones | |
| 19:03:54 | mriedem | when we revert/confirm the API looks up the migration directly, not via the migration_context: https://review.openstack.org/#/c/603930/9/nova/compute/api.py@3244 | |
| 19:04:17 | dansmith | yeah, which is kinda silly | |
| 19:04:31 | mriedem | which, i guess ^ works because we have the finished status and then it goes to 'completed' or something in the compute | |
| 19:04:33 | dansmith | that would be much nicer as "get the instance and get the current migration" instead of "sort and assume the last one is legit" | |
| 19:04:35 | mriedem | which sound the same to me | |
| 19:05:09 | dansmith | I should just stop talking. No, I don't have any good reasons. | |
| 19:05:15 | mriedem | oh i guess on revert the migration status goes to 'reverted' | |
| 19:05:33 | mriedem | and 'confirmed' on confirm | |
| 19:05:51 | mriedem | alright, well, it's just a drop in the bucket of questions in here | |
| 19:06:00 | mriedem | i just wanted to plant the seed of doubt in someone else's mind about this | |
| 19:06:05 | mriedem | you're welcome | |
| 19:18:53 | sean-k-mooney | so can i ask a dumb question related to that converstation. | |
| 19:19:24 | sean-k-mooney | what is the difference logically between a migration context, a migtion object and migration_data | |
| 19:19:50 | sean-k-mooney | i know all three exists but not sure why there is not jsut one datastructure | |
| 19:20:14 | sean-k-mooney | mriedem: dansmith is the answer to ^ documented anywhere | |
| 19:20:52 | dansmith | migration context is attached to an instance and contains a link to the migration record (i.e. the current one) and some other current details | |
| 19:21:04 | dansmith | migration objects are the in-progress and archival history of an instance's movements | |
| 19:21:33 | dansmith | migrate data is transient virt-specific detailage about the low-level bits that is ferried back and forth but never persisted | |
| 19:22:25 | sean-k-mooney | oh ok that actully kind of makes sence. the nameing is unfortuate but the reason for having 3 distinct entities makes sense | |
| 19:23:15 | mriedem | migrate_data is also only for live migration | |
| 19:23:38 | mriedem | hence the name, LiveMigrateData | |
| 19:24:02 | dansmith | yeah, left that detail out | |
| 19:24:24 | sean-k-mooney | mriedem: yes but for cold migrtion we kindo of abuse the migration_context for associting claim with the active migration too right | |
| 19:25:59 | sean-k-mooney | so we dont actully stuff the resocetrack claims into the migration_context as far as i know but i think we must the uuid of the migration_context when claiming the resouce or somthing like that. | |
| 19:27:28 | mriedem | based on my questions above, i'm clearly not the person to ask about the intracacies of how the migration_context is used during resize | |
| 19:27:39 | mriedem | *intricacies even | |
| 19:27:48 | dansmith | sean-k-mooney: migration context has no uuid, it's attached to the instance | |
| 19:28:17 | mriedem | MigrationContext.migration_id could be used to find the migration object if needed | |
| 19:28:22 | mriedem | within the same cell | |
| 19:28:24 | sean-k-mooney | sorry the migrtion record/object reffrence by the migration_context has a uuid which we use | |
| 19:28:24 | dansmith | sean-k-mooney: it has things that we don't need to persist after completion, unlike things we store in the migration record for posterity, like what flavor it was and what flavor it is now | |
| 19:28:46 | dansmith | mriedem: I meant the context doesn't have its own identifier | |
| 19:28:57 | mriedem | ah yeah | |
| 19:29:25 | dansmith | and, unfortunate that we used the id there I guess, as it makes it potentially less helpful for the cross-cell case | |
| 19:29:26 | mriedem | laura is home, the 2:30 vacuuming has started | |