Skip to content
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.

Sign up for GitHub

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

Jump to bottom

✨ Pass preprovisioningNetworkData to Ironic #1380

Merged
merged 2 commits into from
Oct 20, 2023
Merged

✨ Pass preprovisioningNetworkData to Ironic #1380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a56dff
HostConfigData add PreprovisioningNetworkData
hardys Oct 4, 2023
7de3670
Conditionally pass preprovisioningNetworkData to Ironic
hardys Oct 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions controllers/metal3.io/baremetalhost_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,14 +802,25 @@ func (r *BareMetalHostReconciler) registerHost(prov provisioner.Provisioner, inf
return actionError{err}
}

hostConf := &hostConfigData{
host: info.host,
log: info.log.WithName("host_config_data"),
secretManager: r.secretManager(info.log),
}
preprovisioningNetworkData, err := hostConf.PreprovisioningNetworkData()
if err != nil {
return recordActionFailure(info, metal3api.RegistrationError, "failed to read preprovisioningNetworkData")
}

provResult, provID, err := prov.ValidateManagementAccess(
provisioner.ManagementAccessData{
BootMode: info.host.Status.Provisioning.BootMode,
AutomatedCleaningMode: info.host.Spec.AutomatedCleaningMode,
State: info.host.Status.Provisioning.State,
CurrentImage: getCurrentImage(info.host),
PreprovisioningImage: preprovImg,
HasCustomDeploy: hasCustomDeploy(info.host),
BootMode: info.host.Status.Provisioning.BootMode,
AutomatedCleaningMode: info.host.Spec.AutomatedCleaningMode,
State: info.host.Status.Provisioning.State,
CurrentImage: getCurrentImage(info.host),
PreprovisioningImage: preprovImg,
PreprovisioningNetworkData: preprovisioningNetworkData,
HasCustomDeploy: hasCustomDeploy(info.host),
},
credsChanged,
info.host.Status.ErrorType == metal3api.RegistrationError)
Expand Down
20 changes: 20 additions & 0 deletions controllers/metal3.io/host_config_data.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,26 @@ func (hcd *hostConfigData) NetworkData() (string, error) {
return networkDataRaw, err
}

// PreprovisioningNetworkData get preprovisioning network configuration
func (hcd *hostConfigData) PreprovisioningNetworkData() (string, error) {
if hcd.host.Spec.PreprovisioningNetworkDataName == "" {
return "", nil
}
networkDataRaw, err := hcd.getSecretData(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some more digging into the difference between ObtainSecret() (used here) and AcquireSecret(), after having got it wrong several times 😄

Both add a label to the Secret so that it gets watched and cached.
Only AcquireSecret() adds an owner reference so that the BMH gets reconciled if the Secret changes.

In the case of network/user/metaData, what matters is the value at a point in time, when you start provisioning. It's the start of provisioning that kicks off the reconcile, so there's no need to re-reconcile when the Secret changes.

There's a strong argument that preprovisioningNetworkData is the same, and what matters is the value at the point in time where you start inspecting/preparing/deprovisioning.
There's probably another argument that since ironic may reboot the node in response to its own internal events (not the BMO), then we should always provide it with the most up-to-date data we know of. In practice we will get there eventually because we always requeue every few minutes at most, but there may be situations where something unintuitive happens.

This would be interesting to discuss further but not worth blocking on.

hcd.host.Spec.PreprovisioningNetworkDataName,
hcd.host.Namespace,
"networkData",
)
if err != nil {
_, isNoDataErr := err.(NoDataInSecretError)
if isNoDataErr {
hcd.log.Info("PreprovisioningNetworkData networkData key is not set, returning empty data")
return "", nil
}
}
return networkDataRaw, err
}

