Add basic committed resource functionality with reservations#566
Merged
Add basic committed resource functionality with reservations#566
Conversation
knowledge with contentChanged
commitments syncer for flavor group CRs commitments API batch change CRs commitments API version endpoint skeletons for API capacity endpoint basic testing
mblos
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
umswmayj
as code owners
SoWieMarkus
reviewed
Mar 11, 2026
api/v1alpha1/reservation_types.go
Outdated
|
|
||
| // AZ specifies the availability zone for this reservation, if restricted to a specific AZ. | ||
| // +kubebuilder:validation:Optional | ||
| AZ string `json:"az,omitempty"` |
Collaborator
There was a problem hiding this comment.
Can we call that AvailabilityZone instead of AZ?
Member
PhilippMatthes
left a comment
PhilippMatthes
left a comment
There was a problem hiding this comment.
I don't want to gatekeep anything, so you can bring this to main if you like.
These are my findings from my first session reviewing the code.
Unfortunately I wasn't able to look at everything. Hope this helps!
api/v1alpha1/reservation_types.go
Outdated
|
|
||
| // AZ specifies the availability zone for this reservation, if restricted to a specific AZ. | ||
| // +kubebuilder:validation:Optional | ||
| AZ string `json:"az,omitempty"` |
Member
There was a problem hiding this comment.
Should we type this strongly in the spec? In the hypervisor crd, this is just a label. This is fine, I'm just wondering where we should differentiate between spec and labels.
| - nova-decisions-cleanup-task | ||
| # Endpoints configuration for reservations controller | ||
| endpoints: | ||
| novaExternalScheduler: "http://localhost:8080/scheduler/nova/external" |
Member
There was a problem hiding this comment.
This would call localhost, as in the container itself, right? This works as long as the scheduling controllers are initialized within the same pod. Maybe we can make this more robust by 1. adding a comment indicating this dependency or 2. using kubernetes dns to resolve to the correct service, something like cortex-nova-scheduler.svc....
| var flavorGroupsQuery string | ||
|
|
||
| // flavorGroupIdentifierName specifies the extra_spec key used to group flavors. | ||
| const flavorGroupIdentifierName = "quota:hw_version" |
Member
There was a problem hiding this comment.
I would prefer not pulling this out.
|
|
||
| // The smallest flavor in the group (used for CR size quantification) | ||
| SmallestFlavor FlavorInGroup `json:"smallestFlavor"` | ||
| } |
Member
There was a problem hiding this comment.
For features my main concern is always: does this fit into the kubernetes size limit in the production landscape. But I think, this should be fine. Do you want to check just to be safe?
Collaborator
Author
There was a problem hiding this comment.
yes, checked and looks good
| // flavorGroupIdentifierName specifies the extra_spec key used to group flavors. | ||
| const flavorGroupIdentifierName = "quota:hw_version" | ||
|
|
||
| var flavorGroupLog = ctrl.Log.WithName("flavor_group_extractor") |
Member
There was a problem hiding this comment.
Thanks for using the controller log right away instead of slog. This should be the way to go.
Comment on lines
+158
to
+165
| func (m *ReservationManager) buildReservationCRD( | ||
| log logr.Logger, | ||
| state *CommitmentState, | ||
| slotIndex int, | ||
| deltaMemoryBytes int64, | ||
| flavorGroup compute.FlavorGroupFeature, | ||
| creator string, | ||
| ) *v1alpha1.Reservation { |
Member
There was a problem hiding this comment.
This could be a NewReservation() function without the manager as receiver, unless I'm missing why the nested k8s client is needed here.
| if err != nil { | ||
| resp.RejectionReason = fmt.Sprintf("project with unknown resource name %s: %v", projectID, err) | ||
| requireRollback = true | ||
| break ProcessLoop |
Member
There was a problem hiding this comment.
I have to admit, this is the first time I've seen loop markers in go code, didn't even know they existed 😄 To me this sounds like a nice use case to pull each sub-forloop into a separate routine.
| ) | ||
|
|
||
| touchedReservations = make([]v1alpha1.Reservation, 0) | ||
| removedReservations = make([]v1alpha1.Reservation, 0) |
Member
There was a problem hiding this comment.
You can initialize slices as nil and can still iterate over them + append. For example
var touchedReservations []v1alpha1.Reservation
| reservationToDelete = &existing[len(existing)-1] | ||
| existing = existing[:len(existing)-1] // remove from existing list | ||
| } | ||
| removedReservations = append(removedReservations, *reservationToDelete) |
Member
There was a problem hiding this comment.
I tried to understand this code but I failed. I think what I'm missing here would be some good code comments that tell me what this code does.
| // TODO add domain | ||
|
|
||
| // get existing reservations for rollback | ||
| existing_reservations, err := ListReservationsForCommitment(ctx, api.client, string(commitment.UUID), "") |
Member
There was a problem hiding this comment.
Is this helper really needed? All of its usages get creator = ""
Member
There was a problem hiding this comment.
More importantly, will this hit the controller-runtime cache properly or the apiserver?
Collaborator
Author
There was a problem hiding this comment.
Yes, cache is used correctly.
But I decided to introduce labels already in this PR.. making this code useless
SoWieMarkus
reviewed
Mar 11, 2026
Comment on lines
142
to
143
| old := res.DeepCopy() | ||
| patch := client.MergeFrom(old) |
Collaborator
There was a problem hiding this comment.
None of your changes, but we are deep copying after the mutation of the object which will result in an empty diff when merging. Should move this up.
Comment on lines
+72
to
+74
| if _, err := e.DB.Select(&rows, flavorGroupsQuery); err != nil { | ||
| flavorGroupLog.Error(err, "failed to query flavors") | ||
| return nil, err |
Collaborator
There was a problem hiding this comment.
iirc the database can be nil. (See BaseExtractor.ExtractSQL)
Suggested change
| if _, err := e.DB.Select(&rows, flavorGroupsQuery); err != nil { | |
| flavorGroupLog.Error(err, "failed to query flavors") | |
| return nil, err | |
| if e.DB == nil { | |
| return nil, errors.New("database connection is not initialized") | |
| } | |
| if _, err := e.DB.Select(&rows, flavorGroupsQuery); err != nil { | |
| flavorGroupLog.Error(err, "failed to query flavors") | |
| return nil, err |
umswmayj
approved these changes
Mar 12, 2026
Contributor
Test Coverage ReportTest Coverage 📊: 67.9% |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds basic committed resource (CR) functionality: