Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-03-05
15:27:13 stephenfin the updated_at of the aggregate itself
15:27:41 dansmith so us doing that here just worries me a little that if that changed or there was some other detail, we could have situations where updated is before created, etc
15:27:44 dansmith which is just messy
15:28:32 stephenfin It's using 'timeutils.utcnow()'
15:28:49 stephenfin I'm not sure how you'd approach this if you'd want it wholly contained in oslo.db though
15:29:30 stephenfin We want to say if a "child" is updated, set the 'updated_at' for the child and also the "parent" linked by the fk
15:30:21 dansmith stephenfin: make TimestampMixin have a .touch() method
15:30:59 dansmith brinzhang: I -1'd for a couple of things.. I'm really not sure why this is important, but we should be consistent if we're going to do this, IMHO
15:31:28 dansmith brinzhang: it seems very weird to me to get an aggregate updated when I add a host, but not when I set some property on that aggregate, which from the user's perspective is a direct property of the aggregate itself
15:31:43 stephenfin (or more specifically, when updating an instance of AggregateHost, also update the Aggregate referenced via the 'aggregate_id' field)
15:32:02 openstackgerrit John Garbutt proposed openstack/nova master: Fix os-console-output policy to be admin_or_owner https://review.opendev.org/706725
15:32:05 dansmith stephenfin: yep, could do it that way too
15:32:05 stephenfin Hmm, I can't reason how that would work
15:32:26 stephenfin the touch() thing
15:32:42 dansmith agg = query(); agg.touch(); agg.save() ?
15:32:43 stephenfin oh, you mean manually call that whenever we update an AggregateHost instance?
15:32:46 dansmith yeah
15:32:57 dansmith move what we're doing into the mixin so the time stuff is all in the same place
15:32:58 stephenfin gotcha
15:33:26 stephenfin that could work but you've to remember to do it manually for everything that has a "parent" relationship
15:34:04 dansmith stephenfin: that's what this patch is doing right?
15:34:35 stephenfin true
15:34:48 stephenfin I was going to ask what it gave us over what we're doing, but you already said that
15:34:55 stephenfin (the time stuff is all in the same place)
15:35:00 dansmith right
15:35:30 dansmith just because times, timezones, resolution etc is an often screwed up thing, and could even be db backend specific
15:35:48 stephenfin I assume iterating through all foreign key columns on a model, seeing if they have an updated_at row, and updating it if so, would be too clever?
15:36:00 stephenfin s/row/column/
15:36:09 dansmith stephenfin: not FKs but relationships on the db model you mean
15:36:22 dansmith but no, that'd be fine too, it's just more work and testing I think
15:36:57 stephenfin ah, yeah, relationships
15:37:40 luyao hi ,everyone, I have a question about the do_cleanup flag in live migration
15:38:11 luyao the 'do_cleanup' seems just a flag to mark if the instance path need be cleaned, https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L8185
15:38:44 luyao but it control more things , e.g. https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L8354, if do_cleanup is False, it will not invoke driver.cleanup which also do _unflug_vifs and _undefine_domain
15:38:55 luyao is this BUG?
16:55:14 melwitt luyao: as far as I know, that behavior is not a bug
16:58:04 luyao melwitt: thanks for your reply, so why we don’t need to undefine the domain or unplug the bugs if do_cleanup is false
16:58:35 luyao s/bugs/vifs
17:03:12 melwitt luyao: hmm ... maybe we should ask artom, I see his comments in that code block
17:03:56 artom melwitt, after the call, I'd have to reload context :)
17:07:15 luyao artom: hah, thanks in advance :)
17:07:51 artom So it's set by https://github.com/openstack/nova/blob/19cbbbebddadb125b8123ce0350b7146b29a59c6/nova/compute/manager.py#L8160
17:08:09 melwitt the do_cleanup is about whether the instance is on shared storage, if it is on shared storage we don't need to destroy the domain or change the vifs right? because the instance doesn't really have to "move"
17:12:13 artom I thought shared storage was just the disk
17:12:24 artom Everything else lives on the compute host directly
17:12:27 artom No?
17:12:37 melwitt lyarwood halp
17:13:10 melwitt I guess I don't know anymore. I feel like I used to, but maybe that was a dream
17:14:16 kashyap artom: Yes, shared storage means: just the disk, in general ...
17:14:39 kashyap (Only memory and device state will be live-migrated in shared storage setup.)
17:15:17 kashyap On changing the VIFs, I'm not 100% sure, I also feel like melwitt :D
17:15:39 artom luyao, maybe you could tell us what behaviour you're seeing that you think is a bug
17:16:04 artom melwitt, my comment is only there because I added the instance.refresh() call
17:16:09 artom Everything else predates me
17:16:10 melwitt kashyap: heh
17:16:27 melwitt nicely played
17:16:42 kashyap `git shortlog --nse /path/to/file/name` is your weapon, luyao
17:16:54 kashyap (And pick the top three or four members to harass)
17:17:11 luyao artom: I think do_cleanup is just a flag to tell if we need cleanup instance file during migration right?
17:17:37 kashyap (Or even a fine-grained one: `git blame -C -M -L line-$x:line-$y /path/to/file/name/`)
17:18:18 artom luyao, the way it's set, yeah
17:18:27 artom luyao, but it could be used for other things, but still be correct
17:18:49 artom The code is kinda messy, but if it works, it works
17:18:54 luyao but in the code path, if do_cleanup is false, the whole driver.cleanup will not be invoked, but except for instance files cleanup ,we also have other things like undefine domain and unplug vifs
17:19:18 artom luyao, which is why I'm asking if there's a specific behaviour/error you're asking about
17:19:41 artom Because debating the code itself is kinda pointless. There are years and years of multiple people doing multiple things, all for valid reasons
17:19:55 artom To end up with the mess with have now :)
17:20:18 luyao artom: no, I write my own patch ,and alex_xu comment at this, we are both not sure of this
17:20:38 artom luyao, link the patch please :)
17:20:49 luyao https://review.opendev.org/#/c/687856/10/nova/compute/manager.py@8623
17:20:59 artom Tbh, I won't able to look now because I'm running on very little sleep, but I'll try to look later
17:21:36 openstackgerrit Ghanshyam Mann proposed openstack/nova master: Introduce scope_types in os-admin-password https://review.opendev.org/701630
17:22:37 luyao artom: OK, thanks. It's too late for me so I'm going to sleep too. :D
17:22:48 artom luyao, ack :)
17:24:09 lyarwood melwitt: sorry was afk, reading
17:24:37 openstackgerrit Ghanshyam Mann proposed openstack/nova master: Add new default roles in os-admin-password policies https://review.opendev.org/701639
17:25:09 tosky checking a pending patch on stable/pike - it seems that nova-live-migration has still some issues there, and other patches are blocked too
17:26:07 tosky I think we already discussed about this, are there any updates?
17:28:25 melwitt tosky: we did, no updates yet sorry. sean-k-mooney had been trying to get time to do a bit of debugging but afaik they didn't get to it yet
17:29:03 tosky oki, np
17:29:39 lyarwood luyao: we unplug VIFS regardless of that value in post_live_migration_at_source https://github.com/openstack/nova/blob/19cbbbebddadb125b8123ce0350b7146b29a59c6/nova/virt/libvirt/driver.py#L9260-L9267
17:29:52 tosky it's not the most important thing (in the long run, I just want to remove a legacy job from openstack-zuul-jobs :)
17:30:56 lyarwood luyao: iirc Libvirt itself handles the removal of the domain
17:31:56 lyarwood luyao: https://github.com/openstack/nova/blob/19cbbbebddadb125b8123ce0350b7146b29a59c6/nova/virt/libvirt/driver.py#L909-L911 yeah it does
17:32:28 melwitt tosky: yeah, it's odd that it fails so much more on pike than on master. that's part of why digging in will be hard, find what's happening and what's different. I see occasional failures on master but obvious pike is failing nearly all the time last I saw
17:32:43 melwitt *obviously
17:38:09 melwitt lyarwood++
17:40:34 openstackgerrit Merged openstack/nova master: Stop using PlacementDirect https://review.opendev.org/640887
17:44:28 luyao lyarwood: before rollback live migration at destination ，it also check the do_cleanup flag，in this case，i think we need destroy the domain at target host manually right?
17:45:48 lyarwood luyao: just looking at the review now, I didn't get that we were talking about rollback sorry
17:46:26 lyarwood luyao: it depends on the failure tbh, we ask Libvirt to persist the domain but only on a successful migration
17:47:02 lyarwood https://github.com/openstack/nova/blob/19cbbbebddadb125b8123ce0350b7146b29a59c6/nova/virt/libvirt/driver.py#L913-L915
17:47:05 lyarwood https://github.com/openstack/nova/blob/19cbbbebddadb125b8123ce0350b7146b29a59c6/nova/virt/libvirt/guest.py#L609-L611
17:50:51 luyao lyarwood: I understand it may not trigger error in real env, but do_cleanup flag setting and the code path it controls is a little confusing
17:52:18 luyao since do_cleanup is just tell if the instance path file is shared
17:52:31 lyarwood luyao: yeah very, I've added a comment, I think we could look into calling rollback_live_migration_at_destination regardless of that value in a seperate change
17:52:54 lyarwood luyao: I don't know if that's going to break assumptions in other drivers that support LM
17:58:21 luyao lyarwood: thanks for your comments I’ll go into it further tomorrow :)
18:22:53 openstackgerrit Jason Anderson proposed openstack/nova master: Use fair locks in resource tracker https://review.opendev.org/711528
19:58:13 openstackgerrit Victor Coutellier proposed openstack/nova master: Non-Admin user can filter their instances by more filters https://review.opendev.org/701609

Earlier   Later