Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-05-12
10:31:19 stephenfin these are the only changes necessary. I checked. I'll update the commit message shortly
10:31:21 sean-k-mooney stephenfin: you didnt explain why any of the five things needed to be fixed
10:31:29 stephenfin I'll update the commit message shortly
10:31:35 sean-k-mooney and several fo them had #noqa on them
10:31:40 sean-k-mooney so they should have been ignofred
10:31:41 stephenfin they already had noqa
10:31:47 sean-k-mooney yep
10:31:52 stephenfin check my replies
10:31:53 sean-k-mooney so it should not have been checking them
10:33:24 sean-k-mooney stephenfin: https://www.flake8rules.com/rules/E741.html does not feel like we should have it on by default
10:35:52 stephenfin sean-k-mooney: I'd really rather avoid that argument because those tend to be ratholes in nova. I'd much, much rather we just took what flake8 and hacking gave us and dealt with it.
10:36:33 sean-k-mooney arbitary rules like that one make code worse and less readable
10:36:55 stephenfin arbitrary rules like you're *never* allowed exceed 80 characters?
10:37:03 sean-k-mooney i agree it can be consuing a time but its not any worse then using any 1 lettter valiable
10:37:17 stephenfin one man's arbitrary rule is another's good idea
10:37:18 sean-k-mooney stephenfin: yes that has been demonstrated to make code less readable
10:37:25 sean-k-mooney and pep8 enforece 79
10:37:28 sean-k-mooney not 80
10:37:59 stephenfin there is evidence to suggest otherwise https://www.youtube.com/watch?v=wf-BqAjZb8M&t=260
10:38:23 stephenfin https://black.readthedocs.io/en/stable/the_black_code_style.html#line-length would be an informative read
10:38:38 stephenfin but this is exactly where I don't want to end up :D damn it
10:38:53 stephenfin gibi: ta
10:39:23 sean-k-mooney stephenfin: i have read the black style guide it argues against the 80 column limit
10:40:23 sean-k-mooney stephenfin its why it used a 80ish limit rahter then a fixed value.
10:40:37 stephenfin yup
10:41:00 stephenfin to be clear, my argument is against the strict limit
10:41:04 stephenfin 80ish is fine
10:41:13 stephenfin hence the emphasis on *never* above
10:45:48 openstackgerrit Brin Zhang proposed openstack/nova master: Optimize _create_and_bind_arqs logic in conducor https://review.opendev.org/726564
10:47:41 sean-k-mooney stephenfin: i left some more comments. some of the noqas canbe remvoed with light tweeks or we can need to document why.
11:19:07 openstackgerrit Qiu Fossen proposed openstack/nova-specs master: specify mac for creating instance https://review.opendev.org/700429
11:40:48 openstackgerrit Nalini Varshney proposed openstack/nova master: Add migration to make key field type VARBINARY in aggregate_metadata table, https://review.opendev.org/725522
11:45:27 openstackgerrit sean mooney proposed openstack/nova master: [DNM] testing with force_legacy_port_binding workaround https://review.opendev.org/724387
11:45:27 openstackgerrit sean mooney proposed openstack/nova master: add workaround to disable multiple port bindings https://review.opendev.org/724386
11:57:01 openstackgerrit Takashi Natsume proposed openstack/nova master: Remove six.reraise https://review.opendev.org/726898
12:06:07 huaqiang stephenfin: Maybe I should drop my thought of reordering your bp/use-pcpu-and-vpuc-in-one-instance patches. My original thought was the followings patches could be acceleated since my patches donot depend on all the features introduced in the proceeding patches.
12:06:58 huaqiang But the reorder will bring a lot of extra work
12:07:20 huaqiang I'll rebase my pathces by following your patches.
12:10:13 openstackgerrit Takashi Natsume proposed openstack/os-vif master: Remove egg_info in setup.cfg https://review.opendev.org/727173
13:05:13 openstackgerrit Takashi Natsume proposed openstack/nova master: Remove six.reraise https://review.opendev.org/726898
13:46:50 openstackgerrit Lee Yarwood proposed openstack/nova stable/stein: stable-only: skip volume backup tests in cellsv1 job https://review.opendev.org/727147
13:47:57 openstackgerrit Lee Yarwood proposed openstack/nova stable/rocky: stable-only: skip volume backup tests in cellsv1 job https://review.opendev.org/727148
13:48:02 dansmith jsuchome: hey, I +2d that spec and then realized you forgot to address one critical piece of feedback from the last version
13:48:16 dansmith jsuchome: *other* than that, I was going to be happy with it :)
13:48:48 openstackgerrit Lee Yarwood proposed openstack/nova stable/queens: stable-only: skip volume backup tests in cellsv1 job https://review.opendev.org/727150
13:59:06 jsuchome dansmith: I see, I messed up the test part. And the other part I did not address intentionally, I'm not sure it deserves detailed explanation in the spec
13:59:48 jsuchome dansmith: the 'significant changes' in glance.py are really only about executing extra download method under given conditions. And I think this is already covered
14:01:58 dansmith jsuchome: you're changing the base existing glance code more than just calling the rbd function and returning the way the plug point used to work, so I think it's pretty relevant
14:02:13 dansmith jsuchome: there's also no documentation about it in the code change, so I don't even know what the goal of the change is
14:03:05 dansmith jsuchome: it does't need to be detailed, but it should be something, IMHO.
14:04:10 jsuchome I tried to explain it already but now I see I forgot to post my changes ...
14:05:17 jsuchome dansmith: I actually added some comment to that part you are (probably?) mentioning, I assume you mean "Load chunks from the downloaded image file... " bit
14:05:26 jsuchome I've added some comments to the spec
14:05:57 dansmith yeah, so re-reading the code this morning, it looks like we used to not do the image signature verification for things downloaded with the per-scheme handler,
14:06:03 jsuchome (I mean: I've added some comments 1. to the code and 2. now I've also commented the spec)
14:06:08 dansmith and this change is lining it up so we do right?
14:06:55 jsuchome yeah, in previous version, this signature verification was just skipped if there was anything downloaded by the download handler
14:07:20 jsuchome So you think it still should be mentioned in the specs?
14:08:14 jsuchome Maybe I could just mention it in the commit message of the code change
14:08:29 dansmith yeah, so just a line in the spec under proposed change like this is fine: "The glance module also never used to perform image signature verification when the per-scheme module was used. Since we are moving this into core code, we will also fix this so that per-scheme images are verified like all the rest."
14:08:55 dansmith jsuchome: please just add that one line (or something like it) to the spec when you fix the test thing and we can move on
14:09:17 dansmith it should also go into the code change commit message, btw
14:09:44 jsuchome OK, both places then. In a minute
14:10:15 dansmith honestly, if I was doing it, I would break the code into two pieces, one for that fix and one for the rbd module being added
14:10:45 dansmith but, it's already done, so.
14:12:37 dansmith (biab)
14:12:38 jsuchome yeah, it's not exactly part of the feature. Seems like the original author realized it during testing, it appeared in some later PS
14:18:16 openstackgerrit Jiri Suchomel proposed openstack/nova-specs master: Add spec for downloading images via RBD https://review.opendev.org/572805
14:18:17 openstackgerrit Jiri Suchomel proposed openstack/nova master: Add ability to download Glance images into the libvirt image cache via RBD https://review.opendev.org/574301
14:32:06 dansmith jsuchome: +2 on the spec, thanks
14:32:47 jsuchome cool
14:48:22 jsuchome dansmith: two questions: 1. should I add that admin guide change into the same PS as the one with code or rather a new one? 2. where is that ceph CI job I should try to copy& adapt? (I've never done this part before)
14:48:50 dansmith jsuchome: no, do it in a separate patch please
14:49:32 dansmith jsuchome: I'm not super up on the state of the job (i.e. whether it's a legacy or converted job), nor where those bits live depending
14:49:36 dansmith but I bet sean-k-mooney knows
14:50:10 sean-k-mooney which job
14:50:12 sean-k-mooney ceph
14:50:32 sean-k-mooney i think its converted but ill check
14:51:53 openstackgerrit Ghanshyam Mann proposed openstack/python-novaclient master: Bump hacking min version to 3.0.1 https://review.opendev.org/727214
14:52:18 sean-k-mooney devstack-plugin-ceph-tempest-py3 i think is a zullv3 job. we are not defining any legacy playbooks for ti but it comes form devstack not zuul config
14:52:19 dansmith sean-k-mooney: jsuchome needs to take that job, tweak the config on the compute node slightly, and get a one-off run of it at least
14:52:54 sean-k-mooney dansmith: ah ok ill jsut triple check that its zuulv3 if so that is simpel to do
14:54:15 gmann that is zuulv3, derived from tempest-full-py3.
14:54:16 lyarwood https://review.opendev.org/#/c/708038/ is an example of me messing around with the ceph job recently
14:54:26 sean-k-mooney yep its zuul v3 https://github.com/openstack/devstack-plugin-ceph/blob/master/.zuul.yaml#L57-L130
14:55:04 sean-k-mooney jsuchome: what sepcfically do you need to add
14:55:49 dansmith lyarwood: ah nice
14:56:15 dansmith might need a DNM change against devstack to hack the half behavior into place and then control it with something like lyarwood's example
14:56:48 sean-k-mooney dansmith: you should not need too you can create a job that uses it as a parrent and then add your changes
14:56:51 dansmith sean-k-mooney: he needs to set up all the ceph stuff, but configure the compute to *not* use rbd backend, and have a new conf option set to enable direct-from-ceph download
14:57:44 sean-k-mooney so jsuchome just need to override the imagebackend in the nova.conf to be qcow2
14:57:45 dansmith sean-k-mooney: depends on how the ceph bit works in devstack right? in lyarwood's example above, he had https://review.opendev.org/#/c/708035/ for that reason I think
14:58:14 sean-k-mooney e.g. lev devstack and the plugin do its thing but just tell nova not to use it
14:58:21 dansmith sean-k-mooney: probably enough, as long as that sticks and devstack or something downstream doesn't override
14:58:36 sean-k-mooney ceph is set up by https://github.com/openstack/devstack-plugin-ceph
14:58:49 sean-k-mooney but if jsuchome does something like https://review.opendev.org/#/c/724387/3/.zuul.yaml
14:59:30 sean-k-mooney which is using the local.conf [[post-config:/etc/nova/nova.conf]] mechanism to set config options that will run after the plugin
14:59:36 dansmith sean-k-mooney: will that override what the plugin does?
15:00:13 sean-k-mooney ya i belive the order is intree modules then plugins then post-config form local.conf
15:00:17 lyarwood wait, to disable ceph on Nova that plugin has a few variables you can set in the job

Earlier   Later