neonvm-controller: introduce unit tests #935

Omrigan · 2024-05-14T10:44:31Z

First PR, introducing unit tests. The old functional tests are moved to a functest directory.

Part of #763

sharnoff · 2024-05-20T03:26:48Z

Not a full review, but brief thoughts: At first glance, seems like there's a couple other changes that might be good to separate out into other PRs (to make them separate, and merged faster) — in particular, thinking of golangci-lint bump + renaming the files in neonvm/controllers.

Also, not sure if it's still the case, but IIRC there's some requirements around exact matching between golangci-lint versions & the go toolchain version. IIRC #885 was blocking upgrading the Go version, so maybe we can do that now?

Signed-off-by: Oleg Vasilev <[email protected]>

sharnoff

Some comments, didn't go super deep.

neonvm/controllers/functests/vm_controller_test.go1

neonvm/controllers/vm_controller_test.go

pkg/util/ptr.go

Makefile

Signed-off-by: Oleg Vasilev <[email protected]>

sharnoff

Replied to a comment, resolved others. + asked via DM about why move files to the functest directory.

Signed-off-by: Oleg Vasilev <[email protected]>

sharnoff

looks good, mostly just nits

neonvm/controllers/vm_controller_unit_test.go

sharnoff · 2024-06-16T22:05:15Z

neonvm/controllers/vm_controller_unit_test.go

+var reconcilerMetrics = MakeReconcilerMetrics()
+
+func newTestParams(t *testing.T) *testParams {
+	os.Setenv("VM_RUNNER_IMAGE", "vm-runner-img")


This isn't thread-safe, right? can we get away with not setting this for now? (if no: let's extract it into ReconcilerConfig?)

It is thread-safe, the implementation contains a mutex. I'd leave it right now, and later refactor it to be a normal parameter. CC #921

Huh ok, fair enough. Yeah, +1 to refactoring in a follow-up

neonvm/controllers/vm_controller_unit_test.go

sharnoff · 2024-06-16T22:16:36Z

neonvm/controllers/functests/vm_controller_test.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package controllers
+package functests


from DMs: if the reason to move this into a separate package is just so that it can be individually tested, that can also be accomplished with go test <file> -- so I'd prefer leaving this as-is unless there's other reasons as well

There are more than one test file in the controllers/, and it would be possible to run them all, while leaving functests aside.

I still don't quite get what you mean, but if you get value from having them separate, then fair enough, let's go with it :)

I meant there is vm_controller_test.go and vm_qmp_test.go, so by go test ./neonvm/controllers we can run them both.

Also it is "aesthetically" looks better IMO that lightweight unit tests are in the same folder, but heavier functional tests are in a different folder.

Signed-off-by: Oleg Vasilev <[email protected]>

sharnoff

lgtm, nice! one nit regarding package controllers vs package controllers_test

sharnoff · 2024-06-20T02:47:15Z

neonvm/controllers/vm_controller_unit_test.go

@@ -0,0 +1,252 @@
+package controllers


nit: let's make this controllers_test ?

AFAICT the following diff would be required to do that (mostly just doing a couple renames)

Diff

