| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-05-10 | |||
| 16:38:42 | openstackgerrit | Dan Smith proposed openstack/nova master: Address feedback from instance_list smart-cell behavior https://review.openstack.org/567643 | |
| 16:38:45 | dansmith | mriedem: ^ | |
| 16:39:00 | efried | mriedem: ack | |
| 16:43:42 | efried | mriedem: It doesn't look like you actually fixed https://review.openstack.org/#/c/557835/8/nova/tests/unit/network/test_utils.py | |
| 16:43:54 | efried | mriedem: Unless I'm missing something obvious, you added the import but didn't change the offending references. | |
| 16:44:10 | efried | So now you'll fail pep8 *and* py27/35 :) | |
| 16:44:23 | mriedem | you're blind | |
| 16:44:26 | mriedem | everything passed locally | |
| 16:44:39 | efried | mriedem: sorry, I'm blind, ignore me. | |
| 16:46:30 | mriedem | efried: no joke https://review.openstack.org/#/c/558487/ | |
| 16:46:34 | mriedem | sahid would be PISSED! | |
| 16:47:23 | mriedem | apparently that doesn't have any tests? | |
| 16:47:53 | mriedem | https://github.com/openstack/nova/commit/27614dc595d0fac2e57300c85bb34da266849a46 | |
| 16:48:23 | efried | mriedem: I clearly looked into it https://review.openstack.org/#/c/558487/2/nova/network/linux_net.py@a1813 | |
| 16:48:39 | efried | mriedem: Why doesn't my comment show up with yours? Seems like a gerrit ui bug. | |
| 16:48:42 | sean-k-mooney | mriedem: we should proably port that to os-vif at some point rather then doing it in the libvirt vif.py | |
| 16:48:55 | mriedem | sean-k-mooney: yes, both jaypipes and i asked for that in the original patch | |
| 16:48:56 | efried | anyway, IIRC what I discovered was that that method had been removed somewhere else. | |
| 16:49:44 | mriedem | so in his first change, he means to move create_tap_dev, but also moved the trusted vf tests, and then in the 2nd patch deletes the trusted_vf code but not the tests.... | |
| 16:50:02 | sean-k-mooney | mriedem: adding a new pluging wont be two hard ill open a bug for it to track it. we also need to move a few others | |
| 16:50:03 | efried | and yeah, I see that method in master. | |
| 16:50:49 | mriedem | which is also what was causing the pep8/unit test failures | |
| 16:50:51 | mriedem | so i'll fix this up | |
| 16:59:41 | openstackgerrit | Dan Smith proposed openstack/nova master: Address feedback from instance_list smart-cell behavior https://review.openstack.org/567643 | |
| 17:08:32 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Move create_tap_dev to the new utility module. https://review.openstack.org/557835 | |
| 17:08:33 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Move set_vf_interface_vlan to the new utility module. https://review.openstack.org/558487 | |
| 17:09:59 | mriedem | efried: ok those 2 should be golden now | |
| 17:10:08 | efried | mriedem: ack | |
| 17:21:21 | efried | mriedem: +2 on the first one, but didn't +W it - question inline. Can you confirm that that test case was added as part of this patch? | |
| 17:21:35 | efried | (I don't see it in master or in the base) | |
| 17:22:32 | cfriesen_ | mriedem: regarding the DB thing. Doesn't ring a bell, but I'm pretty sure we take down the DB, then switch the VIP, then bring up the DB again on the new side. | |
| 17:24:48 | cfriesen_ | cfriesen: we use shared storage rather than a master/slave postgres setup | |
| 17:25:08 | cfriesen_ | mriedem: ^ (apparently I like talking to myself) | |
| 17:32:48 | cfriesen | is there a reason why we default to "novalocal" as the dhcp_domain even when using neutron? It means the hostname retrieved via metadata has ".novalocal" appended to it by default. | |
| 17:36:44 | openstackgerrit | Eric Fried proposed openstack/nova master: Add tests for alloc_cands with member_of https://review.openstack.org/561399 | |
| 17:40:34 | mriedem | cfriesen: ack | |
| 17:41:10 | mriedem | cfriesen: https://www.openstack.org/summit/vancouver-2018/summit-schedule/events/21754/novaneutron-ops-cross-project-session | |
| 17:49:40 | mriedem | efried: yes it's new and it also looks wrong | |
| 17:50:07 | mriedem | because ex.assert_has_calls(calls) looks like the wrong assertion, since execute should have been called twice | |
| 17:50:18 | mriedem | so that should have probably been ex.return_value.assert_has_calls | |
| 17:52:14 | mriedem | oh nvm, that's on set_device_mtu which is unrelated | |
| 17:52:20 | efried | mriedem: ... yeah. | |
| 17:52:28 | efried | so like, where tf did this test case come from? | |
| 17:52:37 | efried | It's not wrong, it's just... random. | |
| 17:53:24 | mriedem | probably rebase damage from I9872056c440a706b89dd51d3b9f2418951723efa | |
| 17:53:49 | mriedem | yeah it was dropped here https://review.openstack.org/#/c/557543/1/nova/tests/unit/network/test_linux_net.py | |
| 17:54:17 | mriedem | i'll update | |
| 17:54:31 | efried | mriedem: Yup, I see it. | |
| 17:54:42 | efried | mriedem: No need to update, what are you going to do, split it into a separate patch? | |
| 17:54:48 | mriedem | yeah | |
| 17:55:14 | efried | mriedem: Okay. FWIW I'd be fine just commenting in the review that it was accidentally dropped in a bad rebase, add that link ^, and be done. | |
| 17:55:22 | efried | but if you prefer splitting, soit. | |
| 17:57:39 | openstackgerrit | Alvaro Lopez Garcia proposed openstack/nova master: Ensure that periodic reclaim cleans DB deleted instances https://review.openstack.org/323250 | |
| 17:57:47 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Move create_tap_dev to the new utility module. https://review.openstack.org/557835 | |
| 17:57:48 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Move set_vf_interface_vlan to the new utility module. https://review.openstack.org/558487 | |
| 17:57:49 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Add test_set_device_mtu_default back in https://review.openstack.org/567659 | |
| 17:58:50 | efried | mriedem: +2 all the way up. Don't know if you still feel good about approving the bottom one... | |
| 17:58:56 | mriedem | i feel great | |
| 17:59:23 | mriedem | you could also just fast approve https://review.openstack.org/#/c/567659/ | |
| 17:59:27 | mriedem | it's just adding a unit test back in | |
| 18:00:09 | efried | mriedem: done. | |
| 18:13:35 | clutchbeyers | This is a post-newton question: Does anyone know if we set availability_zone in spec_obj on vm provision if the user does not select an availability_zone? | |
| 18:14:23 | clutchbeyers | set in the db after launch* | |
| 18:28:45 | mriedem | clutchbeyers: i know that starting in pike we set the instance.availability_zone based on the compute host that was selected by the scheduler https://review.openstack.org/#/c/446053/ - not sure if that extends to the request spec | |
| 18:28:54 | mriedem | the request spec is really about what the user requested | |
| 18:29:08 | mriedem | so i don't think setting reqspec.az if the user didn't request one would be correct there | |
| 18:31:15 | melwitt | before that change, instance.availability_zone used to get set to the "default availability zone" if one wasn't requested at boot time | |
| 18:36:43 | openstack | Launchpad bug 1768876 in OpenStack Compute (nova) queens "metadata-api fails to get availability zone for instances created before pike" [High,Triaged] | |
| 18:36:43 | mriedem | melwitt: i know, see https://bugs.launchpad.net/nova/+bug/1768876 | |
| 18:36:46 | efried | mriedem, melwitt: Do y'all have a problem if I fast-approve takashin's mox removal patches? They're test-only; if they pass zuul they clearly work; and I'd like to save other reviewers the pain of combing over these (they're pretty tedious). | |
| 18:37:35 | melwitt | efried: passing zuul is the easy part -- reviewing to ensure test coverage is not lost isn't. so I've not thought of them as easy, one person reviews | |
| 18:37:47 | melwitt | but that's just MHO | |
| 18:38:07 | efried | melwitt: roger that. YHO is what I was asking for. Will leave them for other cores to scrutinize. | |
| 18:39:00 | melwitt | mriedem: guh, another regression | |
| 18:40:18 | openstackgerrit | Merged openstack/nova master: Add CellMapping.get_by_project_id() query method https://review.openstack.org/509002 | |
| 18:47:02 | openstackgerrit | Dan Smith proposed openstack/nova master: Change the field type of InstanceMapping.instance_uuid https://review.openstack.org/567669 | |
| 18:51:42 | arvindn05 | mriedem: good call on checking for scheduling volume backed instances | |
| 18:52:06 | clutchbeyers | thanks for that explanation. we have a use-case where whatever az the scheduler chooses, we do not want a resize to migrate across az's. Sounds like the team has made the decision to keep that logic, and I support that. We can fork our own logic for that | |
| 18:52:33 | arvindn05 | for volume back instances the volume meta data is provided as image meta data. see https://github.com/openstack/nova/blob/master/nova/compute/api.py#L1060 | |
| 18:53:08 | openstack | Launchpad bug 1768876 in OpenStack Compute (nova) queens "metadata-api fails to get availability zone for instances created before pike" [High,Triaged] | |
| 18:53:08 | mriedem | clutchbeyers: if you look at my comments in https://bugs.launchpad.net/nova/+bug/1768876 - starting in pike you can't migrate across az's | |
| 18:53:15 | mriedem | which is actually a regression | |
| 18:53:16 | jmccarthy | mriedem: You about ? Trying to get 'test_cleanup_resize_not_same_host' to pass locally with tox, I don't really know this area - any hints how to resolve this conflict ? https://paste.fedoraproject.org/paste/ntZxhXGlWmzNiL--RZU~Ww | |
| 18:53:30 | mriedem | jmccarthy: see my backports? | |
| 18:53:40 | mriedem | https://review.openstack.org/#/q/Ib10081150e125961cba19cfa821bddfac4614408 | |
| 18:54:23 | jmccarthy | mriedem: Oh ! Ok cool lemme check that out | |
| 18:54:59 | mriedem | arvindn05: i could be wrong but i don't think that actually gets stored in the RequestSpec.image field | |
| 18:55:38 | jmccarthy | mreidem: Perfect - Thanks again ! Quite what to do there was giving me a headache lol | |
| 18:55:59 | arvindn05 | from what i saw...the volume meta data is treated exactly the same as image meta data,...the requestSpec.image has volume meta data in the case | |
| 18:56:36 | arvindn05 | the only difference is that for volume backed instance the "id" field in the RequestSpec.image is not present... | |
| 18:56:36 | mriedem | arvindn05: i'm not so sure https://github.com/openstack/nova/commit/0a7427dc58eeb271646a962604106e0e84b870b6 | |
| 18:56:40 | mriedem | but i don't know what you're seeing | |
| 18:56:54 | mriedem | oh that might be true | |
| 18:57:40 | arvindn05 | yup...the code there looks at the id field as well to determine...so looks like its consistent | |
| 18:57:45 | mriedem | oh i guess we use boot_meta here https://github.com/openstack/nova/blob/master/nova/compute/api.py#L882 | |
| 18:58:28 | mriedem | https://github.com/openstack/nova/blob/master/nova/objects/request_spec.py#L132 | |
| 18:58:30 | arvindn05 | now the only question is should we honor that metadata for volume boot? | |
| 18:59:42 | mriedem | idk | |
| 19:00:00 | arvindn05 | https://blueprints.launchpad.net/python-cinderclient/+spec/support-modify-volume-image-metadata an old blueprint discussion | |
| 19:00:10 | mriedem | i'm not sure how much precedent we have for honoring image meta during scheduling that comes from a volume | |