From 325b708ad2010f155cc9036b738bfe3f6d3959a0 Mon Sep 17 00:00:00 2001 From: janiskemper Date: Wed, 13 Sep 2023 13:17:14 +0200 Subject: [PATCH] :seedling: Refactor controller tests and fake HCloud client 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. --- .../hcloudmachinetemplate_controller_test.go | 44 +++++----- .../hetznerbaremetalhost_controller_test.go | 83 ++++--------------- controllers/hetznercluster_controller_test.go | 36 +++++--- .../hcloud/client/fake/hcloud_client.go | 51 +++++++++--- 4 files changed, 103 insertions(+), 111 deletions(-) diff --git a/controllers/hcloudmachinetemplate_controller_test.go b/controllers/hcloudmachinetemplate_controller_test.go index 3553de56e..d383942b9 100644 --- a/controllers/hcloudmachinetemplate_controller_test.go +++ b/controllers/hcloudmachinetemplate_controller_test.go @@ -17,7 +17,6 @@ limitations under the License. package controllers import ( - "fmt" "time" . "github.com/onsi/ginkgo/v2" @@ -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 @@ -53,7 +52,7 @@ var _ = Describe("HCloudMachineTemplateReconciler", func() { capiCluster = &clusterv1.Cluster{ ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test1-", + GenerateName: "test-", Namespace: testNs.Name, Finalizers: []string{clusterv1.ClusterFinalizer}, }, @@ -61,19 +60,17 @@ var _ = Describe("HCloudMachineTemplateReconciler", func() { 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{ { @@ -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{ @@ -114,7 +111,7 @@ var _ = Describe("HCloudMachineTemplateReconciler", func() { }) AfterEach(func() { - Expect(testEnv.Cleanup(ctx, testNs, capiCluster, infraCluster, + Expect(testEnv.Cleanup(ctx, testNs, capiCluster, hetznerCluster, machineTemplate, hetznerSecret)).To(Succeed()) }) @@ -122,35 +119,36 @@ var _ = Describe("HCloudMachineTemplateReconciler", 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()) }) }) }) diff --git a/controllers/hetznerbaremetalhost_controller_test.go b/controllers/hetznerbaremetalhost_controller_test.go index dd05e6832..2ac03a401 100644 --- a/controllers/hetznerbaremetalhost_controller_test.go +++ b/controllers/hetznerbaremetalhost_controller_test.go @@ -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" @@ -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 @@ -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()) @@ -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{}) @@ -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 @@ -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()) }) }) @@ -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 { @@ -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 @@ -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 @@ -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 @@ -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()) @@ -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()) }) @@ -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()) }) @@ -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()) }) @@ -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()) }) diff --git a/controllers/hetznercluster_controller_test.go b/controllers/hetznercluster_controller_test.go index 2b3255c70..7b31b15f6 100644 --- a/controllers/hetznercluster_controller_test.go +++ b/controllers/hetznercluster_controller_test.go @@ -18,7 +18,6 @@ package controllers import ( "context" - "fmt" "time" "github.com/hetznercloud/hcloud-go/hcloud" @@ -54,7 +53,7 @@ var _ = Describe("Hetzner ClusterReconciler", func() { hetznerClusterName string ) BeforeEach(func() { - testNs, err = testEnv.CreateNamespace(ctx, "lb-attachment") + testNs, err = testEnv.CreateNamespace(ctx, "cluster-tests") Expect(err).NotTo(HaveOccurred()) namespace = testNs.Name @@ -124,10 +123,11 @@ var _ = Describe("Hetzner ClusterReconciler", func() { return isPresentAndTrue(key, instance, infrav1.LoadBalancerReadyCondition) }, timeout, time.Second).Should(BeTrue()) - By("updating load balancer specs") newLBName := "new-lb-name" newLBType := "lb31" + By("updating load balancer type") + ph, err := patch.NewHelper(instance, testEnv) Expect(err).ShouldNot(HaveOccurred()) @@ -137,6 +137,8 @@ var _ = Describe("Hetzner ClusterReconciler", func() { return ph.Patch(ctx, instance, patch.WithStatusObservedGeneration{}) }, timeout).Should(BeNil()) + By("updating load balancer name") + ph, err = patch.NewHelper(instance, testEnv) Expect(err).ShouldNot(HaveOccurred()) @@ -146,32 +148,41 @@ var _ = Describe("Hetzner ClusterReconciler", func() { return ph.Patch(ctx, instance, patch.WithStatusObservedGeneration{}) }, timeout).Should(BeNil()) + By("listing load balancers and checking spec") + // Check in hetzner API - Eventually(func() error { + Eventually(func() bool { loadBalancers, err := hcloudClient.ListLoadBalancers(ctx, hcloud.LoadBalancerListOpts{ ListOpts: hcloud.ListOpts{ LabelSelector: utils.LabelsToLabelSelector(map[string]string{instance.ClusterTagKey(): "owned"}), }, }) if err != nil { - return fmt.Errorf("failed to list load balancers: %w", err) + testEnv.GetLogger().Info("failed to list load balancers", "err", err) + return false } if len(loadBalancers) > 1 { - return fmt.Errorf("there are multiple load balancers found: %v", loadBalancers) + testEnv.GetLogger().Info("there are multiple load balancers found", "number of load balancers", loadBalancers) + return false } if len(loadBalancers) == 0 { - return fmt.Errorf("no load balancer found") + testEnv.GetLogger().Info("no load balancer found") + return false } + lb := loadBalancers[0] if lb.Name != newLBName { - return fmt.Errorf("wrong name. Want %s, got %s", newLBName, lb.Name) + testEnv.GetLogger().Info("wrong name", "want", newLBName, "got", lb.Name) + return false } if lb.LoadBalancerType.Name != newLBType { - return fmt.Errorf("wrong name. Want %s, got %s", newLBType, lb.LoadBalancerType.Name) + testEnv.GetLogger().Info("wrong type", "want", newLBType, "got", lb.LoadBalancerType.Name) + return false } - return nil - }, timeout).Should(BeNil()) + + return true + }, timeout, 1*time.Second).Should(BeTrue()) }) It("should update extra targets", func() { @@ -185,6 +196,7 @@ var _ = Describe("Hetzner ClusterReconciler", func() { ph, err := patch.NewHelper(instance, testEnv) Expect(err).ShouldNot(HaveOccurred()) + instance.Spec.ControlPlaneLoadBalancer.ExtraServices = append(instance.Spec.ControlPlaneLoadBalancer.ExtraServices, infrav1.LoadBalancerServiceSpec{ DestinationPort: 8134, @@ -486,7 +498,7 @@ var _ = Describe("Hetzner ClusterReconciler", func() { return -1 } return len(pgs) - }, timeout).Should(Equal(len(newPlacementGroupSpec))) + }, timeout, time.Second).Should(Equal(len(newPlacementGroupSpec))) }, Entry("one pg", []infrav1.HCloudPlacementGroupSpec{{Name: "md-0", Type: "spread"}}), Entry("no pgs", []infrav1.HCloudPlacementGroupSpec{}), diff --git a/pkg/services/hcloud/client/fake/hcloud_client.go b/pkg/services/hcloud/client/fake/hcloud_client.go index af0f18f80..67dd5db11 100644 --- a/pkg/services/hcloud/client/fake/hcloud_client.go +++ b/pkg/services/hcloud/client/fake/hcloud_client.go @@ -21,6 +21,7 @@ import ( "context" "fmt" "net" + "sync" "github.com/hetznercloud/hcloud-go/hcloud" @@ -38,10 +39,15 @@ const DefaultMemoryInGB = float32(4) const DefaultArchitecture = hcloud.ArchitectureX86 type cacheHCloudClient struct { - serverCache serverCache - placementGroupCache placementGroupCache - loadBalancerCache loadBalancerCache - networkCache networkCache + serverCache serverCache + placementGroupCache placementGroupCache + loadBalancerCache loadBalancerCache + networkCache networkCache + counterMutex sync.Mutex + serverIDCounter int + placementGroupIDCounter int + loadBalancerIDCounter int + networkIDCounter int } // NewClient gives reference to the fake client using cache for HCloud API. @@ -51,6 +57,9 @@ func (f *cacheHCloudClientFactory) NewClient(string) hcloudclient.Client { // Close implements Close method of hcloud client interface. func (c *cacheHCloudClient) Close() { + c.counterMutex.Lock() + defer c.counterMutex.Unlock() + cacheHCloudClientInstance.serverCache = serverCache{} cacheHCloudClientInstance.networkCache = networkCache{} cacheHCloudClientInstance.loadBalancerCache = loadBalancerCache{} @@ -72,6 +81,11 @@ func (c *cacheHCloudClient) Close() { idMap: make(map[int]*hcloud.Network), nameMap: make(map[string]struct{}), } + + cacheHCloudClientInstance.serverIDCounter = 0 + cacheHCloudClientInstance.placementGroupIDCounter = 0 + cacheHCloudClientInstance.loadBalancerIDCounter = 0 + cacheHCloudClientInstance.networkIDCounter = 0 } type cacheHCloudClientFactory struct{} @@ -134,13 +148,17 @@ var defaultImage = hcloud.Image{ } func (c *cacheHCloudClient) CreateLoadBalancer(_ context.Context, opts hcloud.LoadBalancerCreateOpts) (*hcloud.LoadBalancer, error) { + c.counterMutex.Lock() + defer c.counterMutex.Unlock() + // cannot have two load balancers with the same name if _, found := c.loadBalancerCache.nameMap[opts.Name]; found { return nil, fmt.Errorf("failed to create lb: already exists") } + c.loadBalancerIDCounter++ lb := &hcloud.LoadBalancer{ - ID: len(c.loadBalancerCache.idMap) + 1, + ID: c.loadBalancerIDCounter, Name: opts.Name, Labels: opts.Labels, Algorithm: *opts.Algorithm, @@ -398,12 +416,16 @@ func (c *cacheHCloudClient) ListImages(_ context.Context, opts hcloud.ImageListO } func (c *cacheHCloudClient) CreateServer(_ context.Context, opts hcloud.ServerCreateOpts) (*hcloud.Server, error) { + c.counterMutex.Lock() + defer c.counterMutex.Unlock() + if _, found := c.serverCache.nameMap[opts.Name]; found { return nil, fmt.Errorf("already exists") } + c.serverIDCounter++ server := &hcloud.Server{ - ID: len(c.serverCache.idMap) + 1, + ID: c.serverIDCounter, Name: opts.Name, Labels: opts.Labels, Image: opts.Image, @@ -436,7 +458,7 @@ func (c *cacheHCloudClient) AttachServerToNetwork(_ context.Context, server *hcl // check if already exists for _, s := range c.networkCache.idMap[opts.Network.ID].Servers { if s.ID == server.ID { - return hcloud.Error{Code: hcloud.ErrorCodeServerAlreadyAdded, Message: "already added"} + return hcloud.Error{Code: hcloud.ErrorCodeServerAlreadyAttached, Message: "already attached"} } } @@ -552,12 +574,16 @@ func (c *cacheHCloudClient) GetServerType(_ context.Context, name string) (*hclo } func (c *cacheHCloudClient) CreateNetwork(_ context.Context, opts hcloud.NetworkCreateOpts) (*hcloud.Network, error) { + c.counterMutex.Lock() + defer c.counterMutex.Unlock() + if _, found := c.networkCache.nameMap[opts.Name]; found { return nil, fmt.Errorf("already exists") } + c.networkIDCounter++ network := &hcloud.Network{ - ID: len(c.networkCache.idMap) + 1, + ID: c.networkIDCounter, Name: opts.Name, Labels: opts.Labels, IPRange: opts.IPRange, @@ -609,12 +635,16 @@ func (c *cacheHCloudClient) ListSSHKeys(_ context.Context, _ hcloud.SSHKeyListOp } func (c *cacheHCloudClient) CreatePlacementGroup(_ context.Context, opts hcloud.PlacementGroupCreateOpts) (*hcloud.PlacementGroup, error) { + c.counterMutex.Lock() + defer c.counterMutex.Unlock() + if _, found := c.placementGroupCache.nameMap[opts.Name]; found { return nil, fmt.Errorf("already exists") } + c.placementGroupIDCounter++ placementGroup := &hcloud.PlacementGroup{ - ID: len(c.placementGroupCache.idMap) + 1, + ID: c.placementGroupIDCounter, Name: opts.Name, Labels: opts.Labels, Type: opts.Type, @@ -623,7 +653,6 @@ func (c *cacheHCloudClient) CreatePlacementGroup(_ context.Context, opts hcloud. // Add placementGroup to cache c.placementGroupCache.idMap[placementGroup.ID] = placementGroup c.placementGroupCache.nameMap[placementGroup.Name] = struct{}{} - return placementGroup, nil } @@ -631,7 +660,9 @@ func (c *cacheHCloudClient) DeletePlacementGroup(_ context.Context, id int) erro if _, found := c.placementGroupCache.idMap[id]; !found { return hcloud.Error{Code: hcloud.ErrorCodeNotFound, Message: "not found"} } + n := c.placementGroupCache.idMap[id] + delete(c.placementGroupCache.nameMap, n.Name) delete(c.placementGroupCache.idMap, id) return nil