diff --git a/neonvm/controllers/vm_controller.go b/neonvm/controllers/vm_controller.go index e9ebfcf2..5f6b2254 100644 --- a/neonvm/controllers/vm_controller.go +++ b/neonvm/controllers/vm_controller.go @@ -60,15 +60,15 @@ import ( ) const ( - virtualmachineFinalizer = "vm.neon.tech/finalizer" + VirtualmachineFinalizer = "vm.neon.tech/finalizer" ) // Definitions to manage status conditions const ( - // typeAvailableVirtualMachine represents the status of the Deployment reconciliation - typeAvailableVirtualMachine = "Available" - // typeDegradedVirtualMachine represents the status used when the custom resource is deleted and the finalizer operations are must to occur. - typeDegradedVirtualMachine = "Degraded" + // TypeAvailableVirtualMachine represents the status of the Deployment reconciliation + TypeAvailableVirtualMachine = "Available" + // TypeDegradedVirtualMachine represents the status used when the custom resource is deleted and the finalizer operations are must to occur. + TypeDegradedVirtualMachine = "Degraded" ) const ( @@ -133,9 +133,9 @@ func (r *VMReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Re // The object is not being deleted, so if it does not have our finalizer, // then lets add the finalizer and update the object. This is equivalent // registering our finalizer. - if !controllerutil.ContainsFinalizer(&vm, virtualmachineFinalizer) { + if !controllerutil.ContainsFinalizer(&vm, VirtualmachineFinalizer) { log.Info("Adding Finalizer for VirtualMachine") - if ok := controllerutil.AddFinalizer(&vm, virtualmachineFinalizer); !ok { + if ok := controllerutil.AddFinalizer(&vm, VirtualmachineFinalizer); !ok { log.Info("Failed to add finalizer from VirtualMachine") return ctrl.Result{Requeue: true}, nil } @@ -147,14 +147,14 @@ func (r *VMReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Re } } else { // The object is being deleted - if controllerutil.ContainsFinalizer(&vm, virtualmachineFinalizer) { + if controllerutil.ContainsFinalizer(&vm, VirtualmachineFinalizer) { // our finalizer is present, so lets handle any external dependency log.Info("Performing Finalizer Operations for VirtualMachine before delete it") r.doFinalizerOperationsForVirtualMachine(ctx, &vm) // remove our finalizer from the list and update it. log.Info("Removing Finalizer for VirtualMachine after successfully perform the operations") - if ok := controllerutil.RemoveFinalizer(&vm, virtualmachineFinalizer); !ok { + if ok := controllerutil.RemoveFinalizer(&vm, VirtualmachineFinalizer); !ok { log.Info("Failed to remove finalizer from VirtualMachine") return ctrl.Result{Requeue: true}, nil } @@ -312,7 +312,7 @@ func (r *VMReconciler) doReconcile(ctx context.Context, vm *vmv1.VirtualMachine) // Let's check and just set the condition status as Unknown when no status are available if vm.Status.Conditions == nil || len(vm.Status.Conditions) == 0 { // set Unknown condition status for AvailableVirtualMachine - meta.SetStatusCondition(&vm.Status.Conditions, metav1.Condition{Type: typeAvailableVirtualMachine, Status: metav1.ConditionUnknown, Reason: "Reconciling", Message: "Starting reconciliation"}) + meta.SetStatusCondition(&vm.Status.Conditions, metav1.Condition{Type: TypeAvailableVirtualMachine, Status: metav1.ConditionUnknown, Reason: "Reconciling", Message: "Starting reconciliation"}) } // NB: .Spec.EnableSSH guaranteed non-nil because the k8s API server sets the default for us. @@ -434,7 +434,7 @@ func (r *VMReconciler) doReconcile(ctx context.Context, vm *vmv1.VirtualMachine) vm.Status.PodIP = vmRunner.Status.PodIP vm.Status.Phase = vmv1.VmRunning meta.SetStatusCondition(&vm.Status.Conditions, - metav1.Condition{Type: typeAvailableVirtualMachine, + metav1.Condition{Type: TypeAvailableVirtualMachine, Status: metav1.ConditionTrue, Reason: "Reconciling", Message: fmt.Sprintf("Pod (%s) for VirtualMachine (%s) created successfully", vm.Status.PodName, vm.Name)}) @@ -452,21 +452,21 @@ func (r *VMReconciler) doReconcile(ctx context.Context, vm *vmv1.VirtualMachine) case runnerSucceeded: vm.Status.Phase = vmv1.VmSucceeded meta.SetStatusCondition(&vm.Status.Conditions, - metav1.Condition{Type: typeAvailableVirtualMachine, + metav1.Condition{Type: TypeAvailableVirtualMachine, Status: metav1.ConditionFalse, Reason: "Reconciling", Message: fmt.Sprintf("Pod (%s) for VirtualMachine (%s) succeeded", vm.Status.PodName, vm.Name)}) case runnerFailed: vm.Status.Phase = vmv1.VmFailed meta.SetStatusCondition(&vm.Status.Conditions, - metav1.Condition{Type: typeDegradedVirtualMachine, + metav1.Condition{Type: TypeDegradedVirtualMachine, Status: metav1.ConditionTrue, Reason: "Reconciling", Message: fmt.Sprintf("Pod (%s) for VirtualMachine (%s) failed", vm.Status.PodName, vm.Name)}) case runnerUnknown: vm.Status.Phase = vmv1.VmPending meta.SetStatusCondition(&vm.Status.Conditions, - metav1.Condition{Type: typeAvailableVirtualMachine, + metav1.Condition{Type: TypeAvailableVirtualMachine, Status: metav1.ConditionUnknown, Reason: "Reconciling", Message: fmt.Sprintf("Pod (%s) for VirtualMachine (%s) in Unknown phase", vm.Status.PodName, vm.Name)}) @@ -484,7 +484,7 @@ func (r *VMReconciler) doReconcile(ctx context.Context, vm *vmv1.VirtualMachine) vm.Status.PodName)) vm.Status.Phase = vmv1.VmFailed meta.SetStatusCondition(&vm.Status.Conditions, - metav1.Condition{Type: typeDegradedVirtualMachine, + metav1.Condition{Type: TypeDegradedVirtualMachine, Status: metav1.ConditionTrue, Reason: "Reconciling", Message: fmt.Sprintf("Pod (%s) for VirtualMachine (%s) not found", vm.Status.PodName, vm.Name)}) @@ -572,21 +572,21 @@ func (r *VMReconciler) doReconcile(ctx context.Context, vm *vmv1.VirtualMachine) case runnerSucceeded: vm.Status.Phase = vmv1.VmSucceeded meta.SetStatusCondition(&vm.Status.Conditions, - metav1.Condition{Type: typeAvailableVirtualMachine, + metav1.Condition{Type: TypeAvailableVirtualMachine, Status: metav1.ConditionFalse, Reason: "Reconciling", Message: fmt.Sprintf("Pod (%s) for VirtualMachine (%s) succeeded", vm.Status.PodName, vm.Name)}) case runnerFailed: vm.Status.Phase = vmv1.VmFailed meta.SetStatusCondition(&vm.Status.Conditions, - metav1.Condition{Type: typeDegradedVirtualMachine, + metav1.Condition{Type: TypeDegradedVirtualMachine, Status: metav1.ConditionTrue, Reason: "Reconciling", Message: fmt.Sprintf("Pod (%s) for VirtualMachine (%s) failed", vm.Status.PodName, vm.Name)}) case runnerUnknown: vm.Status.Phase = vmv1.VmPending meta.SetStatusCondition(&vm.Status.Conditions, - metav1.Condition{Type: typeAvailableVirtualMachine, + metav1.Condition{Type: TypeAvailableVirtualMachine, Status: metav1.ConditionUnknown, Reason: "Reconciling", Message: fmt.Sprintf("Pod (%s) for VirtualMachine (%s) in Unknown phase", vm.Status.PodName, vm.Name)}) @@ -605,7 +605,7 @@ func (r *VMReconciler) doReconcile(ctx context.Context, vm *vmv1.VirtualMachine) vm.Status.PodName)) vm.Status.Phase = vmv1.VmFailed meta.SetStatusCondition(&vm.Status.Conditions, - metav1.Condition{Type: typeDegradedVirtualMachine, + metav1.Condition{Type: TypeDegradedVirtualMachine, Status: metav1.ConditionTrue, Reason: "Reconciling", Message: fmt.Sprintf("Pod (%s) for VirtualMachine (%s) not found", vm.Status.PodName, vm.Name)}) @@ -625,7 +625,7 @@ func (r *VMReconciler) doReconcile(ctx context.Context, vm *vmv1.VirtualMachine) case runnerSucceeded: vm.Status.Phase = vmv1.VmSucceeded meta.SetStatusCondition(&vm.Status.Conditions, - metav1.Condition{Type: typeAvailableVirtualMachine, + metav1.Condition{Type: TypeAvailableVirtualMachine, Status: metav1.ConditionFalse, Reason: "Reconciling", Message: fmt.Sprintf("Pod (%s) for VirtualMachine (%s) succeeded", vm.Status.PodName, vm.Name)}) @@ -633,7 +633,7 @@ func (r *VMReconciler) doReconcile(ctx context.Context, vm *vmv1.VirtualMachine) case runnerFailed: vm.Status.Phase = vmv1.VmFailed meta.SetStatusCondition(&vm.Status.Conditions, - metav1.Condition{Type: typeDegradedVirtualMachine, + metav1.Condition{Type: TypeDegradedVirtualMachine, Status: metav1.ConditionTrue, Reason: "Reconciling", Message: fmt.Sprintf("Pod (%s) for VirtualMachine (%s) failed", vm.Status.PodName, vm.Name)}) @@ -641,7 +641,7 @@ func (r *VMReconciler) doReconcile(ctx context.Context, vm *vmv1.VirtualMachine) case runnerUnknown: vm.Status.Phase = vmv1.VmPending meta.SetStatusCondition(&vm.Status.Conditions, - metav1.Condition{Type: typeAvailableVirtualMachine, + metav1.Condition{Type: TypeAvailableVirtualMachine, Status: metav1.ConditionUnknown, Reason: "Reconciling", Message: fmt.Sprintf("Pod (%s) for VirtualMachine (%s) in Unknown phase", vm.Status.PodName, vm.Name)}) diff --git a/neonvm/controllers/vm_controller_unit_test.go b/neonvm/controllers/vm_controller_unit_test.go index fc7f4429..5db7c824 100644 --- a/neonvm/controllers/vm_controller_unit_test.go +++ b/neonvm/controllers/vm_controller_unit_test.go @@ -1,4 +1,4 @@ -package controllers +package controllers_test import ( "context" @@ -24,6 +24,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" vmv1 "github.com/neondatabase/autoscaling/neonvm/apis/neonvm/v1" + "github.com/neondatabase/autoscaling/neonvm/controllers" ) type mockRecorder struct { @@ -78,13 +79,13 @@ func defaultVm() *vmv1.VirtualMachine { type testParams struct { t *testing.T ctx context.Context - r *VMReconciler + r *controllers.VMReconciler client client.Client origVM *vmv1.VirtualMachine mockRecorder *mockRecorder } -var reconcilerMetrics = MakeReconcilerMetrics() +var reconcilerMetrics = controllers.MakeReconcilerMetrics() func newTestParams(t *testing.T) *testParams { os.Setenv("VM_RUNNER_IMAGE", "vm-runner-img") @@ -107,11 +108,11 @@ func newTestParams(t *testing.T) *testParams { origVM: nil, } - params.r = &VMReconciler{ + params.r = &controllers.VMReconciler{ Client: params.client, Recorder: params.mockRecorder, Scheme: scheme, - Config: &ReconcilerConfig{ + Config: &controllers.ReconcilerConfig{ IsK3s: false, UseContainerMgr: false, MaxConcurrentReconciles: 10, @@ -159,7 +160,7 @@ func TestReconcile(t *testing.T) { assert.Equal(t, reconcile.Result{ Requeue: true, }, res) - assert.Contains(t, params.getVM().Finalizers, virtualmachineFinalizer) + assert.Contains(t, params.getVM().Finalizers, controllers.VirtualmachineFinalizer) // Round 2 res, err = params.r.Reconcile(params.ctx, req) @@ -199,7 +200,7 @@ func prettyPrint(t *testing.T, obj any) { func TestRunningPod(t *testing.T) { params := newTestParams(t) origVM := defaultVm() - origVM.Finalizers = append(origVM.Finalizers, virtualmachineFinalizer) + origVM.Finalizers = append(origVM.Finalizers, controllers.VirtualmachineFinalizer) origVM.Status.Phase = vmv1.VmPending origVM = params.initVM(origVM) @@ -248,5 +249,5 @@ func TestRunningPod(t *testing.T) { // VM is now running assert.Equal(t, vmv1.VmRunning, vm.Status.Phase) assert.Len(t, vm.Status.Conditions, 1) - assert.Equal(t, vm.Status.Conditions[0].Type, typeAvailableVirtualMachine) + assert.Equal(t, vm.Status.Conditions[0].Type, controllers.TypeAvailableVirtualMachine) }

