| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-08 | |||
| 15:56:45 | mriedem | gibi: are you going to amend the stein spec for the microversion and all the other stuff that came up? | |
| 15:57:07 | mriedem | i guess i'm not sure what you're asking me :) | |
| 15:57:51 | gibi | mriedem: yeah I'm plannig to amend the spec | |
| 15:57:58 | gibi | mriedem: so I can remove the move from the scope there | |
| 15:58:53 | mriedem | ok | |
| 15:59:01 | gibi | mriedem: ok | |
| 16:03:01 | gibi | mriedem: there are two trailing patche for the bandwidth series both can be considered bugfixes, https://review.openstack.org/#/c/638711/ and https://review.openstack.org/#/c/639608/ what do you think, shall I push them forward before RC1? | |
| 16:09:33 | openstackgerrit | guang-yee proposed openstack/nova master: pass endpoint interface to Ironic client https://review.openstack.org/640879 | |
| 16:27:09 | mriedem | gibi: on https://review.openstack.org/#/c/639608/ i think that's already a potential bug because of not handling errors from claim_resources | |
| 16:27:27 | mriedem | so it's low priority for me, but i think it's a bug we can report and work on | |
| 16:27:57 | mriedem | as i said before that whole while loop needs to be refactored at some point (that whole damn build_instances method actually) | |
| 16:28:46 | mriedem | gibi: https://review.openstack.org/#/c/638711/ isn't really a bug per se, but it's a performance optimization, so again i don't know that we need to rush it, but it's still good to have | |
| 16:29:14 | mriedem | you could report a low priority bug for tracking that if you wanted - as a perf issue | |
| 16:29:28 | mriedem | like i said, if i'm multi-creating 100 instances in one request, that's 100 separate GET /allocations calls | |
| 16:29:49 | mriedem | although, can you multi-create with a pre-created port...? | |
| 16:29:55 | mriedem | i'm not sure that is supported | |
| 16:30:19 | mriedem | that would be like multi-create with the same pre-created volume | |
| 16:30:23 | mriedem | so that likely doesn't work anyway | |
| 16:31:35 | openstackgerrit | Eric Fried proposed openstack/nova master: update gate test for removal of force evacuate https://review.openstack.org/641986 | |
| 16:32:09 | fried_rice | mriedem: sean-k-mooney stephenfin bauzas tssurya ^ | |
| 16:32:53 | fried_rice | http://logs.openstack.org/86/641986/2/check/nova-live-migration/6f6b678/job-output.txt.gz#_2019-03-08_14_32_56_004357 | |
| 16:33:47 | tssurya | fried_rice: oops | |
| 16:34:42 | cfriesen | mriedem: pretty sure you can't multi-create with pre-created port | |
| 16:34:43 | openstackgerrit | Kashyap Chamarthy proposed openstack/nova-specs master: Re-propose the spec to allow specifying a list of CPU models https://review.openstack.org/642030 | |
| 16:36:27 | mriedem | cfriesen: yeah i didn't think so.. | |
| 16:36:56 | mriedem | fried_rice: ah crap | |
| 16:37:09 | fried_rice | mriedem: Did I do it right? I have very little experience with the CLI | |
| 16:38:14 | cfriesen | Could someone take a look at this and clue me in why my new unit test (test_executable_exists_in_path(), in test_utils.py) is failing? Is this a zuul thing? It works fine locally. | |
| 16:38:26 | cfriesen | https://review.openstack.org/#/c/641932 | |
| 16:38:43 | mriedem | cfriesen: i looked at that the last time you asked and i couldn't spot anything obvious, but i'm sure there is some global you're hitting | |
| 16:39:05 | mriedem | fried_rice: yeah i think so, the 'common' options come before the command, and then the per-command options | |
| 16:39:12 | mriedem | i'm not sure if osc is that picky but i think novaclient is | |
| 16:39:40 | fried_rice | cfriesen: Have you tried full-pathing ls? | |
| 16:40:21 | cfriesen | fried_rice: the "ls" is fine, it's the call to libvirt_utils.executable_exists_in_path() that is unexpectedly failing | |
| 16:40:26 | fried_rice | duh, sorry, yeah | |
| 16:40:40 | fried_rice | I hadn't really parsed yet, that was just something that jumped out at a glance. | |
| 16:40:48 | cfriesen | it's totally reliable locally, fails every time in zuul | |
| 16:42:07 | fried_rice | cfriesen: ah, `type`. Shell builtins can be funny. There's a way to invoke a builtin as an executable, looking... | |
| 16:44:06 | fried_rice | cfriesen: Tough that it can't be reproduced locally; but you can try changing "type ..." to "builtin type ..." | |
| 16:45:16 | fried_rice | cfriesen: also, based on the comment, if what you truly care about is that it's an executable (as opposed to a function, alias, or builtin), you could use /usr/bin/which instead of type. | |
| 16:45:21 | mriedem | cfriesen: there is also this https://wiki.openstack.org/wiki/Testr#Reproducing_Failures | |
| 16:45:24 | mriedem | if you get desperate | |
| 16:45:38 | mriedem | ^ is how you can try to spot racing globals | |
| 16:46:02 | fried_rice | mriedem: whoosh, that could use some updating for stestr, huh | |
| 16:46:50 | mriedem | yeah probably | |
| 16:47:01 | mriedem | good thing anyone with a lp account can change it :) | |
| 16:50:25 | fried_rice | cfriesen: oh, sorry again, the problem is likely to be with $PATH, which definitely shouldn't include /tmp in real life. | |
| 16:50:54 | fried_rice | cfriesen: I'm able to repro locally. | |
| 16:52:01 | cfriesen | fried_rice: ah, excellent. wonder why it works locally here | |
| 16:52:13 | cfriesen | I don't have /tmp in PATH | |
| 16:52:31 | fried_rice | yeah, that's really strange cfriesen, because even if you had /tmp in your $PATH, mktemp creates in a randomly-named subdirectory. | |
| 16:56:13 | cfriesen | I think my bash must be messed up, with "type -P" returning whether the binary is executable even if it's not in $PATH | |
| 16:58:10 | cfriesen | fried_rice: interesting, fedora 29 behaves the same as mine | |
| 16:58:36 | cfriesen | maybe I'll just switch to "which". | |
| 16:58:47 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Dont' wait for VIF plugging during revert resize https://review.openstack.org/639396 | |
| 16:58:49 | mriedem | gibi: smallish thing in https://review.openstack.org/#/c/638711/ | |
| 16:58:59 | mriedem | artom: are you going to ever fix that typo in your commit title? | |
| 16:59:06 | mriedem | don't make me -5 you | |
| 16:59:55 | artom | Christ :( | |
| 16:59:56 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Don't wait for VIF plugging during revert resize https://review.openstack.org/639396 | |
| 17:00:03 | artom | It's, err, the possessive form? | |
| 17:01:28 | fried_rice | cfriesen: using type -P with a full path is weird anyway. | |
| 17:02:08 | fried_rice | cfriesen: Are you actually wanting to know if a fully-pathed file is executable, or are you wanting to know if a name (not a full path) is in your PATH *and* executable? | |
| 17:02:49 | fried_rice | cause the former should really be done natively (os.stat | O_EXEC kind of thing) | |
| 17:03:33 | cfriesen | fried_rice: the latter. using the full path was just for the testcase. I'm trying to make sure that "swtpm" exists on the system before advertising support for emulated TPM on that node. | |
| 17:04:43 | fried_rice | cfriesen: Okay. Then the test case shouldn't use the full path either. | |
| 17:04:57 | cfriesen | one could argue that's up to the operator to ensure, in which case I could skip the check entirely. | |
| 17:05:11 | fried_rice | meh, it seems reasonable, if you can get it to work. | |
| 17:07:30 | mriedem | artom: so i don't think you need to change that finish_revert_migration interface at all, | |
| 17:07:47 | mriedem | we just need to be registering an event callback prior to call migrate_instance_fininsh | |
| 17:07:52 | fried_rice | cfriesen: Is / in your $PATH? | |
| 17:08:12 | cfriesen | nope | |
| 17:08:46 | cfriesen | I think it must be a non-exclusive test, where it checks $PATH but also the full path to the executable if specified | |
| 17:09:10 | artom | mriedem, I thought that's what dansmith *didn't* want | |
| 17:09:12 | fried_rice | weird that it's different on different platforms. | |
| 17:09:43 | mriedem | artom: this is the kind of thing that the 3 of us would probably need to get on a hangout to discuss | |
| 17:09:55 | mriedem | so we stop talking past each other while half paying attention | |
| 17:10:03 | dansmith | aren't we just reverting mriedem's patch to expect an event here? | |
| 17:10:27 | artom | dansmith, you just want to +W a mriedem revert, don't you :) | |
| 17:10:50 | artom | I have to go do lunch with colleagues, but I like the hangouts idea | |
| 17:10:54 | mriedem | yes he's reverting my patch but in an uglier way | |
| 17:10:56 | dansmith | artom: no, I want to avoid the other thing, and since we've been in that "don't wait for the event" state a few times now, I think first step is just getting back to that, unless there's some reason not to | |
| 17:11:03 | cfriesen | fried_rice: if I modify os.environ['PATH'] in the testcase to include the directory containing the temp dir, do you think that'd work? | |
| 17:11:14 | fried_rice | cfriesen: I tried that locally and it didn't work, no. | |
| 17:11:17 | fried_rice | but I'm not sure why. | |
| 17:11:37 | artom | dansmith, so you'd rather I just straight up revert mriedem's patch? Functionally it's the same | |
| 17:11:39 | cfriesen | guess I'm switching to "which" then. :) | |
| 17:12:07 | mriedem | artom: before doing that i'd like to have a plan, | |
| 17:12:13 | fried_rice | cfriesen: Note that type -P doesn't check whether the thing is executable (at least on my bionic) | |
| 17:12:20 | mriedem | because if we know the event is coming, i don't see why we don't wait for it | |
| 17:12:29 | fried_rice | -rw-r--r-- 1 efried efried 4169 Mar 8 08:53 /tmp/f | |
| 17:12:29 | fried_rice | efried:~/openstack/nova$ ll /tmp/f | |
| 17:12:29 | fried_rice | f is /tmp/f | |
| 17:12:29 | fried_rice | efried:~/openstack/nova$ PATH=$PATH:/tmp type f | |
| 17:12:30 | dansmith | artom: I'm not saying a full revert vs. tactical is important | |
| 17:12:33 | mriedem | clearly my change worked albeit is racy | |
| 17:12:47 | artom | mriedem, ah, I see what you mean | |
| 17:12:49 | dansmith | mriedem: because it's hard to do that based on the arrangement of the code | |
| 17:12:56 | mriedem | artom: because i'm pretty sure i made that change b/c of gate failures | |
| 17:13:06 | mriedem | i.e. resize revert + ssh in tempest race fails | |