| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-08-06 | |||
| 12:38:40 | sean-k-mooney | for ovs with hybrid plug we woudl still be hardcoding it to 0 | |
| 12:38:54 | sean-k-mooney | ah | |
| 12:39:13 | sean-k-mooney | well we could but i dont know if those helpful service could break it | |
| 12:39:40 | sean-k-mooney | we could maybe assert its no 0 but im not sure if that would always be the case | |
| 12:41:19 | sean-k-mooney | do you want me to add one for that usecase? as i said im not sure if it would always be correct but i suspect it would be | |
| 12:41:48 | jangutter | sean-k-mooney: I think, whatever choice is made, there's going to be some edge case system that might break it. Both checking != 0 and == 300 could fail so I'd just say pick one and add a NOTE like stephenfin said. | |
| 12:42:52 | sean-k-mooney | ok ill do !=0 and add a note and if we get a bug we can either nuke the test or close it as wont fix | |
| 12:43:25 | sean-k-mooney | or actully maybe i can check sysfs to see if there is a default | |
| 12:43:38 | sean-k-mooney | and skip the test if the default ageing time is 0 | |
| 12:44:26 | jangutter | sean-k-mooney: that might be useful, but I'm almost tempted to say that might point to some bad defaults ppl should investigate. | |
| 12:45:17 | sean-k-mooney | it might but how many people are going to realisticaly run the os-vif functional test beside us | |
| 12:45:53 | jangutter | sean-k-mooney: yep, realistically, the only people interested would be people packaging (i.e. distros). | |
| 12:50:17 | sean-k-mooney | looks like there is no way to set teh bridge againg time globally so i think we are safe with !=0 | |
| 12:50:59 | sean-k-mooney | e.g. via the kernel networkmanager could always change thinks but that is out of scope for this patch | |
| 12:52:13 | stephenfin | Does boot from volume imply we will not have any swap or ephemeral disk also, or does it just apply to the root disk? | |
| 12:52:34 | sean-k-mooney | it just applies to the root disk | |
| 12:52:41 | sean-k-mooney | if the flaovr has swap those should be local | |
| 12:52:45 | stephenfin | Does that mean this is wrong? https://github.com/openstack/nova/blob/master/nova/scheduler/utils.py#L400-L401 | |
| 12:52:50 | sean-k-mooney | same for ephemeral | |
| 12:53:23 | stephenfin | it should read something like | |
| 12:53:35 | stephenfin | if is_bfv: | |
| 12:53:43 | stephenfin | disk = (flavor.swap / 1024) + flavor.ephemeral_gb | |
| 12:53:44 | stephenfin | else: | |
| 12:54:00 | sean-k-mooney | 1 that is only used for nova-manage | |
| 12:54:09 | stephenfin | disk = (flavor.swap / 1024) + flavor.ephemeral_gb + flavor.root_gb | |
| 12:54:32 | sean-k-mooney | 2 its a sligtly differet impleatnion of what we do here https://github.com/openstack/nova/blob/master/nova/scheduler/utils.py#L436-L442 | |
| 12:54:48 | stephenfin | Yeah, and an incorrect one? | |
| 12:55:43 | sean-k-mooney | yep i think so | |
| 12:55:55 | sean-k-mooney | but that is not the only reason that that is slightly broken | |
| 12:56:04 | sean-k-mooney | that being resources_from_flavor | |
| 12:56:20 | stephenfin | yeah, it also shouldn't be setting things to 0. placement doesn't allow that | |
| 12:56:46 | sean-k-mooney | well that is not what i ment but its another bug | |
| 12:58:03 | sean-k-mooney | its ignoring the image which means when you use the nova manage command that executes that function it can ignore the traits on the image | |
| 12:58:30 | sean-k-mooney | actully its not broken | |
| 12:58:32 | sean-k-mooney | its correct | |
| 12:58:45 | sean-k-mooney | rather ((0 if is_bfv else flavor.root_gb) + | |
| 12:58:47 | sean-k-mooney | swap_in_gb + flavor.ephemeral_gb) | |
| 12:58:49 | sean-k-mooney | is correct | |
| 12:58:55 | stephenfin | how? | |
| 12:59:10 | sean-k-mooney | the bracket after root_gb | |
| 12:59:16 | sean-k-mooney | the if only applies to the root gb | |
| 12:59:50 | sean-k-mooney | i guess if you had not swap or ephemeral_gb it woudl still be wrong | |
| 12:59:58 | stephenfin | ah, indeed | |
| 13:00:04 | stephenfin | I missed that. Good spot! | |
| 13:00:28 | stephenfin | Yeah, we should be checking for that but it's not as serious | |
| 13:00:33 | sean-k-mooney | that is handel in the other implemataion here https://github.com/openstack/nova/blob/master/nova/scheduler/utils.py#L456-L459 | |
| 13:01:19 | sean-k-mooney | if you want to fix it just condtionally append https://github.com/openstack/nova/blob/master/nova/scheduler/utils.py#L406 | |
| 13:18:42 | efried | dustinc: left a comment. Better as a @property so you don't have to do icky things like passing arbitrary *args, **kwargs or method names as strings | |
| 13:19:26 | openstackgerrit | Chris Dent proposed openstack/nova master: single pass instance info fetch in host manager https://review.opendev.org/623558 | |
| 13:19:26 | openstackgerrit | Chris Dent proposed openstack/nova master: add InstanceList.get_all_uuids_by_hosts() method https://review.opendev.org/623557 | |
| 13:19:27 | openstackgerrit | Chris Dent proposed openstack/nova master: DNM: Play with cache of host mappings in nova scheduler https://review.opendev.org/674811 | |
| 13:19:27 | openstackgerrit | Chris Dent proposed openstack/nova master: DNM: add profiling to scheduler https://review.opendev.org/674810 | |
| 13:19:48 | cdent | mriedem: then of that ^ stack has some interesting observations about host states | |
| 13:19:56 | cdent | s/then/the end/ | |
| 13:21:17 | sean-k-mooney | cdent: sylvain is on vaction for a few weeks so i or someone else on the team will pick up his placement audit patch | |
| 13:21:25 | sean-k-mooney | cdent: quick question on https://review.opendev.org/#/c/670112/2/nova/cmd/manage.py@2764 | |
| 13:21:39 | sean-k-mooney | cdent: are you asserting that pagination should not be required | |
| 13:21:45 | cdent | hi sean-k-mooney , reading | |
| 13:21:58 | sean-k-mooney | e.g. i can just just remove that not/question | |
| 13:22:19 | cdent | sean-k-mooney: yes, just remove that note, I think. as there's no plan to do pagination (server-side) in placement | |
| 13:22:41 | sean-k-mooney | cool i think the other comment does need to be address | |
| 13:22:47 | cdent | yes | |
| 13:23:11 | sean-k-mooney | i wont get to this today more then likely but ill try and get to it tomrrow or the day after assumeing someone else doesnt pick it up in the mean time | |
| 13:23:19 | mriedem | cdent: where have i seen host mapping caching before.... https://review.opendev.org/#/c/663388/ :) | |
| 13:23:37 | cdent | mriedem: somehow I am not surprised | |
| 13:23:53 | zhouyao | hi sean-k-mooney, could you check this patch? it's been two weeks. https://review.opendev.org/#/c/621646 | |
| 13:24:26 | cdent | mriedem: i'm confused about the track_instance_changes stuff. why be dependent on that? | |
| 13:25:29 | sean-k-mooney | zhouyao: yes i can take a look but you will need to get input form the core team also. ill try and review again today. | |
| 13:26:04 | cdent | mriedem: hrmm, yours is much more...robust | |
| 13:26:12 | cdent | or at least wordy | |
| 13:26:22 | efried | [1] https://review.opendev.org/#/c/605237/4/specs/train/approved/instance-ops.rst | |
| 13:26:22 | efried | sean-k-mooney: I tend to agree re cyborg blocking instance operations. I went to figure out what had been committed to in the spec, and it's a tad wobbly, but the approved spec points to this (open) document in the cyborg repo [1] that seems to declare support for things like pause and shelve. | |
| 13:26:28 | sean-k-mooney | zhouyao: i see you have added mriedem and stephenfin to the review list i would reach out to alex_xu too | |
| 13:27:02 | sean-k-mooney | efried: we had it in the spec for last cycle. or at least discussed it but then we removed it form the spec this cycle | |
| 13:27:03 | mriedem | cdent: "i'm confused about the track_instance_changes stuff. why be dependent on that?" you mean yours or mine? | |
| 13:27:11 | mriedem | mine isn't dependent on jay's series | |
| 13:27:43 | sean-k-mooney | efried: a yes we moved it to cyborg this cycle intead of in the nova spec | |
| 13:28:29 | efried | it should really have been in this spec, but people kept complaining it was too long. | |
| 13:29:01 | sean-k-mooney | efried: ya. that said we did disucss this at some lent both in person at the ptg and last cycle | |
| 13:29:14 | efried | sean-k-mooney: Do you remember what was agreed upon? | |
| 13:29:36 | sean-k-mooney | i though it was everything bar live migration | |
| 13:30:12 | sean-k-mooney | but basically https://review.opendev.org/#/c/605237/4/specs/train/approved/instance-ops.rst i think is more or less what i was expecting | |
| 13:30:19 | cdent | mriedem: I mean in yours. Why even mention that config setting? The idea is that if you got to HostMapping, use the cache. it seems that at least some of the time we hit "# Host is running old version, or updates aren't flowing." | |
| 13:35:24 | efried | sean-k-mooney: I maybe remember Sundar talking about this being an interim step in the overall strategy, where subsequent patches will implement support and remove the blockers one by one. | |
| 13:35:58 | sean-k-mooney | efried: perhaps but normally we just dont expost the feature to the user until its done | |
| 13:36:18 | sean-k-mooney | which in this case would mean putting the patch that reads the flavor extra spec at the end | |
| 13:36:25 | mriedem | "updates aren't flowing" == track_instance_changes=False | |
| 13:36:34 | mriedem | when HostState.updated could be Falsey I'd have to check | |
| 13:36:47 | mriedem | i don't mind removing the mention of the config option in my commit message though | |
| 13:37:04 | efried | sean-k-mooney: In any case, I put a hold on the bottom patch, which is something we do anyway on a series like this until it's closer to done. | |
| 13:37:35 | sean-k-mooney | i saw that there has been some work started on a fake driver and some tempset tests | |
| 13:38:18 | efried | yes. I understand they've been having a tough time with... follow through from... previously committed resources... | |
| 13:38:26 | sean-k-mooney | i only have a 3 day week this week but im hoping to be able to try out some of this in the next week or two | |
| 13:38:32 | mriedem | hmm, i think set_update_time_on_success isn't using what it once did (RequestSpec vs HostState) | |
| 13:39:07 | mriedem | oh nvm | |
| 13:39:42 | sean-k-mooney | efried: well it does look like at least in the last week or so that progess has been picking up a bit | |
| 13:40:36 | sean-k-mooney | i need to try and review more of this work but i also need to review gibi port migrtion stuff so time is a factor. | |
| 13:41:15 | mriedem | cdent: i think the "older computes" comment is from when edleafe wrote this https://review.opendev.org/#/c/160511/ back in kilo :) | |
| 13:49:03 | mriedem | cdent: but yeah i suppose we have to hit that method to get the host mapping at least once per host to determine which cell the host is in so we know where to pull the instances from (which cell db), so in your test with 25 fake computes the profiler shows that method was called 25 times | |
| 13:49:04 | cdent | mriedem: the patch prior to my last one there shows how to do profiling in an eventlet situation, in case you want to try it elsewhere | |