Alternately, I'm ok to leave that as a follow-up if you like.

I want to leave the opportunity to test private functions also, these are unit tests after all.

Signed-off-by: Oleg Vasilev <[email protected]>

Seems like #935 had issues after merging main into it due to #971, but test failure didn't prevent auto-merge.

Omrigan changed the title ~~Oleg/neonvm tests~~ neonvm: introduce unit tests May 17, 2024

Omrigan requested a review from sharnoff May 17, 2024 17:39

Omrigan marked this pull request as ready for review May 17, 2024 17:39

Omrigan added 6 commits May 23, 2024 12:40

add TESTARGS

351aba0

Signed-off-by: Oleg Vasilev <[email protected]>

move old tests

c22275a

Signed-off-by: Oleg Vasilev <[email protected]>

initial test coverage

d4c1037

Signed-off-by: Oleg Vasilev <[email protected]>

wip

955d921

Signed-off-by: Oleg Vasilev <[email protected]>

fix lints

518d8bd

Signed-off-by: Oleg Vasilev <[email protected]>

chace rebase

53db1cf

Signed-off-by: Oleg Vasilev <[email protected]>

Omrigan force-pushed the oleg/neonvm-tests branch from d2e9726 to 53db1cf Compare May 23, 2024 09:45

Omrigan changed the title ~~neonvm: introduce unit tests~~ neonvm-controller: introduce unit tests May 24, 2024

