Enable hardware Provisioning through ISO booting for baremetal Provider #9213

rahulbabu95 · 2025-01-31T00:42:30Z

Issue #, if available:
https://github.com/aws/eks-anywhere-internal/issues/2212

Description of changes:
Upstream CAPT/Tink recently added support for booting the hardware through ISO. This change adds the necessary changes to our cluster spec notably TinkerbellDataCenterConfig to be able to provision the hardware using an ISO. ISO booting removes the dependency of the admin machine to provision the cluster to be in the same L2 as the hardware. Additionally offers static IPAM. The PR adds necessary changes required in our TinkerbellmachineTemplate to be able to ISO boot. The default behavior would still be to netboot. PR also adds required changes at the time of Tinkerbell stack installation so that Smee has all the required flags set to be able to serve ISO and handles toggling between the bootstrap IP and the actual TinkerbelIP.

Additionally, the previous implementation to handle the toggling between the Bootstrap IP to the actual Tinkerbell IP when moving the cluster from kind to the actual CAPI workload cluster had a bug in the sense the underlying eks-a cluster object always had the bootstrap-ip annotation, which meant the controller always also picked up the host IP for the Hegel URLs. This change also fixes that bug by actually updating the underlying eks-a cluster object.

Testing (if applicable):
Manually created a cluster using an Hook ISO image.

Documentation added/planned (if applicable):

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

eks-distro-bot · 2025-01-31T00:42:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from rahulbabu95. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rahulbabu95 · 2025-01-31T00:43:09Z

/hold

codecov · 2025-01-31T00:48:23Z

Codecov Report

Attention: Patch coverage is 54.87805% with 74 lines in your changes missing coverage. Please review.

Project coverage is 72.32%. Comparing base (0ac372d) to head (d360b25).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/clustermanager/cluster_manager.go	2.94%	64 Missing and 2 partials ⚠️
pkg/providers/tinkerbell/stack/stack.go	87.09%	4 Missing ⚠️
pkg/providers/tinkerbell/validate.go	33.33%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9213      +/-   ##
==========================================
- Coverage   72.37%   72.32%   -0.05%     
==========================================
  Files         585      587       +2     
  Lines       45708    46056     +348     
==========================================
+ Hits        33079    33309     +230     
- Misses      10890    10998     +108     
- Partials     1739     1749      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rahulbabu95 · 2025-01-31T10:57:08Z

/retest

panktishah26 · 2025-01-31T19:59:26Z

/test eks-anywhere-release-tooling-test-presubmit

eks-distro-bot · 2025-01-31T20:07:16Z

@rahulbabu95: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
eks-anywhere-release-tooling-test-presubmit	`70428c3`	link	true	`/test eks-anywhere-release-tooling-test-presubmit`

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

jacobweinstock · 2025-02-05T03:32:59Z

pkg/api/v1alpha1/tinkerbelldatacenterconfig.go

