RFE-9669: Add spec.platform.aws.amiID to MachinePool - #2935
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sadiquepp: This pull request references RFE-9669 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds an optional AWS ChangesAWS AMI override
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant MachinePool
participant NewAWSActuator
participant Annotation
participant MasterMachine
MachinePool->>NewAWSActuator: provide spec.platform.aws.amiID
NewAWSActuator->>Annotation: read override when AMIID is unset
NewAWSActuator->>MasterMachine: resolve AMI when overrides are unset
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/controller/machinepool/awsactuator.go (2)
65-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the injected logger for the new AMI message.
Line 68 calls package-level
log.Infof, while this constructor otherwise uses the injectedlogger. This bypasses controller-specific logging configuration and context; uselogger.Infofor a structuredlogger.WithField(...)call instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/machinepool/awsactuator.go` around lines 65 - 69, Update the AMI selection branch in the machine pool constructor to replace the package-level log.Infof call with the injected logger, using logger.Infof or an equivalent structured logger call while preserving the existing message and AMI value.
65-88: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd tests for AMI precedence and fallback.
Cover spec AMI ID over annotation, annotation fallback when the field is nil or empty, and master-based resolution when neither override is present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/machinepool/awsactuator.go` around lines 65 - 88, Add tests covering AMI selection in the logic surrounding masterAMIRefByTag and getAWSAMIID: verify a non-empty pool.Spec.Platform.AWS.AMIID takes precedence over MachinePoolImageIDOverrideAnnotation, the annotation is used when the spec field is nil or empty, and master-based resolution occurs when neither override is set. Assert the selected AMI and relevant error behavior for each fallback path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/controller/machinepool/awsactuator.go`:
- Around line 65-69: Update the AMI selection branch in the machine pool
constructor to replace the package-level log.Infof call with the injected
logger, using logger.Infof or an equivalent structured logger call while
preserving the existing message and AMI value.
- Around line 65-88: Add tests covering AMI selection in the logic surrounding
masterAMIRefByTag and getAWSAMIID: verify a non-empty
pool.Spec.Platform.AWS.AMIID takes precedence over
MachinePoolImageIDOverrideAnnotation, the annotation is used when the spec field
is nil or empty, and master-based resolution occurs when neither override is
set. Assert the selected AMI and relevant error behavior for each fallback path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c05c602-653c-48a4-9981-dafae7825ac9
📒 Files selected for processing (8)
apis/hive/v1/aws/machinepool.goapis/hive/v1/aws/zz_generated.deepcopy.goconfig/crds/hive.openshift.io_machinepools.yamldocs/using-hive.mdhack/app-sre/saas-template.yamlpkg/controller/machinepool/awsactuator.govendor/github.com/openshift/hive/apis/hive/v1/aws/machinepool.govendor/github.com/openshift/hive/apis/hive/v1/aws/zz_generated.deepcopy.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2935 +/- ##
==========================================
- Coverage 50.51% 50.47% -0.04%
==========================================
Files 282 282
Lines 34485 34679 +194
==========================================
+ Hits 17420 17505 +85
- Misses 15707 15815 +108
- Partials 1358 1359 +1
🚀 New features to boost your workflow:
|
2uasimojo
left a comment
2uasimojo
left a comment
There was a problem hiding this comment.
This looks great, thanks! A few minor notes inline.
| // existing behavior of resolving the AMI from the cluster's master machine (or from the | ||
| // hive.openshift.io/image-id-override annotation, if set). |
There was a problem hiding this comment.
I think we should probably not mention the annotation at all in the official API docstring. In fact, with or very shortly after this PR, we may want to consider deprecating it.
|
|
||
| #### Customizing AMI Images | ||
|
|
||
| The AMI image to use for the worker nodes can be customized by specifying the `spec.platform.aws.amiID` field. |
There was a problem hiding this comment.
AMI image
RAS syndrome.
This part of the document tries to be platform-agnostic as much as possible, even though many of the snippets necessarily use a specific platform's config as an example. Can we reword this to be more generic? Like:
#### Customizing VM boot images
By default, we try to determine an appropriate boot image for VMs defined through MachinePools.
For some platforms, a specific image can be supplied in the MachinePool spec.
For example, for AWS, the AMI ID can be specified as follows:
I just looked and it turns out Azure is the only other platform where we allow OS image customization. So we could include an Azure example as well. But in general we would prefer to let the API be authoritative rather than trying to keep this doc comprehensive.
| var amiID string | ||
| if pool.Spec.Platform.AWS != nil && pool.Spec.Platform.AWS.AMIID != nil && *pool.Spec.Platform.AWS.AMIID != "" { | ||
| amiID = *pool.Spec.Platform.AWS.AMIID | ||
| log.Infof("using AMI from spec.platform.aws.amiID: %s", amiID) |
There was a problem hiding this comment.
I agree with the rabbit that we should use logger rather than log here.
Also, prefer structured logging (logger.WithField("AMIID", amiID).Info("using AMI from spec.platform.aws.amiID"))
I recognize you're following the neighboring code... but if you wouldn't mind fixing up L71 similarly, that would be great 👍
6e5f9bd to
d565406
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controller/machinepool/awsactuator.go (1)
65-88: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd tests for the new AMI precedence contract.
The supplied test context covers the master-machine fallback but not
NewAWSActuatorselecting a spec AMIID over the annotation, or the annotation over fallback. Add or verify table-driven cases for those paths, including nil and empty spec values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/machinepool/awsactuator.go` around lines 65 - 88, Add table-driven tests covering AMI precedence in NewAWSActuator: a non-empty spec.platform.aws.AMIID must override the annotation, a non-empty annotation must override master-machine fallback, and nil or empty spec AMIID values must continue to the annotation/fallback paths. Reuse the existing master-machine fallback test setup and assert the selected AMI for each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apis/hive/v1/aws/machinepool.go`:
- Around line 46-50: Align the canonical AMIID documentation in the API source
with the intended fallback wording, then regenerate the Hive vendored type and
published machinepool CRD from the canonical Hive definitions. Do not edit
vendor/github.com/openshift/hive or the CRD directly; ensure all three artifacts
expose identical AMIID documentation.
---
Nitpick comments:
In `@pkg/controller/machinepool/awsactuator.go`:
- Around line 65-88: Add table-driven tests covering AMI precedence in
NewAWSActuator: a non-empty spec.platform.aws.AMIID must override the
annotation, a non-empty annotation must override master-machine fallback, and
nil or empty spec AMIID values must continue to the annotation/fallback paths.
Reuse the existing master-machine fallback test setup and assert the selected
AMI for each case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d254f61b-6574-49fb-8723-ac3e299c6b9e
📒 Files selected for processing (8)
apis/hive/v1/aws/machinepool.goapis/hive/v1/aws/zz_generated.deepcopy.goconfig/crds/hive.openshift.io_machinepools.yamldocs/using-hive.mdhack/app-sre/saas-template.yamlpkg/controller/machinepool/awsactuator.govendor/github.com/openshift/hive/apis/hive/v1/aws/machinepool.govendor/github.com/openshift/hive/apis/hive/v1/aws/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (3)
- apis/hive/v1/aws/zz_generated.deepcopy.go
- hack/app-sre/saas-template.yaml
- docs/using-hive.md
|
|
||
| // AMIID is the AMI to use for machines in this pool. When omitted, Hive falls back to its | ||
| // existing behavior of determining an appropriate boot image for the platform. | ||
| // +optional | ||
| AMIID *string `json:"amiID,omitempty"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Regenerate the dependent API artifacts from one canonical comment.
The AMIID documentation differs across the changed artifacts: vendor/github.com/openshift/hive/apis/hive/v1/aws/machinepool.go:47-49 and config/crds/hive.openshift.io_machinepools.yaml:125-127 describe master/annotation fallback, while this source uses generic fallback wording. Choose the canonical wording, then regenerate the vendored copy and CRD so the API source, vendored type, and published schema do not drift.
As per path instructions, the Hive vendored definitions must mirror the corresponding API definitions and CRDs must be generated from Hive types. Based on learnings, update the canonical source and regenerate vendored artifacts rather than editing vendor/ directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apis/hive/v1/aws/machinepool.go` around lines 46 - 50, Align the canonical
AMIID documentation in the API source with the intended fallback wording, then
regenerate the Hive vendored type and published machinepool CRD from the
canonical Hive definitions. Do not edit vendor/github.com/openshift/hive or the
CRD directly; ensure all three artifacts expose identical AMIID documentation.
Sources: Path instructions, Learnings
Allow specifying a custom AMI directly on a MachinePool's AWS platform spec, rather than only through the existing, explicitly-unsupported hive.openshift.io/image-id-override annotation. - Add AMIID *string to apis/hive/v1/aws.MachinePoolPlatform. - In pkg/controller/machinepool/awsactuator.go, resolve the AMI in this order: spec.platform.aws.amiID, then the hive.openshift.io/image-id-override annotation (unchanged), then the existing master-machine lookup fallback. - Regenerate deepcopy, CRD manifests, and the app-sre SaaS template via make update. Assisted-by: Claude (Anthropic) Signed-off-by: Sadique <sadiqueklr@gmail.com>
d565406 to
59c38f2
Compare
|
/lgtm Thanks @sadiquepp! /override ci/prow/security See #2937 /hold to control merge order |
openshift-ci
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo, sadiquepp The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@2uasimojo: Overrode contexts on behalf of 2uasimojo: ci/prow/security 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 kubernetes-sigs/prow repository. |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
I think this will rebase, retest, and then merge. We should no longer need to override |
|
@sadiquepp: 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. |
3 participants
Allow specifying a custom AMI directly on a MachinePool's AWS platform spec, rather than only through the existing, explicitly-unsupported hive.openshift.io/image-id-override annotation.
Assisted-by: Claude (Anthropic)
Summary by CodeRabbit
spec.platform.aws.amiID.spec.platform.aws.amiID, then the existing cluster annotation (when set), then the existing master-based AMI resolution.amiIDexample.