Skip to content

Commit 4e4ae5b

Browse files
EmilienMEmilienM
committed
Add unit tests and cleanup
1 parent 9b92287 commit 4e4ae5b

File tree

3 files changed

+187
-7
lines changed

3 files changed

+187
-7
lines changed

‎api/v1beta1/conditions_consts.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,9 @@ const (
4444
OpenStackErrorReason = "OpenStackError"
4545
// DependencyFailedReason indicates that a dependent object failed.
4646
DependencyFailedReason = "DependencyFailed"
47+
48+
// ServerUnexpectedDeletedMessage is the message used when the server is unexpectedly deleted via an external agent.
49+
ServerUnexpectedDeletedMessage = "The server was unexpectedly deleted"
4750
)
4851

4952
const (

‎controllers/openstackserver_controller.go

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -439,9 +439,9 @@ func (r *OpenStackServerReconciler) getOrCreateServer(ctx context.Context, logge
439439
if err != nil {
440440
msg = err.Error()
441441
} else {
442-
msg = "server has been unexpectedly deleted"
442+
msg = infrav1.ServerUnexpectedDeletedMessage
443443
}
444-
conditions.MarkFalse(openStackServer, infrav1.InstanceReadyCondition, infrav1.OpenStackErrorReason, clusterv1.ConditionSeverityError, "%s", msg)
444+
conditions.MarkFalse(openStackServer, infrav1.InstanceReadyCondition, infrav1.InstanceNotFoundReason, clusterv1.ConditionSeverityError, msg)
445445
return nil, err
446446
}
447447
}
@@ -456,11 +456,6 @@ func (r *OpenStackServerReconciler) getOrCreateServer(ctx context.Context, logge
456456
logger.Info("Server already exists", "name", openStackServer.Name, "id", instanceStatus.ID())
457457
return instanceStatus, nil
458458
}
459-
if openStackServer.Status.InstanceID != nil {
460-
logger.Info("Not reconciling server in failed state. The previously existing OpenStack instance is no longer available")
461-
conditions.MarkFalse(openStackServer, infrav1.InstanceReadyCondition, infrav1.InstanceNotFoundReason, clusterv1.ConditionSeverityError, "virtual machine no longer exists")
462-
return nil, nil
463-
}
464459

465460
logger.Info("Server does not exist, creating Server", "name", openStackServer.Name)
466461
instanceSpec, err := r.serverToInstanceSpec(ctx, openStackServer)