// MetaData get host metatdata
func (hcd *hostConfigData) MetaData() (string, error) {
if hcd.host.Spec.MetaData == nil {
Expand Down
61 changes: 38 additions & 23 deletions controllers/metal3.io/host_config_data_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,17 +92,19 @@ func TestProvisionWithHostConfig(t *testing.T) {
testBMCSecret := newBMCCredsSecret(defaultSecretName, "User", "Pass")

testCases := []struct {
Scenario string
Host *metal3api.BareMetalHost
UserDataSecret *corev1.Secret
PreprovNetworkDataSecret *corev1.Secret
NetworkDataSecret *corev1.Secret
ExpectedUserData string
ErrUserData bool
ExpectedNetworkData string
ErrNetworkData bool
ExpectedMetaData string
ErrMetaData bool
Scenario string
Host *metal3api.BareMetalHost
UserDataSecret *corev1.Secret
PreprovNetworkDataSecret *corev1.Secret
NetworkDataSecret *corev1.Secret
ExpectedUserData string
ErrUserData bool
ExpectedNetworkData string
ErrNetworkData bool
ExpectedPreprovisioningNetworkData string
ErrPreprovisioningNetworkData bool
ExpectedMetaData string
ErrMetaData bool
}{
{
Scenario: "host with user data only",
Expand Down Expand Up @@ -151,11 +153,12 @@ func TestProvisionWithHostConfig(t *testing.T) {
},
PreprovisioningNetworkDataName: "net-data",
}),
NetworkDataSecret: newSecret("net-data", map[string]string{"networkData": "key: value"}),
ExpectedUserData: "",
ErrUserData: false,
ExpectedNetworkData: base64.StdEncoding.EncodeToString([]byte("key: value")),
ErrNetworkData: false,
NetworkDataSecret: newSecret("net-data", map[string]string{"networkData": "key: value"}),
ExpectedUserData: "",
ErrUserData: false,
ExpectedNetworkData: base64.StdEncoding.EncodeToString([]byte("key: value")),
ExpectedPreprovisioningNetworkData: base64.StdEncoding.EncodeToString([]byte("key: value")),
ErrNetworkData: false,
},
{
Scenario: "host with preprov and regular network data",
Expand All @@ -165,17 +168,19 @@ func TestProvisionWithHostConfig(t *testing.T) {
Address: "ipmi://192.168.122.1:6233",
CredentialsName: defaultSecretName,
},
PreprovisioningNetworkDataName: "preprov-net-data",
PreprovisioningNetworkDataName: "net-data2",
NetworkData: &corev1.SecretReference{
Name: "net-data",
Namespace: namespace,
},
}),
NetworkDataSecret: newSecret("net-data", map[string]string{"networkData": "key: value"}),
ExpectedUserData: "",
ErrUserData: false,
ExpectedNetworkData: base64.StdEncoding.EncodeToString([]byte("key: value")),
ErrNetworkData: false,
NetworkDataSecret: newSecret("net-data", map[string]string{"networkData": "key: value"}),
PreprovNetworkDataSecret: newSecret("net-data2", map[string]string{"networkData": "key: value2"}),
ExpectedUserData: "",
ErrUserData: false,
ExpectedNetworkData: base64.StdEncoding.EncodeToString([]byte("key: value")),
ExpectedPreprovisioningNetworkData: base64.StdEncoding.EncodeToString([]byte("key: value2")),
ErrNetworkData: false,
},
{
Scenario: "host with network data only",
Expand Down Expand Up @@ -336,6 +341,7 @@ func TestProvisionWithHostConfig(t *testing.T) {
c.Create(goctx.TODO(), testBMCSecret)
c.Create(goctx.TODO(), tc.UserDataSecret)
c.Create(goctx.TODO(), tc.NetworkDataSecret)
c.Create(goctx.TODO(), tc.PreprovNetworkDataSecret)
baselog := ctrl.Log.WithName("controllers").WithName("BareMetalHost")
hcd := &hostConfigData{
host: tc.Host,
Expand All @@ -358,7 +364,16 @@ func TestProvisionWithHostConfig(t *testing.T) {
}

if actualNetworkData != tc.ExpectedNetworkData {
t.Fatal(fmt.Errorf("Failed to assert NetworkData. Expected '%s' got '%s'", actualNetworkData, tc.ExpectedNetworkData))
t.Fatal(fmt.Errorf("Failed to assert NetworkData. Expected '%s' got '%s'", tc.ExpectedNetworkData, actualNetworkData))
}

actualPreprovisioningNetworkData, err := hcd.PreprovisioningNetworkData()
if err != nil && !tc.ErrPreprovisioningNetworkData {
t.Fatal(err)
}

if actualPreprovisioningNetworkData != tc.ExpectedPreprovisioningNetworkData {
t.Fatal(fmt.Errorf("Failed to assert PreprovisioningNetworkData. Expected '%s' got '%s'", tc.ExpectedPreprovisioningNetworkData, actualPreprovisioningNetworkData))
}

actualMetaData, err := hcd.MetaData()
Expand Down
19 changes: 19 additions & 0 deletions pkg/provisioner/ironic/ironic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,25 @@ func (p *ironicProvisioner) ValidateManagementAccess(data provisioner.Management
// below.
}

// If no PreprovisioningImage builder is enabled we set the Node network_data
// this enables Ironic to inject the network_data into the ramdisk image
if !p.config.havePreprovImgBuilder {
networkDataRaw := data.PreprovisioningNetworkData
if networkDataRaw != "" {
var networkData map[string]interface{}
if yamlErr := yaml.Unmarshal([]byte(networkDataRaw), &networkData); yamlErr != nil {
p.log.Info("failed to unmarshal networkData from PreprovisioningNetworkData")
result, err = transientError(fmt.Errorf("invalid preprovisioningNetworkData: %w", yamlErr))
return
}
numUpdates := len(updater.Updates)
updater.SetTopLevelOpt("network_data", networkData, ironicNode.NetworkData)
if len(updater.Updates) != numUpdates {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe we should change SetTopLevelOpt to return a boolean (updated or not)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I thought the same - I can perhaps look at a follow-up which refactors things to do that, but it seemed outside of the scope of this PR.

p.log.Info("adding preprovisioning network_data for node", "node", ironicNode.UUID)
}
}
}

ironicNode, success, result, err := p.tryUpdateNode(ironicNode, updater)
if !success {
return
Expand Down
13 changes: 7 additions & 6 deletions pkg/provisioner/provisioner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,13 @@ type PreprovisioningImage struct {
}

type ManagementAccessData struct {
BootMode metal3api.BootMode
AutomatedCleaningMode metal3api.AutomatedCleaningMode
State metal3api.ProvisioningState
CurrentImage *metal3api.Image
PreprovisioningImage *PreprovisioningImage
HasCustomDeploy bool
BootMode metal3api.BootMode
AutomatedCleaningMode metal3api.AutomatedCleaningMode
State metal3api.ProvisioningState
CurrentImage *metal3api.Image
PreprovisioningImage *PreprovisioningImage
PreprovisioningNetworkData string
HasCustomDeploy bool
}

type AdoptData struct {
Expand Down