Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-08-06
12:38:38 jangutter sean-k-mooney: yep, or something helpful like NetworkManager changes it for you, etc. Was just wondering if it wouldn't make sense to have the functional test check for 300.
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: add InstanceList.get_all_uuids_by_hosts() method https://review.opendev.org/623557
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:27 openstackgerrit Chris Dent proposed openstack/nova master: DNM: add profiling to scheduler https://review.opendev.org/674810
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: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 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:22 efried [1] https://review.opendev.org/#/c/605237/4/specs/train/approved/instance-ops.rst
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

Earlier   Later