-
Notifications
You must be signed in to change notification settings - Fork 17
✨ Add multi-architecture IPA support and configurable probes #517
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
base: main
Are you sure you want to change the base?
Conversation
|
Skipping CI for Draft Pull Request. |
|
[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 |
198d5ee to
a14095e
Compare
api/v1alpha1/ironic_types.go
Outdated
| // Format: arch1:url1,arch2:url2 | ||
| // Example: "x86_64:file:///shared/html/images/ipa.x86_64.kernel,aarch64:file:///shared/html/images/ipa.arm64.kernel" | ||
| // +optional | ||
| DeployKernelByArch string `json:"deployKernelByArch,omitempty"` |
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.
I'd rather have a more API-friendly version, something like
directURLs:
- kernel: file:///shared/html/images/ipa.x86_64.kernel
initramfs: file:///shared/html/images/ipa.x86_64.initramfsand for multiple architectures:
- kernel: file:///shared/html/images/ipa.default.kernel
initramfs: file:///shared/html/images/ipa.default.initramfs
- kernel: file:///shared/html/images/ipa.aarch64.kernel
initramfs: file:///shared/html/images/ipa.aarch64.initramfs
architecture: aarch64Several benefits:
- Grouping kernel and initramfs together makes it harder to forget to set one of them
- Users don't need to understand the Ironic's format
- Validation for architecture value
- 1 field instead of 4 proposed
The name directURLs is subject to further discussion. I want to convey that this URL is not fed into the downloader, but is used directly. At the very least, it should be mentioned in the description.
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.
The downside is that user that needs to understand how this is converted to Ironic's format will have to dig this out.
I am not against this approach, but we need to decide how this will be converted into Ironic land, I would assume that in all the cases when this config is specified it gets rendered into *ByArch, in that case it will make sense to simplify this and also it will simplify Probs setup (no more need for overrides)
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.
The downside is that user that needs to understand how this is converted to Ironic's format will have to dig this out.
This format will be self-documenting as everything in Kubernetes (to an extent). The opposite problem is true as well: if we keep the Ironic format, we need users to read the Ironic reference guide.
I would assume that in all the cases when this config is specified it gets rendered into *ByArch
Yes. And if architecture is not set, you can stick it into the default variable (DEPLOY_KERNEL_URL, etc)
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.
Works for me, I will rework this change in this format, should simplify this change significantly
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.
Latest change (still draft), migrates everything to be in overrides, including AgentImages, basically making this verify configurable from consumer side if needed and still keeping code simple enough (no magical and none obvious flows)
|
Thank you for starting this! Some comments inline. |
70c49f2 to
5d764d8
Compare
|
I think that |
Lets handle that in dedicated PR, I don't want to mix and match everything in single place, yes there would be a need to update DHCP configs |
Signed-off-by: s3rj1k <[email protected]>
a277bc6 to
7595cac
Compare
This change allows specifying IPA (Ironic Python Agent) image locations per CPU architecture through the new
spec.overrides.agentImagesfield. Users can define kernel and initramfs URLs for each supported architecture (x86_64, aarch64), which the operator renders asDEPLOY_KERNEL_BY_ARCHandDEPLOY_RAMDISK_BY_ARCHenvironment variables, and those environment variables are consumed by Ironic-image configuration script.Additionally, the httpd container liveness and readiness probes are now configurable via
spec.overrides.httpdLivenessProbeandspec.overrides.httpdReadinessProbe. WhenagentImagesis specified, default probes are disabled and must be configured explicitly if needed.Implements partially #355