| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2023-01-27 | |||
| 18:09:35 | dansmith | so maybe delete it if we wrote it or something | |
| 18:10:01 | dansmith | although, hang on | |
| 18:10:22 | dansmith | let's say I deploy two new computes with the same hostname by accident, they both generate and write unique uuids, | |
| 18:10:30 | dansmith | one will fail to create their compute node because of the clash, | |
| 18:10:48 | dansmith | but if I just rename the offending duplicate one, then the uuid it generated is fine to use when I restart | |
| 18:10:55 | sean-k-mooney | yep | |
| 18:11:14 | dansmith | the "I deleted the id" case seems pretty edge-y to me, because there are tons of things I can randomly delete from a running system that will break stuff | |
| 18:11:25 | dansmith | if we can abort on key conflict at start, then I'm on board, but otherwise, I dunno | |
| 18:11:48 | dansmith | this is a bit like saying I deleted /etc/ssh/ssh_host_key and when I restarted it generated a new one and my clients all complain :) | |
| 18:12:05 | sean-k-mooney | honestly i did this as my first test because i tough it could be a trivial thing that might happen and we should prevent it | |
| 18:12:24 | sean-k-mooney | dansmith: i was more thinging what happens if you fail to bind mount this in a container | |
| 18:13:26 | sean-k-mooney | if the file is ever lost i think it defeats much of the utility of the feature if we allow the agent to start | |
| 18:13:43 | dansmith | okay, but if you do, there's not much harm, because the resolution is to restart with it, and no harm no foul right? | |
| 18:14:00 | dansmith | if you failed to bind mount this, you likely also failed to bind-mount the instance images no? | |
| 18:14:17 | sean-k-mooney | fair its in the state dir | |
| 18:14:18 | dansmith | in the pre-provisioned case, this goes in /etc/nova anyway | |
| 18:14:22 | sean-k-mooney | although it depend on the location | |
| 18:17:00 | sean-k-mooney | any way im goign to keep testing/breaking it for a while and see what else i find | |
| 18:17:09 | sean-k-mooney | i just added the traceback | |
| 18:17:29 | dansmith | ack, I say we punt on that for the moment and see what else you can find | |
| 18:17:47 | dansmith | maybe we can circle back with some extra stuff that makes that smarter | |
| 18:18:02 | dansmith | at least catching keyerror and logging something that says "okay, here's how you've screwed up..." | |
| 18:18:18 | dansmith | because we have that trace right now and it's not super obvious why | |
| 18:26:47 | sean-k-mooney | so i think the abort is broken in general | |
| 18:28:30 | dansmith | the abort on rename? | |
| 18:28:47 | sean-k-mooney | yep so i tried restarting it with the incorrect uuid | |
| 18:29:03 | sean-k-mooney | and it just keeps trying to create the recoed with the incorrect uuid | |
| 18:29:07 | sean-k-mooney | which keeps failing | |
| 18:29:10 | dansmith | does the uuid exist in the db though? | |
| 18:29:21 | sean-k-mooney | ill check but i dont think so | |
| 18:29:30 | dansmith | then it is doing what it should do | |
| 18:29:35 | dansmith | because that's the pre-provisioned case | |
| 18:29:56 | dansmith | if you create an object in the db with that uuid but a different host, then it should trigger the rename detection | |
| 18:30:15 | sean-k-mooney | so i really think this is broken as is | |
| 18:30:37 | sean-k-mooney | but i will try renames later | |
| 18:30:47 | dansmith | if you give it a uuid that doesn't exist in the database, how is it supposed to know that's not what you want it to use? | |
| 18:31:25 | dansmith | you've told it "this is your uuid, end of story" and if there's no object in the db with that uuid, it's going to try to create it | |
| 18:31:51 | dansmith | it should catch and log a better error than the trace, saying there's a name conflict or something, but otherwise it's doing the right thing I think | |
| 18:31:57 | sean-k-mooney | so i dont think we shoudl be ignoring the host/hypervior_hostname entirely espcially when we get the db duplicte key error | |
| 18:32:28 | dansmith | isn't that the entire point of this series? to break that as the link? | |
| 18:32:40 | sean-k-mooney | no | |
| 18:32:51 | sean-k-mooney | its to make sure a host never change its hostname or host value | |
| 18:32:56 | sean-k-mooney | that is not entirly the same thing | |
| 18:33:14 | sean-k-mooney | we want to make sure we always have a stable way to mapp a host to the same comptue node record | |
| 18:33:14 | dansmith | I totally disagree that that's the point of this series :) | |
| 18:33:29 | sean-k-mooney | and that the host and hypervior_hostname dont change | |
| 18:33:35 | dansmith | because I didn't even have the rename protection in there until late | |
| 18:34:18 | sean-k-mooney | well this would not have prevented the issues our downstream custoemr had without the host/hypersior host checks | |
| 18:34:58 | dansmith | it would, if they didn't delete the state file | |
| 18:35:25 | dansmith | like I said, if the uuid is actually a compute node in the db, but with a hostname that doesn't match, it will abort | |
| 18:35:36 | sean-k-mooney | and you want to rely on them not doing something we told them not to do when they are doing something else we told them not to do :) | |
| 18:35:57 | dansmith | all I want to do is make finding the compute node not tied to the hostname | |
| 18:36:18 | sean-k-mooney | ok so that will just result in it failing with placment | |
| 18:36:43 | sean-k-mooney | because we will move the dduplicate key to ther resouce provider crations | |
| 18:36:55 | dansmith | but we won't be recreating providers | |
| 18:37:06 | dansmith | we'll be accessing them by uuid, which hasn't changed right? | |
| 18:37:24 | dansmith | the hostname may be wrong, and if something else claims the hostname, then it will fail to create one, | |
| 18:37:31 | sean-k-mooney | well im about to start testing that stuff so we will see | |
| 18:37:43 | dansmith | but the case we've had downstream was not that they *shuffled* their compute node names, but rather moved to a different naming schema, still no overlaps | |
| 18:37:52 | sean-k-mooney | setting it back to the correct uuid and ill change the CONF.HOST and hostname seperatly | |
| 18:38:46 | sean-k-mooney | anyway my inital feedback is this is not preventign as much as i was expecting it to | |
| 18:38:47 | dansmith | so here's the accepted spec: https://specs.openstack.org/openstack/nova-specs/specs/2023.1/approved/stable-compute-uuid.html#proposed-change | |
| 18:38:57 | dansmith | and I think that's covered here | |
| 18:39:09 | dansmith | it says that the uuid file will be what we use to find the compute node, | |
| 18:39:26 | dansmith | and we will detect compute node renames | |
| 18:40:29 | sean-k-mooney | right and i kind fo assume "we will not intoduce any new DB exctpions that prevent the resouce tracker form working" woudl be an imporant point too | |
| 18:40:31 | dansmith | I really think that if you take out the "randomly deleted a key state file from the system" then it prevents quite a bit | |
| 18:41:07 | dansmith | how is this a new db exception? it's the same db exception as before if we try to create a conflicting compute node record | |
| 18:41:11 | sean-k-mooney | dansmith: well if your relase automation updated the uuid it would cause the same failure im seeing | |
| 18:41:14 | sean-k-mooney | that was basiclaly step 4 | |
| 18:41:30 | sean-k-mooney | no | |
| 18:41:36 | sean-k-mooney | before we looked it up by hostname | |
| 18:41:43 | sean-k-mooney | and would not have got a colliion in this case | |
| 18:42:00 | sean-k-mooney | so we are trying to create a duplicte record that would not have been created before | |
| 18:42:05 | dansmith | okay I'm getting frustrated | |
| 18:42:20 | dansmith | shall we take this to a gmeet? | |
| 18:42:29 | sean-k-mooney | sure | |
| 18:42:39 | sean-k-mooney | im not trying to frusttrate you by the way | |
| 18:42:49 | sean-k-mooney | just letting you knwo what im finding | |
| 18:42:52 | dansmith | meet.google.com/gkf-fdhr-wgr | |
| 19:26:16 | dansmith | sean-k-mooney: one other thing, we could also assert that if we're already upgraded *and* are not starting fresh, we could abort if the uuid file is missing | |
| 19:26:22 | dansmith | i.e. your didn't-bind-mount case | |
| 19:26:45 | dansmith | although, | |
| 19:27:08 | dansmith | if we handle that in the extra check we discussed, we can say "found X expected Y" which will be the easy way for them to fix their stuff, even if X is empty | |
| 19:27:52 | sean-k-mooney | yes i thought that was one of the things i said above. | |
| 19:28:10 | sean-k-mooney | yep | |
| 19:28:22 | dansmith | oh, maybe I was foaming at the mouth and missed it | |
| 19:30:35 | sean-k-mooney | i read over your comments on teh persist change too so im ok to proceed with that now based on what we discussed | |
| 19:31:02 | dansmith | ack | |
| 19:31:10 | sean-k-mooney | so the first 4 have +w and the first 2 are merged | |
| 19:32:45 | dansmith | thanks, I'll wait until those merge or fail before I push anything else up | |
| 19:33:39 | sean-k-mooney | ok im going to see what else i can break or not break and ill review the last 3 on monday | |
| 19:53:53 | opendevreview | Merged openstack/nova master: Add get_available_node_uuids() to virt driver https://review.opendev.org/c/openstack/nova/+/863917 | |
| 20:04:59 | sean-k-mooney | dansmith: fyi im gong to bold the titles of the tests that look odd | |
| 20:05:58 | sean-k-mooney | but the rename logic does not detact a change in hypervior_hostname today as long as CONF.host does not change | |
| 20:06:07 | sean-k-mooney | https://etherpad.opendev.org/p/Stable-compute-uuid-manual-testing#L216 | |
| 20:06:43 | dansmith | and that's because we get that from libvirt yeah? | |
| 20:06:54 | sean-k-mooney | yep | |
| 20:07:27 | sean-k-mooney | so if the value of virsh hostname changes then we update hypervior_hostname in the db and rename the placemnet RP | |
| 20:07:57 | dansmith | and that's problematic why, just because cinder/neutron will be unhappy? | |