-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(2/5) [nexus] Add Affinity/Anti-Affinity groups to database #7444
base: affinity-api
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions, but overall, this looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally LGTM.
One high-level note, which you can take or leave (and which I hope I'm not duplicating from someone else!): There are a handful of comments both here and in #7445 about the non-atomicity of the "reserve resources" and "create VMM record" steps of the instance start saga. I think those steps could be made atomic pretty easily: the query to reserve resources already operates in a large transaction, and the two subsequent steps of the start saga (bump the next-available IP address on the selected sled and create a new VMM record) don't seem like they would make it that much more expensive. (Perhaps trying to contend on the sled record would increase the chances of transaction conflicts?)
It seems like this is inconvenient enough that it might be worthwhile to try changing it. But I'm leery of restructuring an existing saga like this without the ability to run down existing sagas while preparing to update... and, at least in this PR, it sounds like having that would mitigate some of our concerns about not creating reservations and VMM records atomically. So this might not be worth pursuing right now, but I did at least want to note that if the start saga's existing behavior in this respect is a humongous pain, we can probably change it easily.
I acknowledge that this is possible, but I have a couple concerns I want to address before considering this route.
I filed #7468 to summarize some of my thoughts here. |
Agreed, this is admittedly already a big transaction. I'd definitely be in favor of breaking it up some/trying to convert some of it to a CTE. To step back a bit, my thought process here is:
The two concepts at issue are more than close enough for what we're trying to do at this stage, but in the long run I'd love to see if we can smooth out this rough spot by making it possible to ask "where are the VMMs?" instead of "where are the reservations?". Certainly not a blocker, though--just something I wanted to raise as a possibility. |
I thought we agreed to flatten that table down and remove the "kind" field. I think that if we don't do that, we'll end up with multiple columns that are optional and only applicable to some rows. I think it would be better to instead use multiple resource tables and join them appropriately when making allocation decisions, or come up with a new schema when we actually get to the point of implementing this stuff. |
Yeah, I still plan to do that! I think I'm just speculating on "where are we going to have coupling". Let's suppose we have an For instance specifically, do we want to have this happen in a "reservation" stage prior to VMM creation? Or is the creation of the reservation + VMM record going to happen in the same transaction? I do think either option could work. I just want to make sure we think about future patterns here to be careful about contention of these DB rows. |
Thanks @smklein. That makes sense. I kinda assumed you were using |
Pulled out of #7076
Updates schema to include Affinity/Anti-Affinity groups, but does not use these schemas yet.