Skip to content

Commit

Permalink
🌱 Refactor controller tests and fake HCloud client
Browse files Browse the repository at this point in the history 
Refactoring remaining controller tests as well as fixing some bugs in
HCLoud client that caused instability of the tests.

Namely there was a problem in generating IDs, that let to a specific situation
in which we overwrite saved HCloud objects with new objects.
  • Loading branch information
@janiskemper
janiskemper committed Sep 13, 2023
1 parent 0fb5184 commit 325b708
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 111 deletions.
44 changes: 21 additions & 23 deletions controllers/hcloudmachinetemplate_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package controllers

import (
"fmt"
"time"

. "github.com/onsi/ginkgo/v2"
Expand All @@ -34,9 +33,9 @@ import (

var _ = Describe("HCloudMachineTemplateReconciler", func() {
var (
capiCluster *clusterv1.Cluster
infraCluster *infrav1.HetznerCluster
capiCluster *clusterv1.Cluster

hetznerCluster *infrav1.HetznerCluster
machineTemplate *infrav1.HCloudMachineTemplate

testNs *corev1.Namespace
Expand All @@ -53,27 +52,25 @@ var _ = Describe("HCloudMachineTemplateReconciler", func() {

capiCluster = &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
GenerateName: "test1-",
GenerateName: "test-",
Namespace: testNs.Name,
Finalizers: []string{clusterv1.ClusterFinalizer},
},
Spec: clusterv1.ClusterSpec{
InfrastructureRef: &corev1.ObjectReference{
APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1",
Kind: "HetznerCluster",
Name: "hetzner-test1",
Name: "hetzner-test",
Namespace: testNs.Name,
},
},
Status: clusterv1.ClusterStatus{
InfrastructureReady: true,
},
}

Expect(testEnv.Create(ctx, capiCluster)).To(Succeed())

infraCluster = &infrav1.HetznerCluster{
hetznerCluster = &infrav1.HetznerCluster{
ObjectMeta: metav1.ObjectMeta{
Name: "hetzner-test1",
Name: "hetzner-test",
Namespace: testNs.Name,
OwnerReferences: []metav1.OwnerReference{
{
Expand All @@ -87,13 +84,13 @@ var _ = Describe("HCloudMachineTemplateReconciler", func() {
Spec: getDefaultHetznerClusterSpec(),
}

Expect(testEnv.Create(ctx, infraCluster)).To(Succeed())
Expect(testEnv.Create(ctx, hetznerCluster)).To(Succeed())

machineTemplate = &infrav1.HCloudMachineTemplate{
ObjectMeta: metav1.ObjectMeta{
GenerateName: "hcloud-machine-template-",
Namespace: testNs.Name,
OwnerReferences: infraCluster.OwnerReferences,
OwnerReferences: hetznerCluster.OwnerReferences,
},
Spec: infrav1.HCloudMachineTemplateSpec{
Template: infrav1.HCloudMachineTemplateResource{
Expand All @@ -114,43 +111,44 @@ var _ = Describe("HCloudMachineTemplateReconciler", func() {
})

AfterEach(func() {
Expect(testEnv.Cleanup(ctx, testNs, capiCluster, infraCluster,
Expect(testEnv.Cleanup(ctx, testNs, capiCluster, hetznerCluster,
machineTemplate, hetznerSecret)).To(Succeed())
})

Context("Basic test", func() {
It("sets the capacity in status", func() {
Eventually(func() bool {
if err := testEnv.Get(ctx, key, machineTemplate); err != nil {
fmt.Printf("Did not find machine template: %s\n", err)
testEnv.GetLogger().Error(err, "did not find machine template")
return false
}

fmt.Printf("Capacity: %v\n", machineTemplate.Status.Capacity)

// If capacity is not set (yet), there is nothing to compare
if machineTemplate.Status.Capacity.Cpu() == nil || machineTemplate.Status.Capacity.Memory() == nil {
fmt.Printf("Capacity not set: %v\n", machineTemplate.Status.Capacity)
testEnv.GetLogger().Info("capacity not set", "capacity", machineTemplate.Status.Capacity)
return false
}

// Compare CPU
// compare CPU
expectedCPU, err := machinetemplate.GetCPUQuantityFromInt(fake.DefaultCPUCores)
Expect(err).To(Succeed())

if !expectedCPU.Equal(*machineTemplate.Status.Capacity.Cpu()) {
fmt.Printf("CPU did not equal: Expected: %v. Actual: %v\n", expectedCPU, machineTemplate.Status.Capacity.Cpu())
testEnv.GetLogger().Info("cpu did not equal", "expected", expectedCPU, "actual", machineTemplate.Status.Capacity.Cpu())
return false
}

// Compare memory
// compare memory
expectedMemory, err := machinetemplate.GetMemoryQuantityFromFloat32(fake.DefaultMemoryInGB)
Expect(err).To(Succeed())

if !expectedMemory.Equal(*machineTemplate.Status.Capacity.Memory()) {
fmt.Printf("Memory did not equal: Expected: %v. Actual: %v\n", expectedMemory, machineTemplate.Status.Capacity.Memory())
testEnv.GetLogger().Info("memory did not equal", "expected", expectedMemory, "actual", machineTemplate.Status.Capacity.Memory())
return false
}
return expectedMemory.Equal(*machineTemplate.Status.Capacity.Memory())
}, 10*time.Second, time.Second).Should(BeTrue())

return true
}, timeout, time.Second).Should(BeTrue())
})
})
})
83 changes: 17 additions & 66 deletions controllers/hetznerbaremetalhost_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/stretchr/testify/mock"
"github.com/syself/hrobot-go/models"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/utils/pointer"
Expand Down Expand Up @@ -59,8 +60,9 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
host *infrav1.HetznerBareMetalHost
bmMachine *infrav1.HetznerBareMetalMachine
hetznerCluster *infrav1.HetznerCluster
capiCluster *clusterv1.Cluster
capiMachine *clusterv1.Machine

capiCluster *clusterv1.Cluster
capiMachine *clusterv1.Machine

hetznerClusterName string

Expand Down Expand Up @@ -100,15 +102,6 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
Namespace: testNs.Name,
},
},
Status: clusterv1.ClusterStatus{
InfrastructureReady: true,
Conditions: clusterv1.Conditions{
{
Reason: "reason",
Message: "message",
},
},
},
}
Expect(testEnv.Create(ctx, capiCluster)).To(Succeed())

Expand Down Expand Up @@ -164,6 +157,7 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
robotClient.On("DeleteBootRescue", 1).Return(&models.Rescue{Active: false}, nil)
robotClient.On("RebootBMServer", mock.Anything, mock.Anything).Return(&models.ResetPost{}, nil)
robotClient.On("SetBMServerName", 1, mock.Anything).Return(nil, nil)

configureRescueSSHClient(rescueSSHClient)

osSSHClientAfterInstallImage.On("Reboot").Return(sshclient.Output{})
Expand Down Expand Up @@ -209,21 +203,17 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
})

It("creates the host machine", func() {
defer func() {
Expect(testEnv.Delete(ctx, host)).To(Succeed())
}()
Eventually(func() bool {
if err := testEnv.Get(ctx, key, host); err != nil {
return false
}
return true
}, timeout).Should(BeTrue())

Expect(testEnv.Delete(ctx, host)).To(Succeed())
})

It("sets the finalizer", func() {
defer func() {
Expect(testEnv.Delete(ctx, host)).To(Succeed())
}()
Eventually(func() bool {
if err := testEnv.Get(ctx, key, host); err != nil {
return false
Expand All @@ -235,18 +225,17 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
}
return false
}, timeout).Should(BeTrue())

Expect(testEnv.Delete(ctx, host)).To(Succeed())
})

It("deletes successfully", func() {
// Delete the host object
By("deleting the host object")
Expect(testEnv.Delete(ctx, host)).To(Succeed())

// Make sure the it has been deleted
By("making sure the it has been deleted")
Eventually(func() bool {
if err := testEnv.Get(ctx, key, host); err != nil {
return true
}
return false
return apierrors.IsNotFound(testEnv.Get(ctx, key, host))
}, timeout, time.Second).Should(BeTrue())
})
})
Expand Down Expand Up @@ -330,10 +319,14 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
It("reaches the state image installing", func() {
ph, err := patch.NewHelper(host, testEnv)
Expect(err).ShouldNot(HaveOccurred())

host.Spec.RootDeviceHints = &infrav1.RootDeviceHints{
WWN: helpers.DefaultWWN,
}
Expect(ph.Patch(ctx, host, patch.WithStatusObservedGeneration{})).To(Succeed())

Eventually(func() error {
return ph.Patch(ctx, host, patch.WithStatusObservedGeneration{})
}, timeout, time.Second).Should(BeNil())

Eventually(func() bool {
if err := testEnv.Get(ctx, key, host); err != nil {
Expand Down Expand Up @@ -365,13 +358,6 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
})

It("gets selected from a bm machine and provisions", func() {
defer func() {
Expect(testEnv.Delete(ctx, bmMachine))
}()

var machine clusterv1.Machine
Expect(testEnv.Get(ctx, client.ObjectKey{Namespace: testNs.Name, Name: capiMachine.Name}, &machine)).To(Succeed())

Eventually(func() bool {
if err := testEnv.Get(ctx, key, host); err != nil {
return false
Expand Down Expand Up @@ -403,13 +389,6 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
})

It("gets selected from a bm machine and provisions", func() {
defer func() {
Expect(testEnv.Delete(ctx, bmMachine))
}()

var machine clusterv1.Machine
Expect(testEnv.Get(ctx, client.ObjectKey{Namespace: testNs.Name, Name: capiMachine.Name}, &machine)).To(Succeed())

Eventually(func() bool {
if err := testEnv.Get(ctx, key, host); err != nil {
return false
Expand Down Expand Up @@ -530,13 +509,6 @@ var _ = Describe("HetznerBareMetalHostReconciler", func() {
})

It("gets selected from a bm machine and provisions", func() {
defer func() {
Expect(testEnv.Delete(ctx, bmMachine))
}()

var machine clusterv1.Machine
Expect(testEnv.Get(ctx, client.ObjectKey{Namespace: testNs.Name, Name: capiMachine.Name}, &machine)).To(Succeed())

Eventually(func() bool {
if err := testEnv.Get(ctx, key, host); err != nil {
return false
Expand Down Expand Up @@ -595,15 +567,6 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {
Namespace: testNs.Name,
},
},
Status: clusterv1.ClusterStatus{
InfrastructureReady: true,
Conditions: clusterv1.Conditions{
{
Reason: "reason",
Message: "message",
},
},
},
}
Expect(testEnv.Create(ctx, capiCluster)).To(Succeed())

Expand Down Expand Up @@ -722,9 +685,6 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {

It("gives an error", func() {
Eventually(func() bool {
if err := testEnv.Get(ctx, key, host); err != nil {
return false
}
return isPresentAndFalseWithReason(key, host, infrav1.CredentialsAvailableCondition, infrav1.RescueSSHSecretMissingReason)
}, timeout).Should(BeTrue())
})
Expand All @@ -747,9 +707,6 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {
}()

Eventually(func() bool {
if err := testEnv.Get(ctx, key, host); err != nil {
return false
}
return isPresentAndFalseWithReason(key, host, infrav1.CredentialsAvailableCondition, infrav1.SSHCredentialsInSecretInvalidReason)
}, timeout).Should(BeTrue())
})
Expand Down Expand Up @@ -780,9 +737,6 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {

It("gives the right error if secret is missing", func() {
Eventually(func() bool {
if err := testEnv.Get(ctx, key, host); err != nil {
return false
}
return isPresentAndFalseWithReason(key, host, infrav1.CredentialsAvailableCondition, infrav1.OSSSHSecretMissingReason)
}, timeout).Should(BeTrue())
})
Expand All @@ -805,9 +759,6 @@ var _ = Describe("HetznerBareMetalHostReconciler - missing secrets", func() {
}()

Eventually(func() bool {
if err := testEnv.Get(ctx, key, host); err != nil {
return false
}
return isPresentAndFalseWithReason(key, host, infrav1.CredentialsAvailableCondition, infrav1.SSHCredentialsInSecretInvalidReason)
}, timeout).Should(BeTrue())
})
Expand Down
Loading

0 comments on commit 325b708

Please sign in to comment.