+	if config.Spec.IsoBoot {
+		if config.Spec.HookIsoURL != "" {
+			if _, err := url.ParseRequestURI(config.Spec.HookIsoURL); err != nil {
+				return fmt.Errorf("parsing isoURL: %v", err)


nothing to hold up the PR, but something to think about. Because our code base doesn't have a good presentation layer, error messages like this most of the time get surfaced to the user. So while normally an error message that references some internal or local field or variable name is ok, you should think about what name makes sense from the user's perspective.

i think providing the exact field name does help from a user's perspective in the sense that they can go back to the spec to the exact field that causes the error. That being said i will add more context around the error.

jacobweinstock · 2025-02-05T03:33:24Z

pkg/api/v1alpha1/tinkerbelldatacenterconfig_types.go

@@ -26,6 +26,13 @@ type TinkerbellDatacenterConfigSpec struct {
 	SkipLoadBalancerDeployment bool `json:"skipLoadBalancerDeployment,omitempty"`
 	// LoadBalancerInterface can be used to configure a load balancer interface for the Tinkerbell stack.
 	LoadBalancerInterface string `json:"loadBalancerInterface,omitempty"`
+	// IsoBoot can be used to indicate that the hardware must boot using an ISO.


must -> should

jacobweinstock · 2025-02-05T03:35:38Z

pkg/api/v1alpha1/tinkerbelldatacenterconfig_types.go

+	// IsoBoot can be used to indicate that the hardware must boot using an ISO.
+	//+optional
+	IsoBoot bool `json:"isoBoot,omitempty"`
+	// HookIsoURL is the URL of ISO image that will one time boot.


one time boot is indeed accurate but is this how you would describe this to a user? is there other ways to describe the ISO, like being used during the provisioning process, etc.

jacobweinstock · 2025-02-05T03:42:53Z

pkg/providers/tinkerbell/stack/stack.go

+	Install(ctx context.Context, bundle releasev1alpha1.TinkerbellBundle, tinkerbellIP, kubeconfig, hookOverride, isoOverride string, opts ...InstallOption) error
 	UninstallLocal(ctx context.Context) error
 	Uninstall(ctx context.Context, bundle releasev1alpha1.TinkerbellBundle, kubeconfig string) error
-	Upgrade(_ context.Context, _ releasev1alpha1.TinkerbellBundle, tinkerbellIP, kubeconfig, hookOverride string, opts ...InstallOption) error
+	Upgrade(_ context.Context, _ releasev1alpha1.TinkerbellBundle, tinkerbellIP, kubeconfig, hookOverride, isoOverride string, opts ...InstallOption) error


These two interface methods have ...InstallOption. This should allow extending these methods without modifying their function signatures.

Added the field to the Installer struct and updated using the functional options pattern!

jacobweinstock · 2025-02-05T03:43:32Z

pkg/providers/tinkerbell/hardware/catalogue_bmc.go

@@ -11,6 +11,9 @@ import (
 	"github.com/aws/eks-anywhere/pkg/constants"
 )

+// GofishProviderOption is the provider name for Redfish Provider in Rufio.
+const GofishProviderOption = "gofish"


Does this need to be exported?

Good qn. the test file uses this constant to test providerOptions. i do not want to maintain two copies of this constant value hence chose to export it.

Upstream CAPT/Tink recently added support for booting the hardware through ISO. This change adds the necessary changes to our cluster spec notably TinkerbellDataCenterConfig to be able to provision the hardware using an ISO. ISO booting removes the dependency of the admin machine to provision the cluster to be in the same L2 as the hardware. Additionally offers static IPAM. The PR adds necessary changes required in our TinkerbellmachineTemplate to be able to ISO boot. The default behavior would still be to netboot. PR also adds required changes at the time of Tinkerbell stack installation so that Smee has all the required flags set to be able to serve ISO and handles toggling between the bootstrap IP and the actual TinkerbelIP. Signed-off-by: Rahul Ganesh <rahulgab@amazon.com>

rahulbabu95 · 2025-02-05T18:07:07Z

/unhold

rahulbabu95 · 2025-02-05T18:07:42Z

/override codecov/patch

eks-distro-bot · 2025-02-05T18:07:49Z

@rahulbabu95: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

codecov/patch

Only the following failed contexts/checkruns were expected:

eks-anywhere-cli-attribution-presubmit
eks-anywhere-cluster-controller-tooling-presubmit
eks-anywhere-e2e-presubmit
eks-anywhere-generate-files-presubmit
eks-anywhere-presubmit
tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/override codecov/patch

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/test-infra repository.

eks-distro-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 31, 2025

eks-distro-bot added the do-not-merge/hold label Jan 31, 2025

rahulbabu95 force-pushed the l3/add-boot-options branch from 7dde957 to a76cb90 Compare January 31, 2025 04:01

eks-distro-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 31, 2025

rahulbabu95 force-pushed the l3/add-boot-options branch 2 times, most recently from 5f4532c to bfa2b34 Compare February 5, 2025 00:40

jacobweinstock reviewed Feb 5, 2025

View reviewed changes

rahulbabu95 force-pushed the l3/add-boot-options branch from bfa2b34 to d360b25 Compare February 5, 2025 10:20

jacobweinstock approved these changes Feb 5, 2025

View reviewed changes

eks-distro-bot assigned jacobweinstock Feb 5, 2025

eks-distro-bot added the lgtm label Feb 5, 2025

eks-distro-bot removed the do-not-merge/hold label Feb 5, 2025

rahulbabu95 merged commit 21b4f95 into aws:main Feb 5, 2025
11 of 13 checks passed

csplinter mentioned this pull request Feb 28, 2025

Changelog for v0.22.0 #9313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable hardware Provisioning through ISO booting for baremetal Provider #9213

Enable hardware Provisioning through ISO booting for baremetal Provider #9213

rahulbabu95 commented Jan 31, 2025 •

edited

Loading

eks-distro-bot commented Jan 31, 2025

rahulbabu95 commented Jan 31, 2025

codecov bot commented Jan 31, 2025 •

edited

Loading

rahulbabu95 commented Jan 31, 2025

panktishah26 commented Jan 31, 2025

eks-distro-bot commented Jan 31, 2025

jacobweinstock Feb 5, 2025

rahulbabu95 Feb 5, 2025

jacobweinstock Feb 5, 2025

jacobweinstock Feb 5, 2025

jacobweinstock Feb 5, 2025

rahulbabu95 Feb 5, 2025

jacobweinstock Feb 5, 2025

rahulbabu95 Feb 5, 2025

rahulbabu95 commented Feb 5, 2025

rahulbabu95 commented Feb 5, 2025

eks-distro-bot commented Feb 5, 2025

Enable hardware Provisioning through ISO booting for baremetal Provider #9213

Enable hardware Provisioning through ISO booting for baremetal Provider #9213

Conversation

rahulbabu95 commented Jan 31, 2025 • edited Loading

eks-distro-bot commented Jan 31, 2025

rahulbabu95 commented Jan 31, 2025

codecov bot commented Jan 31, 2025 • edited Loading

Codecov Report

rahulbabu95 commented Jan 31, 2025

panktishah26 commented Jan 31, 2025

eks-distro-bot commented Jan 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahulbabu95 commented Feb 5, 2025

rahulbabu95 commented Feb 5, 2025

eks-distro-bot commented Feb 5, 2025

rahulbabu95 commented Jan 31, 2025 •

edited

Loading

codecov bot commented Jan 31, 2025 •

edited

Loading