Posted	Nick	Remark
#openstack-nova - 2020-03-05
15:08:45	brinzhang	If we want to do this feature, I think I can continue this, but we should consider, whether pay time to do ^^
15:09:15	dansmith	brinzhang: we should only do it if there is a compelling reason to, not just because "it's half finished already"
15:09:22	dansmith	I'd rather see the field go away personally
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

Earlier   Later