Omrigan mentioned this pull request May 24, 2024

neonvm-controller: expand the tests with QMP mocked #943

Closed

sharnoff reviewed May 24, 2024

View reviewed changes

Omrigan added 4 commits May 30, 2024 17:09

reenable functests

2a0db8c

Signed-off-by: Oleg Vasilev <[email protected]>

delete util.Ptr

7155899

Signed-off-by: Oleg Vasilev <[email protected]>

Merge branch 'main' into oleg/neonvm-tests

fb8a8c7

Signed-off-by: Oleg Vasilev <[email protected]>

change pkg

376f250

Signed-off-by: Oleg Vasilev <[email protected]>

Omrigan mentioned this pull request May 30, 2024

Basic unit testing coverage for neonvm-controller #763

Open

add comment defaultVm

37dd6a7

Signed-off-by: Oleg Vasilev <[email protected]>

Omrigan requested a review from sharnoff May 31, 2024 09:45

sharnoff reviewed Jun 7, 2024

View reviewed changes

Omrigan requested a review from sharnoff June 10, 2024 09:01

Omrigan added 2 commits June 13, 2024 14:29

Merge branch 'main' into oleg/neonvm-tests

1968858

Signed-off-by: Oleg Vasilev <[email protected]>

use existing fake client implementation

