From 66f5599c272b1ad865d0aaedb56210e59ee65aa8 Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Mon, 1 Jul 2019 13:17:25 -0400 Subject: [PATCH 1/4] gofmt fix --- pkg/cloud/baremetal/actuators/machine/actuator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cloud/baremetal/actuators/machine/actuator.go b/pkg/cloud/baremetal/actuators/machine/actuator.go index 431e134b5..282c75381 100644 --- a/pkg/cloud/baremetal/actuators/machine/actuator.go +++ b/pkg/cloud/baremetal/actuators/machine/actuator.go @@ -503,7 +503,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, }) } From 0281344696a20cf1a559e1729153b3cba6402249 Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Mon, 1 Jul 2019 11:53:44 -0400 Subject: [PATCH 2/4] refactor actuator setHostSpec tests to make it easier to add new cases --- .../actuators/machine/actuator_test.go | 79 ++++++++++++------- 1 file changed, 52 insertions(+), 27 deletions(-) diff --git a/pkg/cloud/baremetal/actuators/machine/actuator_test.go b/pkg/cloud/baremetal/actuators/machine/actuator_test.go index f4e68fd43..a408082f3 100644 --- a/pkg/cloud/baremetal/actuators/machine/actuator_test.go +++ b/pkg/cloud/baremetal/actuators/machine/actuator_test.go @@ -332,25 +332,45 @@ func TestSetHostSpec(t *testing.T) { for _, tc := range []struct { UserDataNamespace string ExpectedUserDataNamespace string + Host bmh.BareMetalHost + ExpectedImage *bmh.Image + ExpectUserData bool }{ { UserDataNamespace: "otherns", ExpectedUserDataNamespace: "otherns", + Host: bmh.BareMetalHost{ + ObjectMeta: metav1.ObjectMeta{ + Name: "host2", + Namespace: "myns", + }, + }, + ExpectedImage: &bmh.Image{ + URL: testImageURL, + Checksum: testImageChecksumURL, + }, + ExpectUserData: true, }, + { 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", - }, - } machine := machinev1.Machine{ ObjectMeta: metav1.ObjectMeta{ Name: "machine1", @@ -364,7 +384,7 @@ func TestSetHostSpec(t *testing.T) { // test setup scheme := runtime.NewScheme() bmoapis.AddToScheme(scheme) - c := fakeclient.NewFakeClientWithScheme(scheme, &host) + c := fakeclient.NewFakeClientWithScheme(scheme, &tc.Host) actuator, err := NewActuator(ActuatorParams{ Client: c, @@ -375,7 +395,7 @@ func TestSetHostSpec(t *testing.T) { } // run the function - err = actuator.setHostSpec(context.TODO(), &host, &machine, config) + err = actuator.setHostSpec(context.TODO(), &tc.Host, &machine, config) if err != nil { t.Errorf("%v", err) return @@ -383,7 +403,7 @@ func TestSetHostSpec(t *testing.T) { // get the saved result savedHost := bmh.BareMetalHost{} - err = c.Get(context.TODO(), client.ObjectKey{Name: host.Name, Namespace: host.Namespace}, &savedHost) + err = c.Get(context.TODO(), client.ObjectKey{Name: tc.Host.Name, Namespace: tc.Host.Namespace}, &savedHost) if err != nil { t.Errorf("%v", err) return @@ -406,25 +426,30 @@ func TestSetHostSpec(t *testing.T) { 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 tc.ExpectedImage == nil { + if savedHost.Spec.Image != nil { + t.Fatalf("Expected image %v but got %v", tc.ExpectedImage, savedHost.Spec.Image) + } + } else { + if *(savedHost.Spec.Image) != *(tc.ExpectedImage) { + t.Fatalf("Expected image %v but got %v", tc.ExpectedImage, savedHost.Spec.Image) + } } - if savedHost.Spec.UserData.Name != testUserDataSecretName { - t.Errorf("expected Userdata.Name %s, got %s", testUserDataSecretName, savedHost.Spec.UserData.Name) + 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) + } } } } From 5765cad418f6b9faf109ab7336960c77348df8e2 Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Mon, 1 Jul 2019 12:55:03 -0400 Subject: [PATCH 3/4] use named scenarios and subtests in setHostSpec tests Name the scenarios and use subtests to make it easier to see which output belongs with which failure. --- .../actuators/machine/actuator_test.go | 149 +++++++++--------- 1 file changed, 78 insertions(+), 71 deletions(-) diff --git a/pkg/cloud/baremetal/actuators/machine/actuator_test.go b/pkg/cloud/baremetal/actuators/machine/actuator_test.go index a408082f3..9760b8a3b 100644 --- a/pkg/cloud/baremetal/actuators/machine/actuator_test.go +++ b/pkg/cloud/baremetal/actuators/machine/actuator_test.go @@ -330,6 +330,7 @@ func TestChooseHost(t *testing.T) { func TestSetHostSpec(t *testing.T) { for _, tc := range []struct { + Scenario string UserDataNamespace string ExpectedUserDataNamespace string Host bmh.BareMetalHost @@ -337,6 +338,7 @@ func TestSetHostSpec(t *testing.T) { ExpectUserData bool }{ { + Scenario: "user data has explicit alternate namespace", UserDataNamespace: "otherns", ExpectedUserDataNamespace: "otherns", Host: bmh.BareMetalHost{ @@ -353,6 +355,7 @@ func TestSetHostSpec(t *testing.T) { }, { + Scenario: "user data has no namespace", UserDataNamespace: "", ExpectedUserDataNamespace: "myns", Host: bmh.BareMetalHost{ @@ -369,88 +372,92 @@ func TestSetHostSpec(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, - }, - } + 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, + }, + } - // test setup - scheme := runtime.NewScheme() - bmoapis.AddToScheme(scheme) - c := fakeclient.NewFakeClientWithScheme(scheme, &tc.Host) + // 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 - } + actuator, err := NewActuator(ActuatorParams{ + Client: c, + }) + 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 - } + // 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: tc.Host.Name, Namespace: tc.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 tc.ExpectedImage == nil { - if savedHost.Spec.Image != nil { - t.Fatalf("Expected image %v but got %v", tc.ExpectedImage, savedHost.Spec.Image) + // validate the result + if savedHost.Spec.ConsumerRef == nil { + t.Errorf("ConsumerRef not set") + return } - } else { - if *(savedHost.Spec.Image) != *(tc.ExpectedImage) { - t.Fatalf("Expected image %v but got %v", tc.ExpectedImage, savedHost.Spec.Image) + if savedHost.Spec.ConsumerRef.Name != machine.Name { + t.Errorf("found machine ref %v", savedHost.Spec.ConsumerRef) } - } - if tc.ExpectUserData { - if savedHost.Spec.UserData == nil { - t.Errorf("UserData not set") - return + if savedHost.Spec.ConsumerRef.Namespace != machine.Namespace { + t.Errorf("found machine ref %v", savedHost.Spec.ConsumerRef) } - if savedHost.Spec.UserData.Namespace != tc.ExpectedUserDataNamespace { - t.Errorf("expected Userdata.Namespace %s, got %s", tc.ExpectedUserDataNamespace, savedHost.Spec.UserData.Namespace) + if savedHost.Spec.ConsumerRef.Kind != "Machine" { + t.Errorf("found machine ref %v", savedHost.Spec.ConsumerRef) } - if savedHost.Spec.UserData.Name != testUserDataSecretName { - t.Errorf("expected Userdata.Name %s, got %s", testUserDataSecretName, savedHost.Spec.UserData.Name) + if savedHost.Spec.Online != true { + t.Errorf("host not set to Online") } - } else { - if savedHost.Spec.UserData != nil { - t.Errorf("did not expect user data, got %v", savedHost.Spec.UserData) + 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) + } + } + }) } } From 899fdcf31ce4dd7800d27f5f4c071e456aa2189d Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Mon, 1 Jul 2019 13:08:56 -0400 Subject: [PATCH 4/4] only set image and user data when we actually want to provision 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. Fixes #93 --- .../baremetal/actuators/machine/actuator.go | 30 ++++++++--- .../actuators/machine/actuator_test.go | 51 +++++++++++++++++++ 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/pkg/cloud/baremetal/actuators/machine/actuator.go b/pkg/cloud/baremetal/actuators/machine/actuator.go index 282c75381..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) } diff --git a/pkg/cloud/baremetal/actuators/machine/actuator_test.go b/pkg/cloud/baremetal/actuators/machine/actuator_test.go index 9760b8a3b..3a161d6dd 100644 --- a/pkg/cloud/baremetal/actuators/machine/actuator_test.go +++ b/pkg/cloud/baremetal/actuators/machine/actuator_test.go @@ -370,6 +370,57 @@ func TestSetHostSpec(t *testing.T) { }, ExpectUserData: true, }, + + { + 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(), + }, + }, + }, + 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", + }, + }, + }, + ExpectedImage: &bmh.Image{ + URL: testImageURL + "test", + Checksum: testImageChecksumURL + "test", + }, + ExpectUserData: false, + }, } { t.Run(tc.Scenario, func(t *testing.T) {