| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-08 | |||
| 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 | |
| 17:13:11 | mriedem | b/c we aren't waiting for plugging | |
| 17:13:19 | mriedem | so you revert this w/o fixing that and we've got new gate failures again | |
| 17:13:21 | dansmith | that's why this whole thing exists, yes | |
| 17:13:25 | mriedem | well, same old gate failures | |
| 17:13:33 | cfriesen | fried_rice: oh, good catch. | |
| 17:13:35 | mriedem | dansmith: we can do the same thing we do for live migration | |
| 17:13:49 | dansmith | can we not wait for the plug event before leaving the compute manager ? | |
| 17:13:58 | fried_rice | cfriesen: `which` is much better, because it also doesn't fire if the file is executable, but not by you. | |
| 17:14:03 | dansmith | like way before we get to the driver.finish_revert_migration() ? | |
| 17:14:26 | artom | dansmith, ah, right. I thought that wasn't possible because we needed the virt driver to plug the vifs | |
| 17:14:40 | dansmith | artom: you've been saying the virt driver isn't causing the vif plug event | |
| 17:14:41 | artom | But if they're already plugged, we can wait in the compute manager, and tell virt not to wait for it | |
| 17:14:43 | dansmith | so which is it? | |
| 17:14:49 | dansmith | right. | |
| 17:15:23 | artom | dansmith, I'm not 100% on how the Neutron agent works, but my understanding is that Neutron will only wire the thing if the virt driver had plugged it | |
| 17:15:38 | dansmith | artom: that contradicts what you said earlier, | |
| 17:15:39 | artom | So we need both: poke Neutron, and plug (or have it be already plugged) | |
| 17:15:57 | dansmith | which is that the vif is already on the source, so the network_api call is what is causing neutron to send the event, which we might miss if we're not fast enough | |
| 17:16:20 | artom | dansmith, because it was never unplugged to begin with | |
| 17:16:22 | mriedem | dansmith: "like way before we get to the driver.finish_revert_migration() ?" yes that's what i' msaying | |
| 17:16:56 | mriedem | artom: right here you are contradicting what you just said, https://review.openstack.org/#/c/639396/3/nova/compute/manager.py@4184 | |
| 17:17:01 | dansmith | mriedem: gotcha, I'm with you now.. I thought yesterday you were saying that a straight revert was the best first step.. with this, we race in control plane code, without this, we race on the instance | |