NO-JIRA: Added reservation affinity support for GCP Machine Spec - WIP #2628
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@MenD32: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Hello @MenD32! Some important instructions when contributing to openshift/api: |
WalkthroughAdds a new GCPReservationAffinity type and an optional ReservationAffinity field to GCPMachineProviderSpec, and updates generated deepcopy, Swagger, and OpenAPI code to include and handle the new type. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
bot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
machine/v1beta1/types_gcpprovider.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
machine/v1beta1/types_gcpprovider.go
🔇 Additional comments (1)
machine/v1beta1/types_gcpprovider.go (1)
211-213: LGTM!The field addition is well-designed: properly documented, correctly marked as optional, and uses a pointer type for optionality.
Signed-off-by: Amit Mendelevitch <amendele@redhat.com>
MenD32
force-pushed
the
feat/gcp-machine-reservation-affinity
branch
from
84292a0 to
8059e5a
Compare
|
Is this feature supported in the upstream cluster-api-provider-gcp? |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
In order for it to be supported, it must be first integrated into the API (adding that feature to the reconciler is my next PR) |
|
@MenD32 To clarify, I meant is the feature supported in https://github.com/kubernetes-sigs/cluster-api-provider-gcp/, which has no dependency on this API that you have raised a PR for |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
machine/v1beta1/types_gcpprovider.go (1)
82-96: Acknowledge NO_RESERVATION fix; conditional validation still needed.Good progress—the
NO_RESERVATIONenum value has been added since the previous review, completing the enum set.However, the conditional validation concern remains: when
ConsumeReservationTypeisSPECIFIC_RESERVATION, bothKeyandValuesmust be non-empty, but kubebuilder tags cannot enforce this. Consider using CEL validation rules (via+kubebuilder:validation:XValidation) to express this constraint declaratively without requiring a webhook service.Example CEL rule:
// +kubebuilder:validation:XValidation:rule="self.consumeReservationType != 'SPECIFIC_RESERVATION' || (has(self.key) && self.key != '' && size(self.values) > 0)",message="key and values are required when consumeReservationType is SPECIFIC_RESERVATION"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
machine/v1beta1/types_gcpprovider.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
machine/v1beta1/types_gcpprovider.go
🔇 Additional comments (1)
machine/v1beta1/types_gcpprovider.go (1)
211-213: LGTM!The
ReservationAffinityfield addition is correctly implemented as an optional pointer field with appropriate JSON tags and documentation.
Ah, thought you were talking about https://github.com/openshift/machine-api-provider-gcp. Its not supported there (in CAPG) currently. |
MenD32
force-pushed
the
feat/gcp-machine-reservation-affinity
branch
3 times, most recently
from
9fa061f to
8a60cec
Compare
Signed-off-by: Amit Mendelevitch <amendele@redhat.com>
MenD32
force-pushed
the
feat/gcp-machine-reservation-affinity
branch
from
8a60cec to
d0a831c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
machine/v1beta1/types_gcpprovider.go(2 hunks)machine/v1beta1/zz_generated.deepcopy.go(2 hunks)machine/v1beta1/zz_generated.swagger_doc_generated.go(2 hunks)openapi/generated_openapi/zz_generated.openapi.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
machine/v1beta1/types_gcpprovider.goopenapi/generated_openapi/zz_generated.openapi.gomachine/v1beta1/zz_generated.swagger_doc_generated.gomachine/v1beta1/zz_generated.deepcopy.go
🧬 Code graph analysis (3)
machine/v1beta1/types_gcpprovider.go (1)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
GCPReservationAffinity(529-531)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
machine/v1beta1/types_gcpprovider.go (1)
GCPReservationAffinity(83-105)
machine/v1beta1/zz_generated.deepcopy.go (1)
machine/v1beta1/types_gcpprovider.go (1)
GCPReservationAffinity(83-105)
🔇 Additional comments (8)
machine/v1beta1/types_gcpprovider.go (1)
220-222: LGTM!The
ReservationAffinityfield is correctly defined as an optional pointer field with appropriate kubebuilder annotations and JSON tags.machine/v1beta1/zz_generated.swagger_doc_generated.go (2)
522-531: LGTM!The generated Swagger documentation correctly reflects the new
GCPReservationAffinitytype with appropriate field descriptions.
482-482: LGTM!The
reservationAffinityfield documentation has been correctly added to theGCPMachineProviderSpecSwagger documentation.machine/v1beta1/zz_generated.deepcopy.go (2)
797-801: LGTM!The integration of
ReservationAffinityintoGCPMachineProviderSpec.DeepCopyIntocorrectly handles the optional pointer field with proper nil checks and deep copying.
895-919: LGTM!The generated
DeepCopyIntoandDeepCopymethods forGCPReservationAffinitycorrectly implement deep copying for all fields:
- Properly handles the
Keypointer field- Correctly copies the
Valuessliceopenapi/generated_openapi/zz_generated.openapi.go (3)
786-786: LGTM! Type registration follows the established pattern.The GCPReservationAffinity type is correctly registered in the OpenAPI definitions map with proper alphabetical ordering.
40039-40044: LGTM! Field addition and dependency updates are correct.The reservationAffinity field is properly added as an optional field (consistent with the pointer type in the source), and the GCPReservationAffinity dependency is correctly included in the dependency list.
Also applies to: 40050-40050
40195-40241: LGTM! Schema definition is mechanically correct.The GCPReservationAffinity schema properly defines the structure with:
- Appropriate field types and descriptions
- Correct use of
x-kubernetes-list-type: "set"for the values array- Documented constraints (RFC1035 compliance, length limits, conditional requirements)
The schema follows established patterns for generated OpenAPI definitions. Note that programmatic validation (such as enum constraints for consumeReservationType or conditional requirements for key/values) would typically be enforced through validation markers in the source type definitions rather than in this generated file.
| // GCPReservationAffinity describes the reservation affinity of the instance on GCP. | ||
| type GCPReservationAffinity struct { | ||
| // consumeReservationType indicates whether the instance should consume from any reservation or a specific reservation. | ||
| // Valid values are "NO_RESERVATION", "ANY_RESERVATION" and "SPECIFIC_RESERVATION". | ||
| // +required | ||
| // +kubebuilder:validation:Enum=NO_RESERVATION;ANY_RESERVATION;SPECIFIC_RESERVATION | ||
| ConsumeReservationType string `json:"consumeReservationType,omitempty"` | ||
| // key is the reservation key of the specific reservation to consume from. | ||
| // The maximum length is 63 characters, and the name must conform to RFC1035. | ||
| // Required if consumeReservationType is set to "SPECIFIC_RESERVATION". | ||
| // When consumeReservationType is not "SPECIFIC_RESERVATION", this field must be empty. | ||
| // +optional | ||
| // +kubebuilder:validation:MaxLength=63 | ||
| Key *string `json:"key,omitempty"` | ||
| // values is the list of reservation values of the specific reservation to consume from. | ||
| // Each value can have a maximum length of 63 characters, and the name must conform to RFC1035. | ||
| // Required if consumeReservationType is set to "SPECIFIC_RESERVATION". | ||
| // When consumeReservationType is not "SPECIFIC_RESERVATION", this field must be empty. | ||
| // +optional | ||
| // +listType=set | ||
| // +kubebuilder:validation:items:MaxLength=63 | ||
| // +kubebuilder:validation:MaxItems=50 | ||
| Values []string `json:"values,omitempty"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the file to confirm the current code state
cat -n machine/v1beta1/types_gcpprovider.go | sed -n '82,105p'Repository: openshift/api
Length of output: 1645
🏁 Script executed:
# Search for similar struct definitions with +required and omitempty to understand project conventions
rg '\+required' machine/v1beta1/types_gcpprovider.go -A 2 -B 2 | head -100Repository: openshift/api
Length of output: 1318
🏁 Script executed:
# Look for other struct definitions in the same file to see patterns
rg 'type.*struct' machine/v1beta1/types_gcpprovider.go -A 15 | head -150Repository: openshift/api
Length of output: 7393
Remove omitempty from required field ConsumeReservationType.
The missing NO_RESERVATION enum value has been correctly added. However, line 88 incorrectly combines +required with omitempty in the JSON tag. Other required fields in this file (e.g., ResourceManagerTag.ParentID) omit the omitempty suffix. Remove it from ConsumeReservationType:
-ConsumeReservationType string `json:"consumeReservationType,omitempty"`
+ConsumeReservationType string `json:"consumeReservationType"`Additionally, the conditional validation constraints documented in lines 91–92 and 98–99 (that Key and Values must be non-empty when ConsumeReservationType == "SPECIFIC_RESERVATION") cannot be enforced via kubebuilder validation tags and will require validation in the provider's reconciler or via a validating admission webhook.
🤖 Prompt for AI Agents
In machine/v1beta1/types_gcpprovider.go around lines 82 to 105, the field
ConsumeReservationType is marked +required but its struct tag includes
`omitempty`; remove `omitempty` from the json tag so the field is always present
in serialized objects (i.e., change `json:"consumeReservationType,omitempty"` to
`json:"consumeReservationType"`). Also note that the conditional requirements
for Key and Values when ConsumeReservationType == "SPECIFIC_RESERVATION" cannot
be enforced via kubebuilder tags — implement those checks in the provider
reconciler or a validating admission webhook to enforce non-empty Key/Values
when SPECIFIC_RESERVATION is selected.
|
@MenD32: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@MenD32 yes the proper flow is to open a PR against CAPG first and then come back to MAPG/OCP once that has been agreed upon. |
Alright, is it ok if I keep this PR as a draft instead of closing it? |
|
Mark this as WIP and then remove the WIP title once you're ready for it to be worked again Please reach out for an API review upstream as well |
MenD32
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
4 participants
Context
Currently, OCP's machine API doesn't support the reservation affinity feature of GCP's instances, thus making reservation use more complicated on OCP in comparison to GCP. This feature is critical for clients with heavy workloads that have already reserved instances for continuous use. (e.g. AI training workloads).
This PR lays the groundwork to support GCP reservation in openshift's machine API.