Posted	Nick	Remark
#openstack-nova - 2019-05-17
16:07:54	aspiers	in response to reviews from efried and stephenfin
16:08:19	aspiers	efried: and one of the comments is "please can we discuss in IRC" ;-)
16:08:30	sean-k-mooney	cool ill finish updating the downstream but then ill look at it then
16:08:42	sean-k-mooney	*bug
16:09:05	efried	aspiers: I'm here, discuss away.
16:09:21	aspiers	efried: I don't want to repeat the long essay I've drafted in Gerrit ;)
16:09:30	aspiers	efried: give me a few mins to finish the response
16:09:49	efried	ack
16:12:01	mriedem	gibi_off: i'm guessing we're not going to be able to backport the fix for https://review.opendev.org/#/c/512623/ without dropping the functional test as well because of all the underlying refactor to make the functional test work?
16:29:29	stephenfin	mriedem: You think a patch to exclude the 'user_data' field for each instance logged in this log sounds reasonable? https://opendev.org/openstack/nova/src/commit/ed5b7c7fc311c2af8c8be09044bb2723141f6f0d/nova/scheduler/host_manager.py#L180
16:30:12	sean-k-mooney	stephenfin: we woudl have to modify the __repr__ function in the ovo to not incude that right
16:30:13	stephenfin	I'm looking at a log where the user has bulk created some instances and it's just pages and pages of base64 (?) data
16:30:32	stephenfin	I guess? I know we do some filtering of objects on security grounds
16:30:41	stephenfin	I was hoping to reuse some of that, if that's possible
16:31:11	sean-k-mooney	ah then ya it could be simple so
16:32:31	sean-k-mooney	well looks like you have the instace info as a dict so you would have to make a copy and just remove the user_data section from teh copy or replace it with a message saying it was remvoed
16:33:01	stephenfin	Or that <?> marker that gets inserted by...something
16:33:08	stephenfin	o.vo normally, I guess
16:33:10	sean-k-mooney	yes that thing
16:33:27	sean-k-mooney	e.g. so you can tell there was user data but not log it
16:35:23	mriedem	stephenfin: you're looking at an old log then,
16:35:29	mriedem	because that was fixed to only log instance uuids
16:35:56	mriedem	https://opendev.org/openstack/nova/commit/4fd7c93726eff4cc0b010741ea1772cf19c314fc
16:36:05	mriedem	https://review.opendev.org/#/q/I0eda1c58a7eb54121230c880818b4b1d0fdf4893
16:36:14	mriedem	take that back to pike and ocata if you want
16:36:32	stephenfin	I'm going to do just that. Thanks, mriedem
16:36:45	mriedem	np
16:36:59	openstackgerrit	Stephen Finucane proposed openstack/nova stable/pike: Do not dump all instances in the scheduler https://review.opendev.org/659832
16:37:07	sean-k-mooney	oh the list grabs the keys that are the uuids neat
16:37:20	openstackgerrit	Stephen Finucane proposed openstack/nova stable/ocata: Do not dump all instances in the scheduler https://review.opendev.org/659833
16:37:37	mriedem	it's a dict, the uuids are the keys
16:37:48	sean-k-mooney	yep
16:38:27	sean-k-mooney	so going from inst_dict to list(inst_dict) when form full dict content to just keys whic is cool
16:39:01	sean-k-mooney	or we could have dont inst_dict.keys() to be explicit but same effect
16:39:52	stephenfin	sean-k-mooney: Python be cool like that
16:40:03	sean-k-mooney	yes and no
16:40:21	stephenfin	list(dict) has the advantage that you'll always get a list back
16:40:31	sean-k-mooney	i know why python iterate over the keys not the items but it always trips me on things like that
16:40:33	stephenfin	whereas with dict.keys(), you'll get an iterator on Python3
16:40:48	sean-k-mooney	also true
16:40:52	openstackgerrit	Adam Spiers proposed openstack/nova master: Add infrastructure for invoking libvirt's getDomainCapabilities API https://review.opendev.org/655268
16:40:53	stephenfin	and I don't know how oslo.log handles that
16:41:21	sean-k-mooney	i just would have expected list(dict) to be the same as list(dict.itmes()) but i also know why that is not the case
16:41:34	aspiers	efried, sean-k-mooney: new patchset 4 and response to patchset 3 ^^^
16:42:02	efried	The tox failure is weird, it's claiming it can't find deps, but they're being set in [testenv]
16:42:02	efried	stephenfin: would you mind taking a quick look at this? https://review.opendev.org/#/c/659682/
16:42:22	stephenfin	looking
16:43:03	efried	stephenfin: This is a throwaway patch, so if it's fixable by adding explicit deps lines back in, that's fine.
16:43:30	stephenfin	efried: They're being set in 'testenv', not 'testenv:functional'
16:43:43	efried	no inheritance there?
16:43:51	efried	okay
16:43:52	stephenfin	Nope. tox isn't that clever
16:44:01	stephenfin	Thankfully, tbh. It's already too clever
16:44:09	efried	so just copy them into those envs
16:44:58	efried	also would have thought having used envdir would be enough
16:45:05	efried	but apparently something is *requiring* deps?
16:45:45	openstackgerrit	Eric Fried proposed openstack/nova stable/stein: Skip _exclude_nested_providers() if not nested https://review.opendev.org/659206
16:45:45	openstackgerrit	Eric Fried proposed openstack/nova stable/stein: DNM: Revert "Use external placement in functional tests" https://review.opendev.org/659682
16:45:55	efried	thanks for the look stephenfin
16:58:47	efried	aspiers: thanks for the updates. Convince me that LibvirtConfigDomainCapsFeature needs to exist *ever*.
16:59:13	aspiers	efried: just saw your comment on the follow-up change about that
16:59:27	aspiers	efried: think I see where you're coming from now, maybe
16:59:42	aspiers	so your point is that a base class isn't needed here
16:59:58	aspiers	even though we expect there to be other feature classes in the future
17:00:16	aspiers	or at least, have no reason to believe that there definitely won't be
17:00:20	aspiers	right?
17:00:27	efried	Right, because the only thing you're doing with that base class is converting a positional 'name' arg into the 'root_name' kwarg.
17:00:40	aspiers	Yeah, that's a fair comment
17:00:41	efried	which the subclasses can do just as easily.
17:01:34	aspiers	but couldn't the same argument apply to LibvirtConfigGuestDeviceAddress?
17:01:50	aspiers	I was trying to follow the precedent which already seemed to exist in config.py
17:03:20	aspiers	Anyway I'm not religious about the need for that base class
17:03:22	efried	Looking... but I tend to be in the camp of "follow precedent thoughtfully"
17:03:37	aspiers	We could always reintroduce a base class later if need be
17:03:43	aspiers	I'm in the same camp for sure :)
17:03:59	efried	no, **DeviceAddress does some actual things in its overrides.
17:04:43	aspiers	I don't see any method in the DeviceAddress base class which adds any value
17:04:56	aspiers	the subclass overrides do add value, naturally
17:05:24	aspiers	but actually there's a much better example which I remember following
17:05:31	aspiers	LibvirtConfigGuestFeature
17:05:40	efried	https://review.opendev.org/#/c/633855/13/nova/virt/libvirt/config.py@1351 ?
17:05:42	aspiers	I was just copying that approach
17:06:21	efried	Yes, LibvirtConfigGuestFeature is similarly worthless and could be removed imo.
17:06:25	efried	(separately)
17:06:51	aspiers	OK, so LibvirtConfigGuestDeviceAddress.__init__() does *one* thing of use: setting self.type
17:07:01	aspiers	so not entirely redundant
17:07:03	efried	yeah, but the other methods do real things.
17:07:13	aspiers	they are overridden by both subclasses though
17:07:23	aspiers	oh, super()
17:07:25	aspiers	OK
17:07:36	aspiers	missed that
17:07:53	efried	**kwargs)
17:07:53	efried	super(LibvirtConfigGuestFeatureACPI, self).__init__(root_name="acpi",
17:07:53	efried	def __init__(self, **kwargs):
17:07:53	efried	class LibvirtConfigGuestFeatureACPI(LibvirtConfigObject):
17:07:53	efried	but for LibvirtConfigGuestFeature, there's no reason e.g. the next one couldn't just be
17:08:11	efried	similarly for the subsequent two.
17:08:12	openstackgerrit	Doug Wiegley proposed openstack/nova master: Improve metadata server performance with large security groups https://review.opendev.org/656084
17:08:14	aspiers	Now that this conversation jogged my memory, I remember I was definitely emulating LibvirtConfigGuestFeature
17:08:20	efried	etc etc

Earlier   Later