‎controllers/openstackserver_controller_test.go

Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import (
3131
"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/ports"
3232
. "github.com/onsi/gomega" //nolint:revive
3333
"go.uber.org/mock/gomock"
34+
corev1 "k8s.io/api/core/v1"
3435
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3536
"k8s.io/utils/ptr"
3637

@@ -39,6 +40,8 @@ import (
3940
"sigs.k8s.io/cluster-api-provider-openstack/pkg/clients/mock"
4041
"sigs.k8s.io/cluster-api-provider-openstack/pkg/cloud/services/compute"
4142
"sigs.k8s.io/cluster-api-provider-openstack/pkg/scope"
43+
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
44+
"sigs.k8s.io/cluster-api/util/conditions"
4245
)
4346

4447
const (
@@ -548,3 +551,182 @@ func Test_OpenStackServerReconcileCreate(t *testing.T) {
548551
})
549552
}
550553
}
554+
555+
func TestOpenStackServerReconciler_getOrCreateServer(t *testing.T) {
556+
tests := []struct {
557+
name string
558+
openStackServer *infrav1alpha1.OpenStackServer
559+
setupMocks func(r *recorders)
560+
wantServer *servers.Server
561+
wantErr bool
562+
wantCondition *clusterv1.Condition
563+
}{
564+
{
565+
name: "instanceID set in status but server not found",
566+
openStackServer: &infrav1alpha1.OpenStackServer{
567+
Status: infrav1alpha1.OpenStackServerStatus{
568+
InstanceID: ptr.To(instanceUUID),
569+
},
570+
},
571+
setupMocks: func(r *recorders) {
572+
r.compute.GetServer(instanceUUID).Return(nil, gophercloud.ErrUnexpectedResponseCode{Actual: 404})
573+
},
574+
wantErr: false,
575+
wantCondition: &clusterv1.Condition{
576+
Type: infrav1.InstanceReadyCondition,
577+
Status: corev1.ConditionFalse,
578+
Reason: infrav1.InstanceNotFoundReason,
579+
Message: infrav1.ServerUnexpectedDeletedMessage,
580+
},
581+
},
582+
{
583+
name: "instanceID set in status but server not found with error",
584+
openStackServer: &infrav1alpha1.OpenStackServer{
585+
Status: infrav1alpha1.OpenStackServerStatus{
586+
InstanceID: ptr.To(instanceUUID),
587+
},
588+
},
589+
setupMocks: func(r *recorders) {
590+
r.compute.GetServer(instanceUUID).Return(nil, fmt.Errorf("error"))
591+
},
592+
wantErr: true,
593+
wantCondition: &clusterv1.Condition{
594+
Type: infrav1.InstanceReadyCondition,
595+
Status: corev1.ConditionFalse,
596+
Reason: infrav1.InstanceNotFoundReason,
597+
Message: "get server \"" + instanceUUID + "\" detail failed: error",
598+
},
599+
},
600+
{
601+
name: "instanceStatus is nil but server found with machine name",
602+
openStackServer: &infrav1alpha1.OpenStackServer{
603+
ObjectMeta: metav1.ObjectMeta{
604+
Name: openStackServerName,
605+
},
606+
Status: infrav1alpha1.OpenStackServerStatus{},
607+
},
608+
setupMocks: func(r *recorders) {
609+
r.compute.ListServers(servers.ListOpts{
610+
Name: "^" + openStackServerName + "$",
611+
}).Return([]servers.Server{{ID: instanceUUID}}, nil)
612+
},
613+
wantErr: false,
614+
wantServer: &servers.Server{
615+
ID: instanceUUID,
616+
},
617+
},
618+
{
619+
name: "instanceStatus is nil and server not found and then created",
620+
openStackServer: &infrav1alpha1.OpenStackServer{
621+
ObjectMeta: metav1.ObjectMeta{
622+
Name: openStackServerName,
623+
},
624+
Status: infrav1alpha1.OpenStackServerStatus{
625+
Resolved: &infrav1alpha1.ResolvedServerSpec{
626+
ImageID: imageUUID,
627+
FlavorID: flavorUUID,
628+
Ports: defaultResolvedPorts,
629+
},
630+
},
631+
},
632+
setupMocks: func(r *recorders) {
633+
r.compute.ListServers(servers.ListOpts{
634+
Name: "^" + openStackServerName + "$",
635+
}).Return([]servers.Server{}, nil)
636+
r.compute.CreateServer(gomock.Any(), gomock.Any()).Return(&servers.Server{ID: instanceUUID}, nil)
637+
},
638+
wantErr: false,
639+
wantServer: &servers.Server{
640+
ID: instanceUUID,
641+
},
642+
},
643+
{
644+
name: "instanceStatus is nil and server not found and then created with error",
645+
openStackServer: &infrav1alpha1.OpenStackServer{
646+
ObjectMeta: metav1.ObjectMeta{
647+
Name: openStackServerName,
648+
},
649+
Status: infrav1alpha1.OpenStackServerStatus{
650+
Resolved: &infrav1alpha1.ResolvedServerSpec{
651+
ImageID: imageUUID,
652+
FlavorID: flavorUUID,
653+
Ports: defaultResolvedPorts,
654+
},
655+
},
656+
},
657+
setupMocks: func(r *recorders) {
658+
r.compute.ListServers(servers.ListOpts{
659+
Name: "^" + openStackServerName + "$",
660+
}).Return([]servers.Server{}, nil)
661+
r.compute.CreateServer(gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("error"))
662+
},
663+
wantErr: true,
664+
wantCondition: &clusterv1.Condition{
665+
Type: infrav1.InstanceReadyCondition,
666+
Status: corev1.ConditionFalse,
667+
Reason: infrav1.InstanceCreateFailedReason,
668+
Message: "error creating Openstack instance: " + "error",
669+
},
670+
},
671+
}
672+
673+
for _, tt := range tests {
674+
t.Run(tt.name, func(t *testing.T) {
675+
g := NewGomegaWithT(t)
676+
log := testr.New(t)
677+
678+
mockCtrl := gomock.NewController(t)
679+
defer mockCtrl.Finish()
680+
681+
mockScopeFactory := scope.NewMockScopeFactory(mockCtrl, "")
682+
scopeWithLogger := scope.NewWithLogger(mockScopeFactory, log)
683+
684+
computeRecorder := mockScopeFactory.ComputeClient.EXPECT()
685+
imageRecorder := mockScopeFactory.ImageClient.EXPECT()
686+
networkRecorder := mockScopeFactory.NetworkClient.EXPECT()
687+
volumeRecorder := mockScopeFactory.VolumeClient.EXPECT()
688+
689+
recorders := &recorders{
690+
compute: computeRecorder,
691+
image: imageRecorder,
692+
network: networkRecorder,
693+
volume: volumeRecorder,
694+
}
695+
696+
if tt.setupMocks != nil {
697+
tt.setupMocks(recorders)
698+
}
699+
700+
computeService, err := compute.NewService(scopeWithLogger)
701+
g.Expect(err).ToNot(HaveOccurred())
702+
703+
reconciler := OpenStackServerReconciler{}
704+
status, err := reconciler.getOrCreateServer(ctx, log, tt.openStackServer, computeService, []string{portUUID})
705+
706+
// Check error result
707+
if tt.wantErr {
708+
g.Expect(err).To(HaveOccurred())
709+
} else {
710+
g.Expect(err).ToNot(HaveOccurred())
711+
}
712+
713+
// Check instance status
714+
if tt.wantServer != nil {
715+
g.Expect(status.ID()).To(Equal(tt.wantServer.ID))
716+
}
717+
718+
// Check the condition is set correctly
719+
if tt.wantCondition != nil {
720+
// print openstackServer conditions
721+
for _, condition := range tt.openStackServer.Status.Conditions {
722+
t.Logf("Condition: %s, Status: %s, Reason: %s", condition.Type, condition.Status, condition.Reason)
723+
}
724+
conditionType := conditions.Get(tt.openStackServer, tt.wantCondition.Type)
725+
g.Expect(conditionType).ToNot(BeNil())
726+
g.Expect(conditionType.Status).To(Equal(tt.wantCondition.Status))
727+
g.Expect(conditionType.Reason).To(Equal(tt.wantCondition.Reason))
728+
g.Expect(conditionType.Message).To(Equal(tt.wantCondition.Message))
729+
}
730+
})
731+
}
732+
}

0 commit comments

Comments
 (0)