diff --git a/pkg/cloud/baremetal/actuators/machine/actuator.go b/pkg/cloud/baremetal/actuators/machine/actuator.go index 431e134b5..b3973cee4 100644 --- a/pkg/cloud/baremetal/actuators/machine/actuator.go +++ b/pkg/cloud/baremetal/actuators/machine/actuator.go @@ -366,6 +366,28 @@ func consumerRefMatches(consumer *corev1.ObjectReference, machine *machinev1.Mac func (a *Actuator) setHostSpec(ctx context.Context, host *bmh.BareMetalHost, machine *machinev1.Machine, config *bmv1alpha1.BareMetalMachineProviderSpec) error { + // We only want to update the image setting if the host does not + // already have a consumer and image. + // + // A consumer without an image is "externally provisioned" and + // assigning the image will trigger reprovisioning. Since the + // control plane nodes will be configured this way, reprovisioning + // will take them offline and try to turn them into workers. + // + // A host with an existing image is also already provisioned and + // upgrades are not supported at this time. To re-provision a + // host, we must fully deprovision it and then provision it again. + if host.Spec.ConsumerRef == nil && host.Spec.Image == nil { + host.Spec.Image = &bmh.Image{ + URL: config.Image.URL, + Checksum: config.Image.Checksum, + } + host.Spec.UserData = config.UserData + if host.Spec.UserData != nil && host.Spec.UserData.Namespace == "" { + host.Spec.UserData.Namespace = machine.Namespace + } + } + host.Spec.ConsumerRef = &corev1.ObjectReference{ Kind: "Machine", Name: machine.Name, @@ -373,15 +395,7 @@ func (a *Actuator) setHostSpec(ctx context.Context, host *bmh.BareMetalHost, mac APIVersion: machine.APIVersion, } - host.Spec.Image = &bmh.Image{ - URL: config.Image.URL, - Checksum: config.Image.Checksum, - } host.Spec.Online = true - host.Spec.UserData = config.UserData - if host.Spec.UserData != nil && host.Spec.UserData.Namespace == "" { - host.Spec.UserData.Namespace = machine.Namespace - } return a.client.Update(ctx, host) } @@ -503,7 +517,7 @@ func (a *Actuator) nodeAddresses(host *bmh.BareMetalHost) ([]corev1.NodeAddress, if host.Status.HardwareDetails.Hostname != "" { addrs = append(addrs, corev1.NodeAddress{ - Type: corev1.NodeHostName, + Type: corev1.NodeHostName, Address: host.Status.HardwareDetails.Hostname, }) } diff --git a/pkg/cloud/baremetal/actuators/machine/actuator_test.go b/pkg/cloud/baremetal/actuators/machine/actuator_test.go index f4e68fd43..3a161d6dd 100644 --- a/pkg/cloud/baremetal/actuators/machine/actuator_test.go +++ b/pkg/cloud/baremetal/actuators/machine/actuator_test.go @@ -330,102 +330,185 @@ func TestChooseHost(t *testing.T) { func TestSetHostSpec(t *testing.T) { for _, tc := range []struct { + Scenario string UserDataNamespace string ExpectedUserDataNamespace string + Host bmh.BareMetalHost + ExpectedImage *bmh.Image + ExpectUserData bool }{ { + Scenario: "user data has explicit alternate namespace", UserDataNamespace: "otherns", ExpectedUserDataNamespace: "otherns", + Host: bmh.BareMetalHost{ + ObjectMeta: metav1.ObjectMeta{ + Name: "host2", + Namespace: "myns", + }, + }, + ExpectedImage: &bmh.Image{ + URL: testImageURL, + Checksum: testImageChecksumURL, + }, + ExpectUserData: true, }, + { + Scenario: "user data has no namespace", UserDataNamespace: "", ExpectedUserDataNamespace: "myns", + Host: bmh.BareMetalHost{ + ObjectMeta: metav1.ObjectMeta{ + Name: "host2", + Namespace: "myns", + }, + }, + ExpectedImage: &bmh.Image{ + URL: testImageURL, + Checksum: testImageChecksumURL, + }, + ExpectUserData: true, }, - } { - // test data - config, providerSpec := newConfig(t, tc.UserDataNamespace, map[string]string{}, []bmv1alpha1.HostSelectorRequirement{}) - host := bmh.BareMetalHost{ - ObjectMeta: metav1.ObjectMeta{ - Name: "host2", - Namespace: "myns", + { + Scenario: "externally provisioned, same machine", + UserDataNamespace: "", + ExpectedUserDataNamespace: "myns", + Host: bmh.BareMetalHost{ + ObjectMeta: metav1.ObjectMeta{ + Name: "host2", + Namespace: "myns", + }, + Spec: bmh.BareMetalHostSpec{ + ConsumerRef: &corev1.ObjectReference{ + Name: "machine1", + Namespace: "myns", + Kind: "Machine", + APIVersion: machinev1.SchemeGroupVersion.String(), + }, + }, }, - } - machine := machinev1.Machine{ - ObjectMeta: metav1.ObjectMeta{ - Name: "machine1", - Namespace: "myns", + ExpectedImage: nil, + ExpectUserData: false, + }, + + { + Scenario: "previously provisioned, different image, unchanged", + UserDataNamespace: "", + ExpectedUserDataNamespace: "myns", + Host: bmh.BareMetalHost{ + ObjectMeta: metav1.ObjectMeta{ + Name: "host2", + Namespace: "myns", + }, + Spec: bmh.BareMetalHostSpec{ + ConsumerRef: &corev1.ObjectReference{ + Name: "machine1", + Namespace: "myns", + Kind: "Machine", + APIVersion: machinev1.SchemeGroupVersion.String(), + }, + Image: &bmh.Image{ + URL: testImageURL + "test", + Checksum: testImageChecksumURL + "test", + }, + }, }, - Spec: machinev1.MachineSpec{ - ProviderSpec: providerSpec, + ExpectedImage: &bmh.Image{ + URL: testImageURL + "test", + Checksum: testImageChecksumURL + "test", }, - } + ExpectUserData: false, + }, + } { - // test setup - scheme := runtime.NewScheme() - bmoapis.AddToScheme(scheme) - c := fakeclient.NewFakeClientWithScheme(scheme, &host) + t.Run(tc.Scenario, func(t *testing.T) { + // test data + config, providerSpec := newConfig(t, tc.UserDataNamespace, map[string]string{}, []bmv1alpha1.HostSelectorRequirement{}) + machine := machinev1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "machine1", + Namespace: "myns", + }, + Spec: machinev1.MachineSpec{ + ProviderSpec: providerSpec, + }, + } - actuator, err := NewActuator(ActuatorParams{ - Client: c, - }) - if err != nil { - t.Errorf("%v", err) - return - } + // test setup + scheme := runtime.NewScheme() + bmoapis.AddToScheme(scheme) + c := fakeclient.NewFakeClientWithScheme(scheme, &tc.Host) + + actuator, err := NewActuator(ActuatorParams{ + Client: c, + }) + if err != nil { + t.Errorf("%v", err) + return + } - // run the function - err = actuator.setHostSpec(context.TODO(), &host, &machine, config) - if err != nil { - t.Errorf("%v", err) - return - } + // run the function + err = actuator.setHostSpec(context.TODO(), &tc.Host, &machine, config) + if err != nil { + t.Errorf("%v", err) + return + } - // get the saved result - savedHost := bmh.BareMetalHost{} - err = c.Get(context.TODO(), client.ObjectKey{Name: host.Name, Namespace: host.Namespace}, &savedHost) - if err != nil { - t.Errorf("%v", err) - return - } + // get the saved result + savedHost := bmh.BareMetalHost{} + err = c.Get(context.TODO(), client.ObjectKey{Name: tc.Host.Name, Namespace: tc.Host.Namespace}, &savedHost) + if err != nil { + t.Errorf("%v", err) + return + } - // validate the result - if savedHost.Spec.ConsumerRef == nil { - t.Errorf("ConsumerRef not set") - return - } - if savedHost.Spec.ConsumerRef.Name != machine.Name { - t.Errorf("found machine ref %v", savedHost.Spec.ConsumerRef) - } - if savedHost.Spec.ConsumerRef.Namespace != machine.Namespace { - t.Errorf("found machine ref %v", savedHost.Spec.ConsumerRef) - } - if savedHost.Spec.ConsumerRef.Kind != "Machine" { - t.Errorf("found machine ref %v", savedHost.Spec.ConsumerRef) - } - if savedHost.Spec.Online != true { - t.Errorf("host not set to Online") - } - if savedHost.Spec.Image == nil { - t.Errorf("Image not set") - return - } - if savedHost.Spec.Image.URL != testImageURL { - t.Errorf("expected ImageURL %s, got %s", testImageURL, savedHost.Spec.Image.URL) - } - if savedHost.Spec.Image.Checksum != testImageChecksumURL { - t.Errorf("expected ImageChecksumURL %s, got %s", testImageChecksumURL, savedHost.Spec.Image.Checksum) - } - if savedHost.Spec.UserData == nil { - t.Errorf("UserData not set") - return - } - if savedHost.Spec.UserData.Namespace != tc.ExpectedUserDataNamespace { - t.Errorf("expected Userdata.Namespace %s, got %s", tc.ExpectedUserDataNamespace, savedHost.Spec.UserData.Namespace) - } - if savedHost.Spec.UserData.Name != testUserDataSecretName { - t.Errorf("expected Userdata.Name %s, got %s", testUserDataSecretName, savedHost.Spec.UserData.Name) - } + // validate the result + if savedHost.Spec.ConsumerRef == nil { + t.Errorf("ConsumerRef not set") + return + } + if savedHost.Spec.ConsumerRef.Name != machine.Name { + t.Errorf("found machine ref %v", savedHost.Spec.ConsumerRef) + } + if savedHost.Spec.ConsumerRef.Namespace != machine.Namespace { + t.Errorf("found machine ref %v", savedHost.Spec.ConsumerRef) + } + if savedHost.Spec.ConsumerRef.Kind != "Machine" { + t.Errorf("found machine ref %v", savedHost.Spec.ConsumerRef) + } + if savedHost.Spec.Online != true { + t.Errorf("host not set to Online") + } + if tc.ExpectedImage == nil { + if savedHost.Spec.Image != nil { + t.Errorf("Expected image %v but got %v", tc.ExpectedImage, savedHost.Spec.Image) + return + } + } else { + if *(savedHost.Spec.Image) != *(tc.ExpectedImage) { + t.Errorf("Expected image %v but got %v", tc.ExpectedImage, savedHost.Spec.Image) + return + } + } + if tc.ExpectUserData { + if savedHost.Spec.UserData == nil { + t.Errorf("UserData not set") + return + } + if savedHost.Spec.UserData.Namespace != tc.ExpectedUserDataNamespace { + t.Errorf("expected Userdata.Namespace %s, got %s", tc.ExpectedUserDataNamespace, savedHost.Spec.UserData.Namespace) + } + if savedHost.Spec.UserData.Name != testUserDataSecretName { + t.Errorf("expected Userdata.Name %s, got %s", testUserDataSecretName, savedHost.Spec.UserData.Name) + } + } else { + if savedHost.Spec.UserData != nil { + t.Errorf("did not expect user data, got %v", savedHost.Spec.UserData) + } + } + }) } }