Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-03-05
15:07:22 brinzhang Now in our create flavor, the request body does not contain this proerty
15:07:37 dansmith efried: not that I've ever heard of
15:07:42 dansmith not really sure what the point of it would be
15:07:46 efried seems like, since we now copy the flavor body into the instance, there's an easy workaround for this: delete the flavor
15:07:48 dansmith it came from a time before we kept flavors with instances
15:07:54 efried jynx
15:08:13 dansmith right, the only case I can imagine is wanting to keep flavors for records of what things were booted from, but it seems confusing to me for the user,
15:08:22 dansmith unless disabled flavors are filtered most of the time
15:08:31 dansmith but it's so much easier to just see it in the instance detail
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 efried microversion 2.47...
15:11:32 dansmith 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 stephenfin Hmm, I can't reason how that would work
15:32:05 dansmith stephenfin: yep, could do it that way too
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

Earlier   Later