| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-08 | |||
| 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 | |
| 17:17:02 | mriedem | So the times are:18:50:51.138 Update port binding 18:50:54.239 Event received 18:50:54.300 Event ignored on both computes 18:50:54.483 Driver finish_revert_migration start | |
| 17:17:06 | dansmith | and you could argue that the latter is better | |
| 17:17:36 | dansmith | I'm a little frustrated by this whole deal, so how about we table this until artom comes back from lunch and we get on a hangout? | |
| 17:17:53 | mriedem | sure, so we could (1) revert my change saying oopsy we still race and then (2) stack on top of that a change to wait for the event prior to changing the port binding which triggers the event | |
| 17:18:09 | dansmith | I don't care about the mechanics | |
| 17:18:23 | mriedem | yeah hangout for sure, since i'm leaving for lunch soon too | |
| 17:18:38 | artom | Cool. Lunch then hangout. At 13... 30? | |
| 17:18:44 | artom | Err, eastern time, so in an hour and 10? | |
| 17:18:45 | mriedem | a hangout will also give me an opportunity to show off my excessively manly beard | |
| 17:19:14 | mriedem | artom: like 3pm ET for me | |
| 17:19:22 | mriedem | which is noon for dan | |
| 17:19:22 | dansmith | mriedem: I assume you mean your sparse patchy white boy beard? | |
| 17:19:31 | mriedem | dansmith: of course | |
| 17:19:42 | dansmith | um, +1:10 from now is not noon for me | |
| 17:20:01 | dansmith | I can't be on a hangout right at noon, but before or shortly after is fine.. but noon is 2:50 from now | |
| 17:20:02 | mriedem | 2pm CT is noon PT yeah? | |
| 17:20:16 | mriedem | yes i'm saying 3PM ET or thereabouts | |
| 17:20:16 | artom | Speak in deltas :) | |
| 17:20:23 | artom | 1:10 from now? | |
| 17:20:28 | mriedem | no | |
| 17:20:29 | dansmith | omg | |
| 17:20:42 | mriedem | how about we just ping later | |
| 17:20:44 | dansmith | (the sound of a gunshot in oregon echoes through) | |
| 17:20:44 | mriedem | break | |