f9a8e8e

Signed-off-by: Oleg Vasilev <[email protected]>

sharnoff reviewed Jun 16, 2024

View reviewed changes

Omrigan added 3 commits June 18, 2024 14:38

Merge branch 'main' into oleg/neonvm-tests

4349c34

Signed-off-by: Oleg Vasilev <[email protected]>

ignore coverage html

5fec735

Signed-off-by: Oleg Vasilev <[email protected]>

changes according to the comments

fdbb58b

Signed-off-by: Oleg Vasilev <[email protected]>

Omrigan requested a review from sharnoff June 19, 2024 09:27

sharnoff approved these changes Jun 20, 2024

View reviewed changes

Merge branch 'main' into oleg/neonvm-tests

82a3903

Signed-off-by: Oleg Vasilev <[email protected]>

Omrigan enabled auto-merge (squash) June 21, 2024 16:29

Omrigan merged commit 9de695e into main Jun 21, 2024
13 of 15 checks passed

Omrigan deleted the oleg/neonvm-tests branch June 21, 2024 16:49

sharnoff added a commit that referenced this pull request Jun 21, 2024

neonvm: Fix tests failing to compile

257fc8f

Seems like #935 had issues after merging main into it due to #971, but test failure didn't prevent auto-merge.

sharnoff mentioned this pull request Jun 21, 2024

neonvm: Fix tests failing to compile #986

Merged

sharnoff added a commit that referenced this pull request Jun 21, 2024

neonvm: Fix tests failing to compile (#986)

15ff20a

Seems like #935 had issues after merging main into it due to #971, but test failure didn't prevent auto-merge.

Omrigan mentioned this pull request Aug 11, 2024

neonvm-controller: expand the tests with QMP mocked #1032

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

neonvm-controller: introduce unit tests #935

neonvm-controller: introduce unit tests #935

Omrigan commented May 14, 2024 •

edited

Loading

sharnoff commented May 20, 2024

sharnoff left a comment

sharnoff left a comment

sharnoff left a comment

sharnoff Jun 16, 2024

Omrigan Jun 18, 2024

sharnoff Jun 20, 2024

sharnoff Jun 16, 2024

Omrigan Jun 17, 2024

sharnoff Jun 20, 2024

Omrigan Jun 21, 2024

sharnoff left a comment

sharnoff Jun 20, 2024

Omrigan Jun 21, 2024 •

edited

Loading

neonvm-controller: introduce unit tests #935

neonvm-controller: introduce unit tests #935

Conversation

Omrigan commented May 14, 2024 • edited Loading

sharnoff commented May 20, 2024

sharnoff left a comment

Choose a reason for hiding this comment

sharnoff left a comment

Choose a reason for hiding this comment

sharnoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharnoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Omrigan Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Omrigan commented May 14, 2024 •

edited

Loading

Omrigan Jun 21, 2024 •

edited

Loading