SPLAT-2615: Added support for legacy AWS dedicated hosts #2650
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@vr4manta: This pull request references SPLAT-2615 which is a valid 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 @vr4manta! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughAdds support for dynamic dedicated-host allocation for AWS host placement: introduces a new public type 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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/L
|
[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 |
|
@vr4manta: This pull request references SPLAT-2615 which is a valid 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
machine/v1beta1/types_awsprovider.go (2)
458-470: Critical:DynamicHostis missing from theHostAffinityenum validation.The
HostAffinitytype at line 459-460 uses+kubebuilder:validation:Enum:=DedicatedHost;AnyAvailablebut does not includeDynamicHost. This means any attempt to setaffinity: DynamicHostwill fail validation at the API server level.🐛 Proposed fix to add DynamicHost to enum validation
// HostAffinity selects how an instance should be placed on AWS Dedicated Hosts. -// +kubebuilder:validation:Enum:=DedicatedHost;AnyAvailable +// +kubebuilder:validation:Enum:=DedicatedHost;AnyAvailable;DynamicHost type HostAffinity string
431-456: Missing validation rule:dynamicHostshould be required when affinity isDynamicHost.The existing XValidation rule at line 432 enforces that
dedicatedHostis required whenaffinity == 'DedicatedHost'. A similar rule should be added for theDynamicHostcase, and mutual exclusivity betweendedicatedHostanddynamicHostshould be enforced.🔧 Proposed fix to add validation rules
// HostPlacement is the type that will be used to configure the placement of AWS instances. // +kubebuilder:validation:XValidation:rule="has(self.affinity) && self.affinity == 'DedicatedHost' ? has(self.dedicatedHost) : true",message="dedicatedHost is required when affinity is DedicatedHost, and optional otherwise" +// +kubebuilder:validation:XValidation:rule="has(self.affinity) && self.affinity == 'DynamicHost' ? has(self.dynamicHost) : true",message="dynamicHost is required when affinity is DynamicHost" +// +kubebuilder:validation:XValidation:rule="!(has(self.dedicatedHost) && has(self.dynamicHost))",message="dedicatedHost and dynamicHost are mutually exclusive" // +union type HostPlacement struct {
🤖 Fix all issues with AI agents
In `@machine/v1beta1/zz_generated.swagger_doc_generated.go`:
- Line 140: The "affinity" description string in
zz_generated.swagger_doc_generated.go currently lists allowed values as
"AnyAvailable and DedicatedHost" but also documents DynamicHost behavior; update
the description for the "affinity" entry to include DynamicHost in the allowed
values list (e.g., "Allowed values are AnyAvailable, DedicatedHost, and
DynamicHost") so the documentation matches the subsequent paragraphs describing
`dynamicHost`; locate the "affinity" key in the generated swagger doc string and
edit the sentence accordingly.
In `@openapi/openapi.json`:
- Around line 24244-24264: The discriminator mapping and validation are
inconsistent: update the x-kubernetes-unions mapping so the "dynamicHost" field
maps to "DynamicHost" (not "DynamicHostAllocation") to match the affinity enum,
and add "DynamicHost" to the kubebuilder enum validation for the affinity field
(ensure the kubebuilder:validation:Enum on the affinity constant includes
DedicatedHost;DynamicHost;AnyAvailable); modify the entries for affinity,
dynamicHost, and dedicatedHost accordingly.
🧹 Nitpick comments (5)
openapi/openapi.json (5)
4582-4585: MissingmaxLengthconstraint for the name field.The description states the name "must not exceed 256 characters," but the schema lacks a
maxLength: 256constraint to enforce this at the API level. Without this constraint, the API will accept strings exceeding the documented limit.Suggested fix
"name": { "description": "name is the name of the acceptable risk. It must be a non-empty string and must not exceed 256 characters.", - "type": "string" + "type": "string", + "maxLength": 256 }
5875-5886: MissingmaxItemsconstraint.The description specifies "must not contain more than 500 entries," but no
maxItems: 500constraint is defined.Suggested fix
"conditionalUpdateRisks": { "description": "conditionalUpdateRisks contains the list of risks associated with conditionalUpdates. When performing a conditional update, all its associated risks will be compared with the set of accepted risks in the spec.desiredUpdate.acceptRisks field. If all risks for a conditional update are included in the spec.desiredUpdate.acceptRisks set, the conditional update can proceed, otherwise it is blocked. The risk names in the list must be unique. conditionalUpdateRisks must not contain more than 500 entries.", "type": "array", "items": { "default": {}, "$ref": "#/definitions/com.github.openshift.api.config.v1.ConditionalUpdateRisk" }, + "maxItems": 500, "x-kubernetes-list-map-keys": [ "name" ], "x-kubernetes-list-type": "map" },
6094-6102: Missing validation constraints forriskNames.The description specifies entry max length of 256 characters and array max of 500 entries, but neither
maxLengthnormaxItemsconstraints are defined.Suggested fix
"riskNames": { "description": "riskNames represents the set of the names of conditionalUpdateRisks that are relevant to this update for some clusters. The Applies condition of each conditionalUpdateRisks entry declares if that risk applies to this cluster. A conditional update is accepted only if each of its risks either does not apply to the cluster or is considered acceptable by the cluster administrator. The latter means that the risk names are included in value of the spec.desiredUpdate.acceptRisks field. Entries must be unique and must not exceed 256 characters. riskNames must not contain more than 500 entries.", "type": "array", "items": { "type": "string", - "default": "" + "default": "", + "maxLength": 256 }, + "maxItems": 500, "x-kubernetes-list-type": "set" },
6129-6140: MissingmaxItemsconstraint.The description states "must not contain more than one entry," but no
maxItems: 1constraint is defined.Suggested fix
"conditions": { "description": "conditions represents the observations of the conditional update risk's current status. Known types are: * Applies, for whether the risk applies to the current cluster. The condition's types in the list must be unique. conditions must not contain more than one entry.", "type": "array", "items": { "default": {}, "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Condition" }, + "maxItems": 1, "x-kubernetes-list-map-keys": [ "type" ], "x-kubernetes-list-type": "map" },
11639-11650: MissingmaxItemsconstraint.The description specifies "must not contain more than 1000 entries," but no
maxItems: 1000constraint is defined.Suggested fix
"acceptRisks": { "description": "acceptRisks is an optional set of names of conditional update risks that are considered acceptable. A conditional update is performed only if all of its risks are acceptable. This list may contain entries that apply to current, previous or future updates. The entries therefore may not map directly to a risk in .status.conditionalUpdateRisks. acceptRisks must not contain more than 1000 entries. Entries in this list must be unique.", "type": "array", "items": { "default": {}, "$ref": "#/definitions/com.github.openshift.api.config.v1.AcceptRisk" }, + "maxItems": 1000, "x-kubernetes-list-map-keys": [ "name" ], "x-kubernetes-list-type": "map" },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
machine/v1beta1/types_awsprovider.gomachine/v1beta1/zz_generated.deepcopy.gomachine/v1beta1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.json
🧰 Additional context used
🧬 Code graph analysis (3)
machine/v1beta1/zz_generated.deepcopy.go (2)
machine/v1beta1/types_awsprovider.go (1)
DynamicHostAllocationSpec(488-492)machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
DynamicHostAllocationSpec(109-111)
machine/v1beta1/types_awsprovider.go (1)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
DynamicHostAllocationSpec(109-111)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
machine/v1beta1/types_awsprovider.go (1)
DynamicHostAllocationSpec(488-492)
🔇 Additional comments (15)
machine/v1beta1/types_awsprovider.go (2)
486-492: LGTM onDynamicHostAllocationSpecstruct definition.The struct is well-defined with appropriate documentation and follows established patterns for tag specifications in the codebase.
450-455: Verify: Feature gate annotation may be needed for consistency.The
DynamicHostAllocationfield doesn't have a feature gate annotation, while the parentHostfield (line 318-319) is gated behindAWSDedicatedHosts. Since the new field is withinHostPlacement, it may inherit the gating, but verify this is intentional and aligns with the team's conventions.machine/v1beta1/zz_generated.deepcopy.go (2)
560-581: LGTM on generated DeepCopy methods forDynamicHostAllocationSpec.The auto-generated deepcopy logic correctly handles the
Tagsmap with proper nil checking and element-by-element copying, consistent with other map-based fields in the codebase.
963-967: LGTM onHostPlacement.DeepCopyIntoextension.The generated code correctly uses
DeepCopyIntoforDynamicHostAllocationsince it contains a map, unlikeDedicatedHostwhich uses direct assignment for its primitive field.openapi/generated_openapi/zz_generated.openapi.go (5)
811-817: LGTM!Schema registration for
DynamicHostAllocationSpecfollows the existing alphabetical ordering and naming convention.
41233-41260: LGTM!The
DynamicHostAllocationSpecschema is well-structured with thetagsproperty correctly defined as a map of strings. The description clearly states the one-host-per-machine allocation behavior.
42036-42041: LGTM!The updated
affinitydescription comprehensively documents the newDynamicHostoption, clearly explaining the allocation behavior and the requirement for thedynamicHostfield.
42049-42054: LGTM!The
dynamicHostproperty is correctly defined with a reference toDynamicHostAllocationSpec. The description appropriately notes the mutual exclusivity withdedicatedHost.
42062-42074: LGTM!The discriminator mapping correctly associates
dynamicHostwithDynamicHostAllocation(matching the Go field name pattern), and dependencies properly includeDynamicHostAllocationSpec.machine/v1beta1/zz_generated.swagger_doc_generated.go (2)
104-111: LGTM!The swagger documentation for
DynamicHostAllocationSpecis well-structured and accurately reflects the type definition fromtypes_awsprovider.go. The descriptions are clear and follow the established pattern in this file.
142-142: LGTM!The
dynamicHostdocumentation clearly describes its purpose and explicitly notes the mutual exclusivity withdedicatedHost, which helps prevent misconfiguration.openapi/openapi.json (4)
6351-6351: TLS profile documentation updates look good.The updated descriptions now correctly reference the Mozilla Server Side TLS configuration guidelines and provide clearer cipher suite examples for each profile type.
Also applies to: 8537-8537, 8815-8815, 9745-9745, 11323-11323, 11334-11350
11684-11686: LGTM!The description text is accurate and grammatically correct.
28978-28978: LGTM!The capability descriptions are consistently updated to include
GuidedTour.Also applies to: 29627-29627
23763-23776: No action required. The schema intentionally delegates tag validation to the AWS provider rather than enforcing constraints at the schema level. This is consistent with the established pattern throughout the codebase, where tag fields (of typeadditionalPropertieswith string values) have no length or count constraints. AWS tag limits (50 tags max, 128-character key limit, 256-character value limit) will be enforced by the AWS provider at runtime, which is the appropriate place for cloud-provider-specific validation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
machine/v1beta1/types_awsprovider.go (1)
431-456: Missing validation:DynamicHostaffinity should requiredynamicHostfield.The existing
XValidationrule at line 432 only enforces thatDedicatedHostaffinity requires thededicatedHostfield. However, the comment at line 438 states that when affinity isDynamicHost, thedynamicHostfield "must be set." This requirement is not enforced by any validation rule.Additionally, consider enforcing mutual exclusivity between
dedicatedHostanddynamicHostfields.🛠️ Suggested validation update
// HostPlacement is the type that will be used to configure the placement of AWS instances. // +kubebuilder:validation:XValidation:rule="has(self.affinity) && self.affinity == 'DedicatedHost' ? has(self.dedicatedHost) : true",message="dedicatedHost is required when affinity is DedicatedHost, and optional otherwise" +// +kubebuilder:validation:XValidation:rule="has(self.affinity) && self.affinity == 'DynamicHost' ? has(self.dynamicHost) : true",message="dynamicHost is required when affinity is DynamicHost" +// +kubebuilder:validation:XValidation:rule="!(has(self.dedicatedHost) && has(self.dynamicHost))",message="dedicatedHost and dynamicHost are mutually exclusive" // +union type HostPlacement struct {
♻️ Duplicate comments (2)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
140-140: Update "Allowed values" to include DynamicHost.The description states "Allowed values are AnyAvailable and DedicatedHost" but then proceeds to describe behavior for
DynamicHost. The allowed values list should include all three options for consistency.openapi/openapi.json (1)
24256-24264: Discriminator value inconsistency:dynamicHostmaps to"DynamicHostAllocation"but should be"DynamicHost".The affinity field description (line 24244) indicates the enum value is
DynamicHost, notDynamicHostAllocation. Compare withdedicatedHostwhich correctly maps to"DedicatedHost". The discriminator value should match the affinity enum value.
🧹 Nitpick comments (1)
machine/v1beta1/types_awsprovider.go (1)
485-492: Consider tag representation consistency across the API.The
Tagsfield inDynamicHostAllocationSpecusesmap[string]string, while instance tags inAWSMachineProviderConfig(line 32) use[]TagSpecificationwith separateNameandValuefields. This inconsistency in tag representation across the API could be confusing for users.Additionally, AWS enforces tag constraints: keys must be 1–128 characters and values 0–256 characters, and tag keys cannot start with the reserved prefix
aws:. Consider documenting these constraints or adding validation if applicable to this new dedicated host allocation feature.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
machine/v1beta1/types_awsprovider.gomachine/v1beta1/zz_generated.deepcopy.gomachine/v1beta1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.json
🧰 Additional context used
🧬 Code graph analysis (3)
machine/v1beta1/types_awsprovider.go (1)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
DynamicHostAllocationSpec(109-111)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
machine/v1beta1/types_awsprovider.go (1)
DynamicHostAllocationSpec(488-492)
machine/v1beta1/zz_generated.deepcopy.go (2)
machine/v1beta1/types_awsprovider.go (1)
DynamicHostAllocationSpec(488-492)machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
DynamicHostAllocationSpec(109-111)
🔇 Additional comments (19)
machine/v1beta1/zz_generated.deepcopy.go (2)
560-581: LGTM - Generated deepcopy functions forDynamicHostAllocationSpecfollow correct patterns.The map deep copy logic for
Tagsmatches the established pattern used elsewhere in this file (e.g.,AzureMachineProviderSpec.Tags,GCPDisk.Labels).
963-967: LGTM - Correctly usesDeepCopyIntofor the new field.The code properly calls
DeepCopyIntoinstead of shallow assignment (**out = **in) becauseDynamicHostAllocationSpeccontains aTagsmap that requires deep copying. This is consistent with how other pointer fields containing reference types are handled in this file.openapi/generated_openapi/zz_generated.openapi.go (5)
811-817: LGTM!The new
DynamicHostAllocationSpectype is correctly registered in the OpenAPI definitions map, following the existing alphabetical ordering convention.
41233-41260: LGTM!The
DynamicHostAllocationSpecschema is well-defined:
- Clear description explaining the purpose (dynamic dedicated host allocation with exactly one host per machine)
- The
tagsproperty correctly defines amap[string]stringusing theAdditionalPropertiespattern- The
tagsfield being optional is appropriate for this use case
42035-42041: LGTM!The affinity field description is well-updated to document the new
DynamicHostoption. The behavior is clearly explained: automatic dedicated host allocation with restart affinity to the same host.
42046-42057: LGTM!The
dynamicHostproperty is correctly added to theHostPlacementschema:
- References the new
DynamicHostAllocationSpectype- Description clearly documents mutual exclusivity with
dedicatedHost- Appropriately optional since it's only required when affinity is
DynamicHost
42062-42075: LGTM!The discriminator mapping and dependencies are correctly updated:
dynamicHostfield maps toDynamicHostAllocationdiscriminator value, aligning with the expected affinity enum value- Dependencies array properly includes
DynamicHostAllocationSpecmachine/v1beta1/zz_generated.swagger_doc_generated.go (2)
104-111: LGTM!The new
DynamicHostAllocationSpecswagger documentation correctly describes the type and itstagsfield, aligning with the struct definition intypes_awsprovider.go.
142-142: LGTM!The
dynamicHostdocumentation correctly describes mutual exclusivity withdedicatedHostand the automatic allocation behavior.machine/v1beta1/types_awsprovider.go (1)
458-471: LGTM!The
HostAffinityenum validation is correctly updated to includeDynamicHost, and the newHostAffinityDynamicHostconstant follows the established naming pattern with appropriate documentation.openapi/openapi.json (9)
4575-4587: LGTM - AcceptRisk type definition looks correct.The type structure is appropriate. Note that while the description mentions the name must be non-empty and ≤256 characters, the JSON schema itself doesn't include
minLength/maxLengthconstraints. This is typical for generated OpenAPI specs where validation is enforced via kubebuilder tags at the Go type level.
5875-5886: LGTM - conditionalUpdateRisks field properly structured.The map-type list with
nameas the key ensures uniqueness. The max 500 entries constraint is documented appropriately.
6094-6102: LGTM - riskNames field correctly uses set semantics.The
x-kubernetes-list-type: setensures entry uniqueness as required.
6129-6140: LGTM - conditions field follows standard Kubernetes pattern.Map-type list keyed by
typeis the correct pattern for condition arrays.
6351-6351: LGTM - TLS profile documentation improvements.The description updates provide consistent formatting and clearer examples for TLS configuration.
Also applies to: 8537-8537, 8815-8815, 9745-9745, 11323-11323, 11334-11350
11639-11650: LGTM - acceptRisks field properly integrated into Update type.The map-type list with
namekey ensures administrators can manage accepted risks with uniqueness guarantees.
11685-11686: LGTM - acceptedRisks description clarification.
23763-23776: LGTM - DynamicHostAllocationSpec type definition is well-structured.The
tagsproperty correctly usesadditionalPropertiesfor key-value string pairs, which aligns with AWS resource tagging patterns.
28978-28979: LGTM - Console capability documentation updates.Also applies to: 29627-29627
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@vr4manta: all tests passed! 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. |
2 participants
SPLAT-2615
Changes
Notes
For dynamic hosts, we are currently allowing admins to provide a set of tags to apply to the new host. Other future enhancements for it can be contained in the new DynamicHostAllocation struct.
This PR is implementing the OCP equivalent to the changes in kubernetes-sigs/cluster-api-provider-aws#5631.