| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-03-05 | |||
| 15:09:37 | efried | yeah, if we do anything at all, that would be my vote | |
| 15:09:48 | brinzhang | dansmith: yeah, agree | |
| 15:09:49 | efried | tframbo: let me clarify: We now copy flavor information into the instance record, so that any changes to the original flavor (including deleting it) don't get confused with how the instance was originally created. | |
| 15:10:02 | efried | tframbo: which means there's really no need to "disable" a flavor -- just delete it instead. | |
| 15:10:18 | efried | dansmith: since what release do we do the flavor-into-instance copy? | |
| 15:10:38 | efried | certainly before rocky. | |
| 15:10:39 | dansmith | technially, not sure, a long time.. not sure when we *expose* it | |
| 15:10:46 | dansmith | but it was sdague era, IIRC, so it's been a while | |
| 15:11:32 | dansmith | 2.47 | |
| 15:11:32 | efried | microversion 2.47... | |
| 15:11:48 | efried | ...pike. | |
| 15:12:23 | dansmith | 2017 | |
| 15:12:29 | brinzhang | https://docs.openstack.org/nova/latest/reference/api-microversion-history.html#id43 | |
| 15:12:33 | tframbo | yeah ,I just want to know the "disable" value , if is meaningful.if is unuseful, we might should delete it. | |
| 15:16:25 | efried | tframbo: agree. Though that would require a microversion, so it may not be worth the effort. Perhaps the next time gmann does an "API cleanup" sweep... | |
| 15:16:54 | efried | tframbo: you could propose to filter it out of the CLI output I suppose. Though again, not sure that would be worth the effort. | |
| 15:17:45 | efried | Okay, I'm going to go take actual vacation for a couple days, and then I'm really gone. o/ | |
| 15:17:47 | brinzhang | yes, we should consider is it worth to do | |
| 15:18:25 | brinzhang | too later for me, good morning everyone ^^ | |
| 15:18:28 | brinzhang | bye | |
| 15:18:48 | tframbo | thank you ,all | |
| 15:18:54 | tframbo | bye | |
| 15:20:13 | brinzhang | later.. | |
| 15:20:33 | brinzhang | dansmith: Can you review these bug fix? https://review.opendev.org/#/q/status:open+project:openstack/nova+branch:master+topic:bug/1663456 | |
| 15:21:14 | brinzhang | dansmith: stephenfin want to get your check ^^ | |
| 15:21:56 | dansmith | brinzhang: I'm not sure why stephenfin +2d that but also said it should be done in oslo.db | |
| 15:23:06 | dansmith | I dunno if TimestampMixin does the time the same way, but if it didn't or it changed, it would be confusing that we're setting it to one thing and then potentially have that roll backwards, forwards, or be different from the create time | |
| 15:23:19 | dansmith | why not fix this at the oslo.db layer and not merge this? | |
| 15:23:22 | brinzhang | dansmith: I tend to use the current implementation, and I think it's good enough. | |
| 15:24:34 | dansmith | I'm not sure what that means | |
| 15:26:11 | stephenfin | dansmith: I don't know what that means either but the oslo.db part wouldn't fix this entirely | |
| 15:26:43 | dansmith | I see now it's add/delete host that they want it updated at | |
| 15:26:54 | dansmith | the thing I don't like here, | |
| 15:26:55 | stephenfin | brinzhang is saying that when we add a new host to an aggregate, we should update the updated_at updated | |
| 15:26:56 | stephenfin | yeah | |
| 15:27:12 | dansmith | is that I don't know that oslo.db is using time.utcnow() (I expect it is, but..) | |
| 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 | |