From 3bafd90c1cd7b626c13ec81310cb6dd111191260 Mon Sep 17 00:00:00 2001 From: Balazs Nadasdi Date: Fri, 12 Nov 2021 15:40:04 +0100 Subject: [PATCH 01/11] feat: Add retry counter on failed reconciliation If reconciliation failed, increase the Retry counter, set the NotBefore field to a future date and reschedule a retry. A Go routine handles the force retry because the system tries to reconcile only if an event tell the system to do that or with the fixed periodical Resync (which is slow for that). Because we never tracked if a MicroVM was able to boot or not, we just let the reconciler to check if the process is not there and react to the results. In case the MicroVM was not able to boot, we reported back a success on the MicroVM start step, which is not right and we can't track failed state with that. As a solution, now a step has a Verify function that will be called after Do. If the result is false, it marks the step failed. That way we can start the MicroVM, wait a bit and check if it's still running, if it's not running, the start failed. --- core/application/reconcile.go | 79 ++++++++++++++++++++++---- core/errors/errors.go | 1 + core/models/microvm.go | 2 + core/steps/event/publish.go | 4 ++ core/steps/microvm/create.go | 4 ++ core/steps/microvm/delete.go | 4 ++ core/steps/microvm/start.go | 24 ++++++++ core/steps/network/interface_create.go | 4 ++ core/steps/network/interface_delete.go | 4 ++ core/steps/runtime/dir_create.go | 4 ++ core/steps/runtime/dir_delete.go | 4 ++ core/steps/runtime/initrd_mount.go | 4 ++ core/steps/runtime/kernel_mount.go | 4 ++ core/steps/runtime/repo_release.go | 4 ++ core/steps/runtime/volume_mount.go | 4 ++ go.mod | 4 ++ go.sum | 3 + pkg/planner/actuator.go | 4 ++ pkg/planner/planner.go | 2 + 19 files changed, 151 insertions(+), 12 deletions(-) diff --git a/core/application/reconcile.go b/core/application/reconcile.go index 5a475616..adc95f1f 100644 --- a/core/application/reconcile.go +++ b/core/application/reconcile.go @@ -3,17 +3,22 @@ package application import ( "context" "fmt" + "time" "github.com/sirupsen/logrus" + "github.com/weaveworks/flintlock/api/events" "github.com/weaveworks/flintlock/core/models" "github.com/weaveworks/flintlock/core/plans" "github.com/weaveworks/flintlock/core/ports" portsctx "github.com/weaveworks/flintlock/core/ports/context" + "github.com/weaveworks/flintlock/pkg/defaults" "github.com/weaveworks/flintlock/pkg/log" "github.com/weaveworks/flintlock/pkg/planner" ) +const backoffBaseInSeconds = 20 + func (a *app) ReconcileMicroVM(ctx context.Context, id, namespace string) error { logger := log.GetLogger(ctx).WithField("action", "reconcile") @@ -52,14 +57,10 @@ func (a *app) ResyncMicroVMs(ctx context.Context, namespace string) error { return nil } -func (a *app) plan(spec *models.MicroVM, logger *logrus.Entry) (planner.Plan, error) { +func (a *app) plan(spec *models.MicroVM, logger *logrus.Entry) planner.Plan { l := logger.WithField("stage", "plan") l.Info("Generate plan") - if spec.Status.Retry > a.cfg.MaximumRetry { - return nil, reachedMaximumRetryError{vmid: spec.ID, retries: spec.Status.Retry} - } - // Delete only if the spec was marked as deleted. if spec.Spec.DeletedAt != 0 { input := &plans.DeletePlanInput{ @@ -67,7 +68,7 @@ func (a *app) plan(spec *models.MicroVM, logger *logrus.Entry) (planner.Plan, er VM: spec, } - return plans.MicroVMDeletePlan(input), nil + return plans.MicroVMDeletePlan(input) } input := &plans.CreateOrUpdatePlanInput{ @@ -75,22 +76,69 @@ func (a *app) plan(spec *models.MicroVM, logger *logrus.Entry) (planner.Plan, er VM: spec, } - return plans.MicroVMCreateOrUpdatePlan(input), nil + return plans.MicroVMCreateOrUpdatePlan(input) +} + +func (a *app) reschedule(ctx context.Context, logger *logrus.Entry, spec *models.MicroVM) error { + spec.Status.Retry++ + waitTime := time.Duration(spec.Status.Retry*backoffBaseInSeconds) * time.Second + spec.Status.NotBefore = time.Now().Add(waitTime).Unix() + + logger.Infof( + "[%d/%d] reconciliation failed, reschedule at %s", + spec.Status.Retry, + a.cfg.MaximumRetry, + time.Unix(spec.Status.NotBefore, 0), + ) + + if _, err := a.ports.Repo.Save(ctx, spec); err != nil { + return fmt.Errorf("saving spec after plan failed: %w", err) + } + + go func(id, ns string, sleepTime time.Duration) { + logger.Info("Wait to emit update") + time.Sleep(sleepTime) + logger.Info("Emit pdate") + + _ = a.ports.EventService.Publish( + context.Background(), + defaults.TopicMicroVMEvents, + &events.MicroVMSpecUpdated{ + ID: id, + Namespace: ns, + }, + ) + }(spec.ID.Name(), spec.ID.Namespace(), waitTime) + + return nil } func (a *app) reconcile(ctx context.Context, spec *models.MicroVM, logger *logrus.Entry) error { - l := logger.WithField("vmid", spec.ID.String()) - l.Info("Starting reconciliation") + localLogger := logger.WithField("vmid", spec.ID.String()) + localLogger.Info("Starting reconciliation") - plan, planErr := a.plan(spec, l) - if planErr != nil { - return planErr + if spec.Status.Retry > a.cfg.MaximumRetry { + spec.Status.State = models.FailedState + + logger.Error(reachedMaximumRetryError{vmid: spec.ID, retries: spec.Status.Retry}) + + return nil + } + + if spec.Status.NotBefore > 0 && time.Now().Before(time.Unix(spec.Status.NotBefore, 0)) { + return nil } + plan := a.plan(spec, localLogger) + execCtx := portsctx.WithPorts(ctx, a.ports) executionID, err := a.ports.IdentifierService.GenerateRandom() if err != nil { + if scheduleErr := a.reschedule(ctx, localLogger, spec); scheduleErr != nil { + return fmt.Errorf("saving spec after plan failed: %w", scheduleErr) + } + return fmt.Errorf("generating plan execution id: %w", err) } @@ -98,6 +146,10 @@ func (a *app) reconcile(ctx context.Context, spec *models.MicroVM, logger *logru stepCount, err := actuator.Execute(execCtx, plan, executionID) if err != nil { + if scheduleErr := a.reschedule(ctx, localLogger, spec); scheduleErr != nil { + return fmt.Errorf("saving spec after plan failed: %w", scheduleErr) + } + return fmt.Errorf("executing plan: %w", err) } @@ -109,6 +161,9 @@ func (a *app) reconcile(ctx context.Context, spec *models.MicroVM, logger *logru return nil } + spec.Status.Retry = 0 + spec.Status.NotBefore = 0 + if _, err := a.ports.Repo.Save(ctx, spec); err != nil { return fmt.Errorf("saving spec after plan execution: %w", err) } diff --git a/core/errors/errors.go b/core/errors/errors.go index 71a63eab..7057e9d5 100644 --- a/core/errors/errors.go +++ b/core/errors/errors.go @@ -20,6 +20,7 @@ var ( ErrUnsupportedIfaceType = errors.New("unsupported network interface type") ErrIfaceNotFound = errors.New("network interface not found") ErrMissingStatusInfo = errors.New("status is not defined") + ErrUnableToBoot = errors.New("microvm is unable to boot") ) // TopicNotFoundError is an error created when a topic with a specific name isn't found. diff --git a/core/models/microvm.go b/core/models/microvm.go index 53334388..f8f9d4f3 100644 --- a/core/models/microvm.go +++ b/core/models/microvm.go @@ -59,6 +59,8 @@ type MicroVMStatus struct { NetworkInterfaces NetworkInterfaceStatuses `json:"network_interfaces"` // Retry is a counter about how many times we retried to reconcile. Retry int `json:"retry"` + // DeletedAt indicates the time the microvm was marked as deleted. + NotBefore int64 `json:"not_before" validate:"omitempty"` } // Kernel is the specification of the kernel and its arguments. diff --git a/core/steps/event/publish.go b/core/steps/event/publish.go index 9678db68..49051c4a 100644 --- a/core/steps/event/publish.go +++ b/core/steps/event/publish.go @@ -47,3 +47,7 @@ func (s *eventPublish) Do(ctx context.Context) ([]planner.Procedure, error) { return nil, nil } + +func (s *eventPublish) Verify(ctx context.Context) error { + return nil +} diff --git a/core/steps/microvm/create.go b/core/steps/microvm/create.go index a12fadcd..266c0e02 100644 --- a/core/steps/microvm/create.go +++ b/core/steps/microvm/create.go @@ -63,3 +63,7 @@ func (s *createStep) Do(ctx context.Context) ([]planner.Procedure, error) { return nil, nil } + +func (s *createStep) Verify(ctx context.Context) error { + return nil +} diff --git a/core/steps/microvm/delete.go b/core/steps/microvm/delete.go index 25c70156..b00d18cb 100644 --- a/core/steps/microvm/delete.go +++ b/core/steps/microvm/delete.go @@ -60,3 +60,7 @@ func (s *deleteStep) Do(ctx context.Context) ([]planner.Procedure, error) { return nil, nil } + +func (s *deleteStep) Verify(ctx context.Context) error { + return nil +} diff --git a/core/steps/microvm/start.go b/core/steps/microvm/start.go index 0dab0421..f0b55f89 100644 --- a/core/steps/microvm/start.go +++ b/core/steps/microvm/start.go @@ -3,6 +3,7 @@ package microvm import ( "context" "fmt" + "time" "github.com/sirupsen/logrus" @@ -13,6 +14,8 @@ import ( "github.com/weaveworks/flintlock/pkg/planner" ) +const waitToBoot = 10 + func NewStartStep(vm *models.MicroVM, vmSvc ports.MicroVMService) planner.Procedure { return &startStep{ vm: vm, @@ -63,3 +66,24 @@ func (s *startStep) Do(ctx context.Context) ([]planner.Procedure, error) { return nil, nil } + +func (s *startStep) Verify(ctx context.Context) error { + logger := log.GetLogger(ctx).WithFields(logrus.Fields{ + "step": s.Name(), + "vmid": s.vm.ID, + }) + logger.Debug("waiting for the microvm to start") + time.Sleep(waitToBoot * time.Second) + logger.Debug("verify microvm is started") + + state, err := s.vmSvc.State(ctx, s.vm.ID.String()) + if err != nil { + return fmt.Errorf("checking if microvm is running: %w", err) + } + + if state != ports.MicroVMStateRunning { + return errors.ErrUnableToBoot + } + + return nil +} diff --git a/core/steps/network/interface_create.go b/core/steps/network/interface_create.go index f12830c7..43963140 100644 --- a/core/steps/network/interface_create.go +++ b/core/steps/network/interface_create.go @@ -123,3 +123,7 @@ func (s *createInterface) Do(ctx context.Context) ([]planner.Procedure, error) { return nil, nil } + +func (s *createInterface) Verify(ctx context.Context) error { + return nil +} diff --git a/core/steps/network/interface_delete.go b/core/steps/network/interface_delete.go index 5840756a..dbc0c464 100644 --- a/core/steps/network/interface_delete.go +++ b/core/steps/network/interface_delete.go @@ -96,3 +96,7 @@ func (s deleteInterface) ShouldDo(ctx context.Context) (bool, error) { return exists, nil } + +func (s deleteInterface) Verify(ctx context.Context) error { + return nil +} diff --git a/core/steps/runtime/dir_create.go b/core/steps/runtime/dir_create.go index e3012142..427d0fa5 100644 --- a/core/steps/runtime/dir_create.go +++ b/core/steps/runtime/dir_create.go @@ -105,3 +105,7 @@ func (s *createDirectory) directoryExists() (bool, error) { return exists, nil } + +func (s *createDirectory) Verify(ctx context.Context) error { + return nil +} diff --git a/core/steps/runtime/dir_delete.go b/core/steps/runtime/dir_delete.go index a550da44..9c7ac696 100644 --- a/core/steps/runtime/dir_delete.go +++ b/core/steps/runtime/dir_delete.go @@ -72,3 +72,7 @@ func (s *deleteDirectory) targetExists() (bool, error) { return exists, nil } + +func (s *deleteDirectory) Verify(ctx context.Context) error { + return nil +} diff --git a/core/steps/runtime/initrd_mount.go b/core/steps/runtime/initrd_mount.go index 689dd820..b059f2ce 100644 --- a/core/steps/runtime/initrd_mount.go +++ b/core/steps/runtime/initrd_mount.go @@ -95,3 +95,7 @@ func (s *initrdMount) getMountSpec() *ports.ImageMountSpec { Use: models.ImageUseInitrd, } } + +func (s *initrdMount) Verify(ctx context.Context) error { + return nil +} diff --git a/core/steps/runtime/kernel_mount.go b/core/steps/runtime/kernel_mount.go index 28faaaf1..bea23fbf 100644 --- a/core/steps/runtime/kernel_mount.go +++ b/core/steps/runtime/kernel_mount.go @@ -95,3 +95,7 @@ func (s *kernelMount) getMountSpec() *ports.ImageMountSpec { Use: models.ImageUseKernel, } } + +func (s *kernelMount) Verify(ctx context.Context) error { + return nil +} diff --git a/core/steps/runtime/repo_release.go b/core/steps/runtime/repo_release.go index 182b907f..ecd26db9 100644 --- a/core/steps/runtime/repo_release.go +++ b/core/steps/runtime/repo_release.go @@ -65,3 +65,7 @@ func (s *repoRelease) Do(ctx context.Context) ([]planner.Procedure, error) { return nil, nil } + +func (s *repoRelease) Verify(ctx context.Context) error { + return nil +} diff --git a/core/steps/runtime/volume_mount.go b/core/steps/runtime/volume_mount.go index 54e4c9bc..b9d26014 100644 --- a/core/steps/runtime/volume_mount.go +++ b/core/steps/runtime/volume_mount.go @@ -95,3 +95,7 @@ func (s *volumeMount) getMountSpec() *ports.ImageMountSpec { Use: models.ImageUseVolume, } } + +func (s *volumeMount) Verify(ctx context.Context) error { + return nil +} diff --git a/go.mod b/go.mod index 3ae2a774..275a4378 100644 --- a/go.mod +++ b/go.mod @@ -74,6 +74,7 @@ require ( github.com/gogo/protobuf v1.3.2 // indirect github.com/golang/groupcache v0.0.0-20200121045136-8c9f03a8e57e // indirect github.com/golang/protobuf v1.5.2 // indirect + github.com/google/subcommands v1.0.1 // indirect github.com/hashicorp/errwrap v1.0.0 // indirect github.com/hashicorp/go-multierror v1.1.0 // indirect github.com/hashicorp/hcl v1.0.0 // indirect @@ -102,9 +103,12 @@ require ( go.mongodb.org/mongo-driver v1.7.3 // indirect go.opencensus.io v0.23.0 // indirect golang.org/x/crypto v0.0.0-20210817164053-32db794688a5 // indirect + golang.org/x/mod v0.4.2 // indirect golang.org/x/sync v0.0.0-20210220032951-036812b2e83c // indirect golang.org/x/sys v0.0.0-20210823070655-63515b42dcdf // indirect golang.org/x/text v0.3.6 // indirect + golang.org/x/tools v0.1.5 // indirect + golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect google.golang.org/genproto v0.0.0-20211021150943-2b146023228c // indirect gopkg.in/ini.v1 v1.63.2 // indirect gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect diff --git a/go.sum b/go.sum index 4da437cc..f7a457c7 100644 --- a/go.sum +++ b/go.sum @@ -528,6 +528,7 @@ github.com/google/pprof v0.0.0-20210601050228-01bbb1931b22/go.mod h1:kpwsk12EmLe github.com/google/pprof v0.0.0-20210609004039-a478d1d731e9/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE= github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE= github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI= +github.com/google/subcommands v1.0.1 h1:/eqq+otEXm5vhfBrbREPCSVQbvofip6kIz+mX5TUH7k= github.com/google/subcommands v1.0.1/go.mod h1:ZjhPrFU+Olkh9WazFPsl27BQ4UPiG37m3yTrtFlrHVk= github.com/google/uuid v1.0.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= @@ -1026,6 +1027,7 @@ golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.4.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.4.1/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= +golang.org/x/mod v0.4.2 h1:Gz96sIWK3OalVv/I/qNygP42zyoKp3xptRVCWRFEBvo= golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= @@ -1294,6 +1296,7 @@ golang.org/x/tools v0.1.1/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/tools v0.1.2/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/tools v0.1.3/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/tools v0.1.4/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= +golang.org/x/tools v0.1.5 h1:ouewzE6p+/VEB31YYnTbEJdi8pFqKp4P4n85vwo3DHA= golang.org/x/tools v0.1.5/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/pkg/planner/actuator.go b/pkg/planner/actuator.go index f9275740..bef5530e 100644 --- a/pkg/planner/actuator.go +++ b/pkg/planner/actuator.go @@ -102,6 +102,10 @@ func (e *actuatorImpl) react(ctx context.Context, steps []Procedure, logger *log if err != nil { return numStepsExecuted, fmt.Errorf("executing step %s: %w", step.Name(), err) } + + if verifyErr := step.Verify(ctx); verifyErr != nil { + return numStepsExecuted, fmt.Errorf("verifying step %s: %w", step.Name(), verifyErr) + } } } diff --git a/pkg/planner/planner.go b/pkg/planner/planner.go index 7bd1352d..8a885315 100644 --- a/pkg/planner/planner.go +++ b/pkg/planner/planner.go @@ -25,4 +25,6 @@ type Procedure interface { Do(ctx context.Context) ([]Procedure, error) // ShouldDo determines if this procedure should be executed ShouldDo(ctx context.Context) (bool, error) + // Verify the state after Do. + Verify(ctx context.Context) error } From e1c3626109f4b1739891debe7af93ef1740376d4 Mon Sep 17 00:00:00 2001 From: Balazs Nadasdi Date: Fri, 12 Nov 2021 15:54:32 +0100 Subject: [PATCH 02/11] Fix test // will do proper test later --- pkg/planner/actuator_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/planner/actuator_test.go b/pkg/planner/actuator_test.go index baadbdc2..24161c53 100644 --- a/pkg/planner/actuator_test.go +++ b/pkg/planner/actuator_test.go @@ -173,3 +173,7 @@ func (p *testProc) Do(ctx context.Context) ([]planner.Procedure, error) { func (p *testProc) ShouldDo(ctx context.Context) (bool, error) { return true, nil } + +func (p *testProc) Verify(ctx context.Context) error { + return nil +} From e48e1d78783cf663d7600686489f558f28adaaaf Mon Sep 17 00:00:00 2001 From: Balazs Nadasdi Date: Mon, 15 Nov 2021 11:14:04 +0100 Subject: [PATCH 03/11] Stop hard failing resync on boot If one fails, we can still listen on new requests and reconcile vms, if they are failing always, the retry logic will handle this. --- core/steps/microvm/start.go | 2 +- infrastructure/controllers/microvm_controller.go | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/core/steps/microvm/start.go b/core/steps/microvm/start.go index f0b55f89..013dcf40 100644 --- a/core/steps/microvm/start.go +++ b/core/steps/microvm/start.go @@ -14,7 +14,7 @@ import ( "github.com/weaveworks/flintlock/pkg/planner" ) -const waitToBoot = 10 +const waitToBoot = 5 func NewStartStep(vm *models.MicroVM, vmSvc ports.MicroVMService) planner.Procedure { return &startStep{ diff --git a/infrastructure/controllers/microvm_controller.go b/infrastructure/controllers/microvm_controller.go index 2aca33fc..e34ae14f 100644 --- a/infrastructure/controllers/microvm_controller.go +++ b/infrastructure/controllers/microvm_controller.go @@ -48,7 +48,10 @@ func (r *MicroVMController) Run(ctx context.Context, if resyncOnStart { if err := r.resyncSpecs(ctx, logger); err != nil { - return fmt.Errorf("resyncing specs on start: %w", err) + // Do not return here, if one fails, we can still listen on + // new requests and reconcile vms, if they are failing always, + // the retry logic will handle this. + logger.Errorf("resyncing specs on start: %s", err.Error()) } } From 7ec697903717a7f809c0e243fe69aec7d11fc9e2 Mon Sep 17 00:00:00 2001 From: Balazs Nadasdi Date: Tue, 16 Nov 2021 11:13:52 +0100 Subject: [PATCH 04/11] Test --- core/plans/const.go | 2 + core/plans/microvm_create_update.go | 2 +- core/steps/event/publish_test.go | 4 ++ core/steps/microvm/create_test.go | 10 ++++ core/steps/microvm/delete_test.go | 10 ++++ core/steps/microvm/start.go | 20 +++++--- core/steps/microvm/start_test.go | 57 ++++++++++++++++++--- core/steps/network/interface_create_test.go | 32 ++++++++---- core/steps/network/interface_delete_test.go | 30 +++++++---- core/steps/runtime/dir_create_test.go | 10 ++++ core/steps/runtime/dir_delete_test.go | 27 +++++----- core/steps/runtime/initrd_mount_test.go | 18 +++++++ core/steps/runtime/kernel_mount_test.go | 18 +++++++ core/steps/runtime/repo_release_test.go | 15 ++++++ core/steps/runtime/volume_mount_test.go | 15 ++++++ 15 files changed, 224 insertions(+), 46 deletions(-) diff --git a/core/plans/const.go b/core/plans/const.go index 50f6e891..32aad7fb 100644 --- a/core/plans/const.go +++ b/core/plans/const.go @@ -3,4 +3,6 @@ package plans const ( MicroVMDeletePlanName = "microvm_delete" MicroVMCreateOrUpdatePlanName = "microvm_create_update" + + microVMBootTime = 30 ) diff --git a/core/plans/microvm_create_update.go b/core/plans/microvm_create_update.go index 238c5d77..0da1ae84 100644 --- a/core/plans/microvm_create_update.go +++ b/core/plans/microvm_create_update.go @@ -75,7 +75,7 @@ func (p *microvmCreateOrUpdatePlan) Create(ctx context.Context) ([]planner.Proce } // MicroVM provider start - if err := p.addStep(ctx, microvm.NewStartStep(p.vm, ports.Provider)); err != nil { + if err := p.addStep(ctx, microvm.NewStartStep(p.vm, ports.Provider, microVMBootTime)); err != nil { return nil, fmt.Errorf("adding microvm start step: %w", err) } diff --git a/core/steps/event/publish_test.go b/core/steps/event/publish_test.go index 0b0e93bb..d1b00005 100644 --- a/core/steps/event/publish_test.go +++ b/core/steps/event/publish_test.go @@ -41,10 +41,12 @@ func TestNewPublish(t *testing.T) { // now than crying later if the system does not do what we want. shouldDo, _ := step.ShouldDo(ctx) subSteps, err := step.Do(ctx) + verifyErr := step.Verify(ctx) g.Expect(shouldDo).To(g.BeTrue()) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(err).To(g.BeNil()) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewPublish_eventServiceFailure(t *testing.T) { @@ -67,7 +69,9 @@ func TestNewPublish_eventServiceFailure(t *testing.T) { step := event.NewPublish(testTopic, evt, eventService) subSteps, err := step.Do(ctx) + verifyErr := step.Verify(ctx) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(err).ToNot(g.BeNil()) + g.Expect(verifyErr).To(g.BeNil()) } diff --git a/core/steps/microvm/create_test.go b/core/steps/microvm/create_test.go index 901af2d9..c075c43d 100644 --- a/core/steps/microvm/create_test.go +++ b/core/steps/microvm/create_test.go @@ -58,11 +58,13 @@ func TestNewCreateStep(t *testing.T) { shouldDo, shouldErr := step.ShouldDo(ctx) subSteps, doErr := step.Do(ctx) + verifyErr := step.Verify(ctx) g.Expect(shouldDo).To(g.BeTrue()) g.Expect(shouldErr).To(g.BeNil()) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).To(g.BeNil()) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewCreateStep_StateCheck(t *testing.T) { @@ -96,9 +98,11 @@ func TestNewCreateStep_StateCheck(t *testing.T) { Return(testCase.State, nil) shouldDo, shouldErr := step.ShouldDo(ctx) + verifyErr := step.Verify(ctx) g.Expect(shouldDo).To(g.Equal(testCase.ExpectToRun)) g.Expect(shouldErr).To(g.BeNil()) + g.Expect(verifyErr).To(g.BeNil()) } } @@ -119,9 +123,11 @@ func TestNewCreateStep_StateCheckError(t *testing.T) { Return(ports.MicroVMStateUnknown, errors.New("i have no idea")) shouldDo, shouldErr := step.ShouldDo(ctx) + verifyErr := step.Verify(ctx) g.Expect(shouldDo).To(g.BeFalse()) g.Expect(shouldErr).ToNot(g.BeNil()) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewCreateStep_VMIsNotDefined(t *testing.T) { @@ -137,9 +143,11 @@ func TestNewCreateStep_VMIsNotDefined(t *testing.T) { step := microvm.NewCreateStep(vm, microVMService) subSteps, err := step.Do(ctx) + verifyErr := step.Verify(ctx) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(err).To(g.MatchError(internalerr.ErrSpecRequired)) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewCreateStep_ServiceCreateError(t *testing.T) { @@ -159,7 +167,9 @@ func TestNewCreateStep_ServiceCreateError(t *testing.T) { Return(errors.New("ensuring state dir: ....")) subSteps, err := step.Do(ctx) + verifyErr := step.Verify(ctx) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(err).ToNot(g.BeNil()) + g.Expect(verifyErr).To(g.BeNil()) } diff --git a/core/steps/microvm/delete_test.go b/core/steps/microvm/delete_test.go index b5f1a395..1ebad8ae 100644 --- a/core/steps/microvm/delete_test.go +++ b/core/steps/microvm/delete_test.go @@ -59,11 +59,13 @@ func TestNewDeleteStep(t *testing.T) { shouldDo, shouldErr := step.ShouldDo(ctx) subSteps, doErr := step.Do(ctx) + verifyErr := step.Verify(ctx) g.Expect(shouldDo).To(g.BeTrue()) g.Expect(shouldErr).To(g.BeNil()) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).To(g.BeNil()) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewDeleteStep_StateCheck(t *testing.T) { @@ -97,9 +99,11 @@ func TestNewDeleteStep_StateCheck(t *testing.T) { Return(testCase.State, nil) shouldDo, shouldErr := step.ShouldDo(ctx) + verifyErr := step.Verify(ctx) g.Expect(shouldDo).To(g.Equal(testCase.ExpectToRun)) g.Expect(shouldErr).To(g.BeNil()) + g.Expect(verifyErr).To(g.BeNil()) } } @@ -120,9 +124,11 @@ func TestNewDeleteStep_StateCheckError(t *testing.T) { Return(ports.MicroVMStateUnknown, errors.New("i have no idea")) shouldDo, shouldErr := step.ShouldDo(ctx) + verifyErr := step.Verify(ctx) g.Expect(shouldDo).To(g.BeFalse()) g.Expect(shouldErr).ToNot(g.BeNil()) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewDeleteStep_VMIsNotDefined(t *testing.T) { @@ -138,9 +144,11 @@ func TestNewDeleteStep_VMIsNotDefined(t *testing.T) { step := microvm.NewDeleteStep(vm, microVMService) subSteps, err := step.Do(ctx) + verifyErr := step.Verify(ctx) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(err).To(g.MatchError(internalerr.ErrSpecRequired)) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewDeleteStep_ServiceDeleteError(t *testing.T) { @@ -160,7 +168,9 @@ func TestNewDeleteStep_ServiceDeleteError(t *testing.T) { Return(errors.New("ensuring state dir: ....")) subSteps, err := step.Do(ctx) + verifyErr := step.Verify(ctx) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(err).ToNot(g.BeNil()) + g.Expect(verifyErr).To(g.BeNil()) } diff --git a/core/steps/microvm/start.go b/core/steps/microvm/start.go index 013dcf40..839681bc 100644 --- a/core/steps/microvm/start.go +++ b/core/steps/microvm/start.go @@ -14,18 +14,22 @@ import ( "github.com/weaveworks/flintlock/pkg/planner" ) -const waitToBoot = 5 - -func NewStartStep(vm *models.MicroVM, vmSvc ports.MicroVMService) planner.Procedure { +func NewStartStep( + vm *models.MicroVM, + vmSvc ports.MicroVMService, + bootTime int, +) planner.Procedure { return &startStep{ - vm: vm, - vmSvc: vmSvc, + vm: vm, + vmSvc: vmSvc, + bootTime: bootTime, } } type startStep struct { - vm *models.MicroVM - vmSvc ports.MicroVMService + vm *models.MicroVM + vmSvc ports.MicroVMService + bootTime int } // Name is the name of the procedure/operation. @@ -73,7 +77,7 @@ func (s *startStep) Verify(ctx context.Context) error { "vmid": s.vm.ID, }) logger.Debug("waiting for the microvm to start") - time.Sleep(waitToBoot * time.Second) + time.Sleep(time.Duration(s.bootTime) * time.Second) logger.Debug("verify microvm is started") state, err := s.vmSvc.State(ctx, s.vm.ID.String()) diff --git a/core/steps/microvm/start_test.go b/core/steps/microvm/start_test.go index 42d90c5e..40edae24 100644 --- a/core/steps/microvm/start_test.go +++ b/core/steps/microvm/start_test.go @@ -44,13 +44,18 @@ func TestNewStartStep(t *testing.T) { ctx := context.Background() vm := testVMToStart() - step := microvm.NewStartStep(vm, microVMService) + step := microvm.NewStartStep(vm, microVMService, 1) microVMService. EXPECT(). State(ctx, vm.ID.String()). Return(ports.MicroVMStateConfigured, nil) + microVMService. + EXPECT(). + State(ctx, vm.ID.String()). + Return(ports.MicroVMStateRunning, nil) + microVMService. EXPECT(). Start(ctx, vm.ID.String()). @@ -58,11 +63,13 @@ func TestNewStartStep(t *testing.T) { shouldDo, shouldErr := step.ShouldDo(ctx) subSteps, doErr := step.Do(ctx) + verifyErr := step.Verify(ctx) g.Expect(shouldDo).To(g.BeTrue()) g.Expect(shouldErr).To(g.BeNil()) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).To(g.BeNil()) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewStartStep_StateCheck(t *testing.T) { @@ -87,7 +94,7 @@ func TestNewStartStep_StateCheck(t *testing.T) { ctx := context.Background() vm := testVMToStart() - step := microvm.NewStartStep(vm, microVMService) + step := microvm.NewStartStep(vm, microVMService, 1) for _, testCase := range stateTestCases { microVMService. @@ -100,7 +107,6 @@ func TestNewStartStep_StateCheck(t *testing.T) { g.Expect(shouldDo).To(g.Equal(testCase.ExpectToRun)) g.Expect(shouldErr).To(g.BeNil()) } - } func TestNewStartStep_StateCheckError(t *testing.T) { @@ -112,7 +118,7 @@ func TestNewStartStep_StateCheckError(t *testing.T) { ctx := context.Background() vm := testVMToStart() - step := microvm.NewStartStep(vm, microVMService) + step := microvm.NewStartStep(vm, microVMService, 1) microVMService. EXPECT(). @@ -135,7 +141,7 @@ func TestNewStartStep_VMIsNotDefined(t *testing.T) { microVMService := mock.NewMockMicroVMService(mockCtrl) ctx := context.Background() - step := microvm.NewStartStep(vm, microVMService) + step := microvm.NewStartStep(vm, microVMService, 1) subSteps, err := step.Do(ctx) @@ -152,7 +158,7 @@ func TestNewStartStep_ServiceStartError(t *testing.T) { vm := testVMToStart() ctx := context.Background() - step := microvm.NewStartStep(vm, microVMService) + step := microvm.NewStartStep(vm, microVMService, 1) microVMService. EXPECT(). @@ -164,3 +170,42 @@ func TestNewStartStep_ServiceStartError(t *testing.T) { g.Expect(subSteps).To(g.BeEmpty()) g.Expect(err).ToNot(g.BeNil()) } + +func TestNewStartStep_unableToBoot(t *testing.T) { + g.RegisterTestingT(t) + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + microVMService := mock.NewMockMicroVMService(mockCtrl) + vm := testVMToStart() + ctx := context.Background() + + step := microvm.NewStartStep(vm, microVMService, 1) + + microVMService. + EXPECT(). + Start(ctx, vm.ID.String()). + Return(nil) + + microVMService. + EXPECT(). + State(ctx, vm.ID.String()). + Return(ports.MicroVMStateUnknown, nil) + + subSteps, err := step.Do(ctx) + verifyErr := step.Verify(ctx) + + g.Expect(subSteps).To(g.BeEmpty()) + g.Expect(err).To(g.BeNil()) + g.Expect(verifyErr).To(g.MatchError(internalerr.ErrUnableToBoot)) + + microVMService. + EXPECT(). + State(ctx, vm.ID.String()). + Return(ports.MicroVMStateUnknown, errors.New("nope")) + + verifyErr = step.Verify(ctx) + + g.Expect(verifyErr).ToNot(g.BeNil()) + g.Expect(verifyErr).ToNot(g.MatchError(internalerr.ErrUnableToBoot)) +} diff --git a/core/steps/network/interface_create_test.go b/core/steps/network/interface_create_test.go index 3d3c4128..c14c9b32 100644 --- a/core/steps/network/interface_create_test.go +++ b/core/steps/network/interface_create_test.go @@ -30,14 +30,16 @@ func TestNewNetworkInterface_everythingIsEmpty(t *testing.T) { Times(0) step := network.NewNetworkInterface(vmid, iface, status, svc) - shouldDo, err := step.ShouldDo(ctx) + shouldDo, err := step.ShouldDo(ctx) g.Expect(err).To(g.BeNil()) g.Expect(shouldDo).To(g.BeTrue()) _, err = step.Do(ctx) - g.Expect(err).To(g.MatchError(errors.ErrGuestDeviceNameRequired)) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewNetworkInterface_doesNotExist(t *testing.T) { @@ -81,8 +83,10 @@ func TestNewNetworkInterface_doesNotExist(t *testing.T) { Times(1) _, err = step.Do(ctx) - g.Expect(err).To(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewNetworkInterface_emptyStatus(t *testing.T) { @@ -116,8 +120,10 @@ func TestNewNetworkInterface_emptyStatus(t *testing.T) { Times(0) _, err = step.Do(ctx) - g.Expect(err).ToNot(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewNetworkInterface_existingInterface(t *testing.T) { @@ -168,8 +174,10 @@ func TestNewNetworkInterface_existingInterface(t *testing.T) { Times(1) _, err = step.Do(ctx) - g.Expect(err).To(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewNetworkInterface_missingInterface(t *testing.T) { @@ -241,8 +249,10 @@ func TestNewNetworkInterface_svcError(t *testing.T) { g.Expect(shouldDo).To(g.BeFalse()) _, err = step.Do(ctx) - g.Expect(err).To(g.MatchError(errors.ErrParentIfaceRequired)) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewNetworkInterface_fillChangedStatus(t *testing.T) { @@ -275,10 +285,12 @@ func TestNewNetworkInterface_fillChangedStatus(t *testing.T) { Times(1) _, err := step.Do(ctx) - g.Expect(err).To(g.BeNil()) g.Expect(status.MACAddress).To(g.Equal(reverseMACAddress)) g.Expect(status.HostDeviceName).To(g.Equal(expectedTapDeviceName)) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewNetworkInterface_createError(t *testing.T) { @@ -298,8 +310,8 @@ func TestNewNetworkInterface_createError(t *testing.T) { Times(0) step := network.NewNetworkInterface(vmid, iface, status, svc) - shouldDo, err := step.ShouldDo(ctx) + shouldDo, err := step.ShouldDo(ctx) g.Expect(err).To(g.BeNil()) g.Expect(shouldDo).To(g.BeTrue()) @@ -314,6 +326,8 @@ func TestNewNetworkInterface_createError(t *testing.T) { Times(1) _, err = step.Do(ctx) - g.Expect(err).To(g.MatchError(errors.ErrParentIfaceRequired)) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } diff --git a/core/steps/network/interface_delete_test.go b/core/steps/network/interface_delete_test.go index bf2c690a..26f71855 100644 --- a/core/steps/network/interface_delete_test.go +++ b/core/steps/network/interface_delete_test.go @@ -31,8 +31,8 @@ func TestDeleteNetworkInterface_doesNotExist(t *testing.T) { Times(1) step := network.DeleteNetworkInterface(vmid, iface, svc) - shouldDo, err := step.ShouldDo(ctx) + shouldDo, err := step.ShouldDo(ctx) g.Expect(err).To(g.BeNil()) g.Expect(shouldDo).To(g.BeFalse()) @@ -42,8 +42,10 @@ func TestDeleteNetworkInterface_doesNotExist(t *testing.T) { Times(1) _, err = step.Do(ctx) - g.Expect(err).To(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestDeleteNetworkInterface_emptyStatus(t *testing.T) { @@ -58,14 +60,16 @@ func TestDeleteNetworkInterface_emptyStatus(t *testing.T) { ctx := context.Background() step := network.DeleteNetworkInterface(vmid, iface, svc) - shouldDo, err := step.ShouldDo(ctx) + shouldDo, err := step.ShouldDo(ctx) g.Expect(err).To(g.BeNil()) g.Expect(shouldDo).To(g.BeFalse()) _, err = step.Do(ctx) - g.Expect(err).ToNot(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestDeleteNetworkInterface_exists(t *testing.T) { @@ -85,8 +89,8 @@ func TestDeleteNetworkInterface_exists(t *testing.T) { Times(1) step := network.DeleteNetworkInterface(vmid, iface, svc) - shouldDo, err := step.ShouldDo(ctx) + shouldDo, err := step.ShouldDo(ctx) g.Expect(err).To(g.BeNil()) g.Expect(shouldDo).To(g.BeTrue()) @@ -104,8 +108,10 @@ func TestDeleteNetworkInterface_exists(t *testing.T) { Times(1) _, err = step.Do(ctx) - g.Expect(err).To(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestDeleteNetworkInterface_exists_errorDeleting(t *testing.T) { @@ -125,8 +131,8 @@ func TestDeleteNetworkInterface_exists_errorDeleting(t *testing.T) { Times(1) step := network.DeleteNetworkInterface(vmid, iface, svc) - shouldDo, err := step.ShouldDo(ctx) + shouldDo, err := step.ShouldDo(ctx) g.Expect(err).To(g.BeNil()) g.Expect(shouldDo).To(g.BeTrue()) @@ -144,8 +150,10 @@ func TestDeleteNetworkInterface_exists_errorDeleting(t *testing.T) { Times(1) _, err = step.Do(ctx) - g.Expect(err).ToNot(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestDeleteNetworkInterface_IfaceExistsError(t *testing.T) { @@ -165,12 +173,14 @@ func TestDeleteNetworkInterface_IfaceExistsError(t *testing.T) { Times(2) step := network.DeleteNetworkInterface(vmid, iface, svc) - shouldDo, err := step.ShouldDo(ctx) + shouldDo, err := step.ShouldDo(ctx) g.Expect(err).ToNot(g.BeNil()) g.Expect(shouldDo).To(g.BeFalse()) _, err = step.Do(ctx) - g.Expect(err).To(g.MatchError(errors.ErrParentIfaceRequired)) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } diff --git a/core/steps/runtime/dir_create_test.go b/core/steps/runtime/dir_create_test.go index c1979afe..5402b359 100644 --- a/core/steps/runtime/dir_create_test.go +++ b/core/steps/runtime/dir_create_test.go @@ -6,6 +6,7 @@ import ( "testing" . "github.com/onsi/gomega" + g "github.com/onsi/gomega" "github.com/spf13/afero" "github.com/weaveworks/flintlock/core/steps/runtime" @@ -27,6 +28,9 @@ func TestCreateDirectory_NotExists(t *testing.T) { Expect(err).NotTo(HaveOccurred()) Expect(len(childSteps)).To(Equal(0)) + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) + testDirExists(t, testDir, dirMode, fs) } @@ -52,6 +56,9 @@ func TestCreateDirectory_Exists(t *testing.T) { Expect(err).NotTo(HaveOccurred()) Expect(len(childSteps)).To(Equal(0)) + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) + testDirExists(t, testDir, dirMode, fs) } @@ -78,6 +85,9 @@ func TestCreateDirectory_ExistsButChangeMode(t *testing.T) { Expect(err).NotTo(HaveOccurred()) Expect(len(childSteps)).To(Equal(0)) + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) + testDirExists(t, testDir, dirMode, fs) } diff --git a/core/steps/runtime/dir_delete_test.go b/core/steps/runtime/dir_delete_test.go index e4f50288..656b240c 100644 --- a/core/steps/runtime/dir_delete_test.go +++ b/core/steps/runtime/dir_delete_test.go @@ -5,14 +5,13 @@ import ( "os" "testing" - "github.com/onsi/gomega" + g "github.com/onsi/gomega" "github.com/spf13/afero" - "github.com/stretchr/testify/assert" "github.com/weaveworks/flintlock/core/steps/runtime" ) func TestDeleteDirectory_NotExists(t *testing.T) { - gomega.RegisterTestingT(t) + g.RegisterTestingT(t) testDir := "/test/or/not-to-test" @@ -22,15 +21,17 @@ func TestDeleteDirectory_NotExists(t *testing.T) { step := runtime.NewDeleteDirectory(testDir, fs) should, shouldErr := step.ShouldDo(ctx) extraSteps, doErr := step.Do(ctx) + verifyErr := step.Verify(ctx) - assert.NoError(t, shouldErr) - assert.False(t, should) - assert.NoError(t, doErr) - assert.Empty(t, extraSteps) + g.Expect(should).To(g.BeFalse()) + g.Expect(shouldErr).To(g.BeNil()) + g.Expect(doErr).To(g.BeNil()) + g.Expect(extraSteps).To(g.BeEmpty()) + g.Expect(verifyErr).To(g.BeNil()) } func TestDeleteDirectory_Exists(t *testing.T) { - gomega.RegisterTestingT(t) + g.RegisterTestingT(t) testDir := "/test/or/not-to-test" @@ -42,9 +43,11 @@ func TestDeleteDirectory_Exists(t *testing.T) { step := runtime.NewDeleteDirectory(testDir, fs) should, shouldErr := step.ShouldDo(ctx) extraSteps, doErr := step.Do(ctx) + verifyErr := step.Verify(ctx) - assert.NoError(t, shouldErr) - assert.True(t, should) - assert.NoError(t, doErr) - assert.Empty(t, extraSteps) + g.Expect(should).To(g.BeTrue()) + g.Expect(shouldErr).To(g.BeNil()) + g.Expect(doErr).To(g.BeNil()) + g.Expect(extraSteps).To(g.BeEmpty()) + g.Expect(verifyErr).To(g.BeNil()) } diff --git a/core/steps/runtime/initrd_mount_test.go b/core/steps/runtime/initrd_mount_test.go index 0e6887b7..65b6efe3 100644 --- a/core/steps/runtime/initrd_mount_test.go +++ b/core/steps/runtime/initrd_mount_test.go @@ -84,6 +84,9 @@ func TestInitrdMount(t *testing.T) { g.Expect(vm.Status.InitrdMount).To( g.BeEquivalentTo(&expectedMount), ) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestInitrdMount_noInitrd(t *testing.T) { @@ -103,6 +106,9 @@ func TestInitrdMount_noInitrd(t *testing.T) { g.Expect(shouldDo).To(g.BeFalse()) g.Expect(shouldErr).To(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestInitrdMount_statusAlreadySet(t *testing.T) { @@ -151,6 +157,9 @@ func TestInitrdMount_statusAlreadySet(t *testing.T) { g.Expect(shouldDo).To(g.BeFalse()) g.Expect(shouldErr).ToNot(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestInitrdMount_vmNotSet(t *testing.T) { @@ -171,6 +180,9 @@ func TestInitrdMount_vmNotSet(t *testing.T) { g.Expect(shouldErr).To(g.MatchError(internalerr.ErrSpecRequired)) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).To(g.MatchError(internalerr.ErrSpecRequired)) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestInitrdMount_pullAndMountError(t *testing.T) { @@ -193,6 +205,9 @@ func TestInitrdMount_pullAndMountError(t *testing.T) { g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).ToNot(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestInitrdMount_emptyResponse(t *testing.T) { @@ -215,4 +230,7 @@ func TestInitrdMount_emptyResponse(t *testing.T) { g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).To(g.MatchError(internalerr.ErrNoMount)) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } diff --git a/core/steps/runtime/kernel_mount_test.go b/core/steps/runtime/kernel_mount_test.go index b2a85018..70d12014 100644 --- a/core/steps/runtime/kernel_mount_test.go +++ b/core/steps/runtime/kernel_mount_test.go @@ -80,6 +80,9 @@ func TestKernelMount(t *testing.T) { g.Expect(vm.Status.KernelMount).To( g.BeEquivalentTo(&expectedMount), ) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestKernelMount_noKernel(t *testing.T) { @@ -103,6 +106,9 @@ func TestKernelMount_noKernel(t *testing.T) { g.Expect(doErr).To(g.MatchError(internalerr.ErrKernelImageRequired)) g.Expect(subSteps).To(g.BeEmpty()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestKernelMount_statusAlreadySet(t *testing.T) { @@ -151,6 +157,9 @@ func TestKernelMount_statusAlreadySet(t *testing.T) { g.Expect(shouldDo).To(g.BeFalse()) g.Expect(shouldErr).ToNot(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestKernelMount_vmNotSet(t *testing.T) { @@ -171,6 +180,9 @@ func TestKernelMount_vmNotSet(t *testing.T) { g.Expect(shouldErr).To(g.MatchError(internalerr.ErrSpecRequired)) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).To(g.MatchError(internalerr.ErrSpecRequired)) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestKernelMount_pullAndMountError(t *testing.T) { @@ -193,6 +205,9 @@ func TestKernelMount_pullAndMountError(t *testing.T) { g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).ToNot(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestKernelMount_emptyResponse(t *testing.T) { @@ -215,4 +230,7 @@ func TestKernelMount_emptyResponse(t *testing.T) { g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).To(g.MatchError(internalerr.ErrNoMount)) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } diff --git a/core/steps/runtime/repo_release_test.go b/core/steps/runtime/repo_release_test.go index 61d0de0e..652d64f6 100644 --- a/core/steps/runtime/repo_release_test.go +++ b/core/steps/runtime/repo_release_test.go @@ -62,6 +62,9 @@ func TestNewRepoRelease(t *testing.T) { g.Expect(shouldErr).To(g.BeNil()) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).To(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewRepoRelease_doesNotExist(t *testing.T) { @@ -84,6 +87,9 @@ func TestNewRepoRelease_doesNotExist(t *testing.T) { g.Expect(shouldDo).To(g.BeFalse()) g.Expect(shouldErr).To(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewRepoRelease_VMIsNotDefined(t *testing.T) { @@ -105,6 +111,9 @@ func TestNewRepoRelease_VMIsNotDefined(t *testing.T) { g.Expect(shouldErr).To(g.MatchError(internalerrors.ErrSpecRequired)) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).To(g.MatchError(internalerrors.ErrSpecRequired)) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewRepoRelease_existsCheckFails(t *testing.T) { @@ -127,6 +136,9 @@ func TestNewRepoRelease_existsCheckFails(t *testing.T) { g.Expect(shouldDo).To(g.BeFalse()) g.Expect(shouldErr).ToNot(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestNewRepoRelease_repoServiceError(t *testing.T) { @@ -157,4 +169,7 @@ func TestNewRepoRelease_repoServiceError(t *testing.T) { g.Expect(shouldErr).To(g.BeNil()) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).ToNot(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } diff --git a/core/steps/runtime/volume_mount_test.go b/core/steps/runtime/volume_mount_test.go index 85210431..af0d0c98 100644 --- a/core/steps/runtime/volume_mount_test.go +++ b/core/steps/runtime/volume_mount_test.go @@ -103,6 +103,9 @@ func TestVolumeMount(t *testing.T) { g.Expect(shouldErr).To(g.BeNil()) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).To(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } g.Expect(vm.Status.Volumes).To(g.HaveLen(2)) @@ -148,6 +151,9 @@ func TestVolumeMount_statusAlreadySetBoth(t *testing.T) { g.Expect(shouldErr).To(g.BeNil()) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).To(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } g.Expect(vm.Status.Volumes).To(g.HaveLen(2)) @@ -203,6 +209,9 @@ func TestVolumeMount_retry(t *testing.T) { g.Expect(shouldErr).To(g.BeNil()) g.Expect(subSteps).To(g.BeEmpty()) g.Expect(doErr).To(g.BeNil()) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } g.Expect(vm.Status.Volumes).To(g.HaveLen(2)) @@ -283,6 +292,9 @@ func TestVolumeMount_doError(t *testing.T) { g.Expect(extraSteps).To(g.BeEmpty()) g.Expect(doErr).To(g.MatchError(internalerr.ErrNoVolumeMount)) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } func TestVolumeMount_nilStatus(t *testing.T) { @@ -309,4 +321,7 @@ func TestVolumeMount_nilStatus(t *testing.T) { g.Expect(extraSteps).To(g.BeEmpty()) g.Expect(doErr).To(g.MatchError(internalerr.ErrMissingStatusInfo)) + + verifyErr := step.Verify(ctx) + g.Expect(verifyErr).To(g.BeNil()) } From a1f93d05ca73d9574b009d044f14bbf28b9af9c9 Mon Sep 17 00:00:00 2001 From: Balazs Nadasdi Date: Tue, 16 Nov 2021 16:16:01 +0100 Subject: [PATCH 05/11] typo fix --- core/application/reconcile.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/application/reconcile.go b/core/application/reconcile.go index adc95f1f..f6727e6d 100644 --- a/core/application/reconcile.go +++ b/core/application/reconcile.go @@ -98,7 +98,7 @@ func (a *app) reschedule(ctx context.Context, logger *logrus.Entry, spec *models go func(id, ns string, sleepTime time.Duration) { logger.Info("Wait to emit update") time.Sleep(sleepTime) - logger.Info("Emit pdate") + logger.Info("Emit update") _ = a.ports.EventService.Publish( context.Background(), From 741f74280111250484e48011475232d2fc26748f Mon Sep 17 00:00:00 2001 From: Balazs Nadasdi Date: Wed, 17 Nov 2021 11:27:28 +0100 Subject: [PATCH 06/11] Remove sleep from Start --- core/plans/const.go | 2 -- core/plans/microvm_create_update.go | 2 +- core/steps/microvm/start.go | 14 ++++---------- core/steps/microvm/start_test.go | 12 ++++++------ 4 files changed, 11 insertions(+), 19 deletions(-) diff --git a/core/plans/const.go b/core/plans/const.go index 32aad7fb..50f6e891 100644 --- a/core/plans/const.go +++ b/core/plans/const.go @@ -3,6 +3,4 @@ package plans const ( MicroVMDeletePlanName = "microvm_delete" MicroVMCreateOrUpdatePlanName = "microvm_create_update" - - microVMBootTime = 30 ) diff --git a/core/plans/microvm_create_update.go b/core/plans/microvm_create_update.go index 0da1ae84..238c5d77 100644 --- a/core/plans/microvm_create_update.go +++ b/core/plans/microvm_create_update.go @@ -75,7 +75,7 @@ func (p *microvmCreateOrUpdatePlan) Create(ctx context.Context) ([]planner.Proce } // MicroVM provider start - if err := p.addStep(ctx, microvm.NewStartStep(p.vm, ports.Provider, microVMBootTime)); err != nil { + if err := p.addStep(ctx, microvm.NewStartStep(p.vm, ports.Provider)); err != nil { return nil, fmt.Errorf("adding microvm start step: %w", err) } diff --git a/core/steps/microvm/start.go b/core/steps/microvm/start.go index 839681bc..3da0fa67 100644 --- a/core/steps/microvm/start.go +++ b/core/steps/microvm/start.go @@ -3,7 +3,6 @@ package microvm import ( "context" "fmt" - "time" "github.com/sirupsen/logrus" @@ -17,19 +16,16 @@ import ( func NewStartStep( vm *models.MicroVM, vmSvc ports.MicroVMService, - bootTime int, ) planner.Procedure { return &startStep{ - vm: vm, - vmSvc: vmSvc, - bootTime: bootTime, + vm: vm, + vmSvc: vmSvc, } } type startStep struct { - vm *models.MicroVM - vmSvc ports.MicroVMService - bootTime int + vm *models.MicroVM + vmSvc ports.MicroVMService } // Name is the name of the procedure/operation. @@ -76,8 +72,6 @@ func (s *startStep) Verify(ctx context.Context) error { "step": s.Name(), "vmid": s.vm.ID, }) - logger.Debug("waiting for the microvm to start") - time.Sleep(time.Duration(s.bootTime) * time.Second) logger.Debug("verify microvm is started") state, err := s.vmSvc.State(ctx, s.vm.ID.String()) diff --git a/core/steps/microvm/start_test.go b/core/steps/microvm/start_test.go index 40edae24..3fbc5968 100644 --- a/core/steps/microvm/start_test.go +++ b/core/steps/microvm/start_test.go @@ -44,7 +44,7 @@ func TestNewStartStep(t *testing.T) { ctx := context.Background() vm := testVMToStart() - step := microvm.NewStartStep(vm, microVMService, 1) + step := microvm.NewStartStep(vm, microVMService) microVMService. EXPECT(). @@ -94,7 +94,7 @@ func TestNewStartStep_StateCheck(t *testing.T) { ctx := context.Background() vm := testVMToStart() - step := microvm.NewStartStep(vm, microVMService, 1) + step := microvm.NewStartStep(vm, microVMService) for _, testCase := range stateTestCases { microVMService. @@ -118,7 +118,7 @@ func TestNewStartStep_StateCheckError(t *testing.T) { ctx := context.Background() vm := testVMToStart() - step := microvm.NewStartStep(vm, microVMService, 1) + step := microvm.NewStartStep(vm, microVMService) microVMService. EXPECT(). @@ -141,7 +141,7 @@ func TestNewStartStep_VMIsNotDefined(t *testing.T) { microVMService := mock.NewMockMicroVMService(mockCtrl) ctx := context.Background() - step := microvm.NewStartStep(vm, microVMService, 1) + step := microvm.NewStartStep(vm, microVMService) subSteps, err := step.Do(ctx) @@ -158,7 +158,7 @@ func TestNewStartStep_ServiceStartError(t *testing.T) { vm := testVMToStart() ctx := context.Background() - step := microvm.NewStartStep(vm, microVMService, 1) + step := microvm.NewStartStep(vm, microVMService) microVMService. EXPECT(). @@ -180,7 +180,7 @@ func TestNewStartStep_unableToBoot(t *testing.T) { vm := testVMToStart() ctx := context.Background() - step := microvm.NewStartStep(vm, microVMService, 1) + step := microvm.NewStartStep(vm, microVMService) microVMService. EXPECT(). From 2c2fd1ef34293c149cf30a760ee42c0c4fa07a0e Mon Sep 17 00:00:00 2001 From: Balazs Nadasdi Date: Wed, 17 Nov 2021 13:13:02 +0100 Subject: [PATCH 07/11] Revert "Remove sleep from Start" This reverts commit 741f74280111250484e48011475232d2fc26748f. --- core/plans/const.go | 2 ++ core/plans/microvm_create_update.go | 2 +- core/steps/microvm/start.go | 14 ++++++++++---- core/steps/microvm/start_test.go | 12 ++++++------ 4 files changed, 19 insertions(+), 11 deletions(-) diff --git a/core/plans/const.go b/core/plans/const.go index 50f6e891..32aad7fb 100644 --- a/core/plans/const.go +++ b/core/plans/const.go @@ -3,4 +3,6 @@ package plans const ( MicroVMDeletePlanName = "microvm_delete" MicroVMCreateOrUpdatePlanName = "microvm_create_update" + + microVMBootTime = 30 ) diff --git a/core/plans/microvm_create_update.go b/core/plans/microvm_create_update.go index 238c5d77..0da1ae84 100644 --- a/core/plans/microvm_create_update.go +++ b/core/plans/microvm_create_update.go @@ -75,7 +75,7 @@ func (p *microvmCreateOrUpdatePlan) Create(ctx context.Context) ([]planner.Proce } // MicroVM provider start - if err := p.addStep(ctx, microvm.NewStartStep(p.vm, ports.Provider)); err != nil { + if err := p.addStep(ctx, microvm.NewStartStep(p.vm, ports.Provider, microVMBootTime)); err != nil { return nil, fmt.Errorf("adding microvm start step: %w", err) } diff --git a/core/steps/microvm/start.go b/core/steps/microvm/start.go index 3da0fa67..839681bc 100644 --- a/core/steps/microvm/start.go +++ b/core/steps/microvm/start.go @@ -3,6 +3,7 @@ package microvm import ( "context" "fmt" + "time" "github.com/sirupsen/logrus" @@ -16,16 +17,19 @@ import ( func NewStartStep( vm *models.MicroVM, vmSvc ports.MicroVMService, + bootTime int, ) planner.Procedure { return &startStep{ - vm: vm, - vmSvc: vmSvc, + vm: vm, + vmSvc: vmSvc, + bootTime: bootTime, } } type startStep struct { - vm *models.MicroVM - vmSvc ports.MicroVMService + vm *models.MicroVM + vmSvc ports.MicroVMService + bootTime int } // Name is the name of the procedure/operation. @@ -72,6 +76,8 @@ func (s *startStep) Verify(ctx context.Context) error { "step": s.Name(), "vmid": s.vm.ID, }) + logger.Debug("waiting for the microvm to start") + time.Sleep(time.Duration(s.bootTime) * time.Second) logger.Debug("verify microvm is started") state, err := s.vmSvc.State(ctx, s.vm.ID.String()) diff --git a/core/steps/microvm/start_test.go b/core/steps/microvm/start_test.go index 3fbc5968..40edae24 100644 --- a/core/steps/microvm/start_test.go +++ b/core/steps/microvm/start_test.go @@ -44,7 +44,7 @@ func TestNewStartStep(t *testing.T) { ctx := context.Background() vm := testVMToStart() - step := microvm.NewStartStep(vm, microVMService) + step := microvm.NewStartStep(vm, microVMService, 1) microVMService. EXPECT(). @@ -94,7 +94,7 @@ func TestNewStartStep_StateCheck(t *testing.T) { ctx := context.Background() vm := testVMToStart() - step := microvm.NewStartStep(vm, microVMService) + step := microvm.NewStartStep(vm, microVMService, 1) for _, testCase := range stateTestCases { microVMService. @@ -118,7 +118,7 @@ func TestNewStartStep_StateCheckError(t *testing.T) { ctx := context.Background() vm := testVMToStart() - step := microvm.NewStartStep(vm, microVMService) + step := microvm.NewStartStep(vm, microVMService, 1) microVMService. EXPECT(). @@ -141,7 +141,7 @@ func TestNewStartStep_VMIsNotDefined(t *testing.T) { microVMService := mock.NewMockMicroVMService(mockCtrl) ctx := context.Background() - step := microvm.NewStartStep(vm, microVMService) + step := microvm.NewStartStep(vm, microVMService, 1) subSteps, err := step.Do(ctx) @@ -158,7 +158,7 @@ func TestNewStartStep_ServiceStartError(t *testing.T) { vm := testVMToStart() ctx := context.Background() - step := microvm.NewStartStep(vm, microVMService) + step := microvm.NewStartStep(vm, microVMService, 1) microVMService. EXPECT(). @@ -180,7 +180,7 @@ func TestNewStartStep_unableToBoot(t *testing.T) { vm := testVMToStart() ctx := context.Background() - step := microvm.NewStartStep(vm, microVMService) + step := microvm.NewStartStep(vm, microVMService, 1) microVMService. EXPECT(). From 6476409cd31f4108212b1ea9f466a370f24964ec Mon Sep 17 00:00:00 2001 From: Balazs Nadasdi Date: Wed, 17 Nov 2021 13:14:40 +0100 Subject: [PATCH 08/11] sleep only 5s --- core/plans/const.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/plans/const.go b/core/plans/const.go index 32aad7fb..67ae7300 100644 --- a/core/plans/const.go +++ b/core/plans/const.go @@ -4,5 +4,5 @@ const ( MicroVMDeletePlanName = "microvm_delete" MicroVMCreateOrUpdatePlanName = "microvm_create_update" - microVMBootTime = 30 + microVMBootTime = 5 ) From bd7138eaa5096e8adf40f59cf7c4f47f01f4aaac Mon Sep 17 00:00:00 2001 From: Balazs Nadasdi Date: Wed, 17 Nov 2021 15:01:42 +0100 Subject: [PATCH 09/11] address comments --- core/models/microvm.go | 2 +- core/steps/microvm/start.go | 16 ++++++++-------- pkg/planner/planner.go | 7 ++++++- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/core/models/microvm.go b/core/models/microvm.go index f8f9d4f3..1515700f 100644 --- a/core/models/microvm.go +++ b/core/models/microvm.go @@ -59,7 +59,7 @@ type MicroVMStatus struct { NetworkInterfaces NetworkInterfaceStatuses `json:"network_interfaces"` // Retry is a counter about how many times we retried to reconcile. Retry int `json:"retry"` - // DeletedAt indicates the time the microvm was marked as deleted. + // NotBefore tells the system to do not reconsile until given timestamp. NotBefore int64 `json:"not_before" validate:"omitempty"` } diff --git a/core/steps/microvm/start.go b/core/steps/microvm/start.go index 839681bc..607687f3 100644 --- a/core/steps/microvm/start.go +++ b/core/steps/microvm/start.go @@ -17,19 +17,19 @@ import ( func NewStartStep( vm *models.MicroVM, vmSvc ports.MicroVMService, - bootTime int, + bootWaitTimeSeconds int, ) planner.Procedure { return &startStep{ - vm: vm, - vmSvc: vmSvc, - bootTime: bootTime, + vm: vm, + vmSvc: vmSvc, + bootWaitTimeSeconds: bootWaitTimeSeconds, } } type startStep struct { - vm *models.MicroVM - vmSvc ports.MicroVMService - bootTime int + vm *models.MicroVM + vmSvc ports.MicroVMService + bootWaitTimeSeconds int } // Name is the name of the procedure/operation. @@ -77,7 +77,7 @@ func (s *startStep) Verify(ctx context.Context) error { "vmid": s.vm.ID, }) logger.Debug("waiting for the microvm to start") - time.Sleep(time.Duration(s.bootTime) * time.Second) + time.Sleep(time.Duration(s.bootWaitTimeSeconds) * time.Second) logger.Debug("verify microvm is started") state, err := s.vmSvc.State(ctx, s.vm.ID.String()) diff --git a/pkg/planner/planner.go b/pkg/planner/planner.go index 8a885315..d60984d6 100644 --- a/pkg/planner/planner.go +++ b/pkg/planner/planner.go @@ -25,6 +25,11 @@ type Procedure interface { Do(ctx context.Context) ([]Procedure, error) // ShouldDo determines if this procedure should be executed ShouldDo(ctx context.Context) (bool, error) - // Verify the state after Do. + // Verify the state after Do. Most cases it can return nil + // without doing anything, but in special cases we want to measure + // resources if they are in the desired state. + // Example: When we start MicroVM, it may does not tell us if it was + // successful or not, in Verify we can verify if it's running or not + // and report back an error if the state is not the desired state. Verify(ctx context.Context) error } From 5a71a81d4c1c3474662ad15b8d129ceea26a4f44 Mon Sep 17 00:00:00 2001 From: Balazs Nadasdi Date: Thu, 18 Nov 2021 17:08:18 +0100 Subject: [PATCH 10/11] address some of the comments --- core/application/reconcile.go | 13 +++++++------ core/models/microvm.go | 2 +- core/steps/microvm/start_test.go | 14 ++++++++------ 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/core/application/reconcile.go b/core/application/reconcile.go index f6727e6d..ee454e92 100644 --- a/core/application/reconcile.go +++ b/core/application/reconcile.go @@ -92,15 +92,13 @@ func (a *app) reschedule(ctx context.Context, logger *logrus.Entry, spec *models ) if _, err := a.ports.Repo.Save(ctx, spec); err != nil { - return fmt.Errorf("saving spec after plan failed: %w", err) + return fmt.Errorf("saving spec failed: %w", err) } go func(id, ns string, sleepTime time.Duration) { - logger.Info("Wait to emit update") time.Sleep(sleepTime) - logger.Info("Emit update") - _ = a.ports.EventService.Publish( + err := a.ports.EventService.Publish( context.Background(), defaults.TopicMicroVMEvents, &events.MicroVMSpecUpdated{ @@ -108,6 +106,9 @@ func (a *app) reschedule(ctx context.Context, logger *logrus.Entry, spec *models Namespace: ns, }, ) + if err != nil { + logger.Errorf("failed to publish an update event for %s/%s", ns, id) + } }(spec.ID.Name(), spec.ID.Namespace(), waitTime) return nil @@ -136,7 +137,7 @@ func (a *app) reconcile(ctx context.Context, spec *models.MicroVM, logger *logru executionID, err := a.ports.IdentifierService.GenerateRandom() if err != nil { if scheduleErr := a.reschedule(ctx, localLogger, spec); scheduleErr != nil { - return fmt.Errorf("saving spec after plan failed: %w", scheduleErr) + return fmt.Errorf("rescheduling failed: %w", scheduleErr) } return fmt.Errorf("generating plan execution id: %w", err) @@ -147,7 +148,7 @@ func (a *app) reconcile(ctx context.Context, spec *models.MicroVM, logger *logru stepCount, err := actuator.Execute(execCtx, plan, executionID) if err != nil { if scheduleErr := a.reschedule(ctx, localLogger, spec); scheduleErr != nil { - return fmt.Errorf("saving spec after plan failed: %w", scheduleErr) + return fmt.Errorf("rescheduling failed: %w", scheduleErr) } return fmt.Errorf("executing plan: %w", err) diff --git a/core/models/microvm.go b/core/models/microvm.go index 1515700f..ea439236 100644 --- a/core/models/microvm.go +++ b/core/models/microvm.go @@ -59,7 +59,7 @@ type MicroVMStatus struct { NetworkInterfaces NetworkInterfaceStatuses `json:"network_interfaces"` // Retry is a counter about how many times we retried to reconcile. Retry int `json:"retry"` - // NotBefore tells the system to do not reconsile until given timestamp. + // NotBefore tells the system to do not reconcile until given timestamp. NotBefore int64 `json:"not_before" validate:"omitempty"` } diff --git a/core/steps/microvm/start_test.go b/core/steps/microvm/start_test.go index 40edae24..f76abd1a 100644 --- a/core/steps/microvm/start_test.go +++ b/core/steps/microvm/start_test.go @@ -14,6 +14,8 @@ import ( "github.com/weaveworks/flintlock/infrastructure/mock" ) +const bootTimeInSeconds = 1 + func testVMToStart() *models.MicroVM { vmid, _ := models.NewVMID("vm", "ns") return &models.MicroVM{ @@ -44,7 +46,7 @@ func TestNewStartStep(t *testing.T) { ctx := context.Background() vm := testVMToStart() - step := microvm.NewStartStep(vm, microVMService, 1) + step := microvm.NewStartStep(vm, microVMService, bootTimeInSeconds) microVMService. EXPECT(). @@ -94,7 +96,7 @@ func TestNewStartStep_StateCheck(t *testing.T) { ctx := context.Background() vm := testVMToStart() - step := microvm.NewStartStep(vm, microVMService, 1) + step := microvm.NewStartStep(vm, microVMService, bootTimeInSeconds) for _, testCase := range stateTestCases { microVMService. @@ -118,7 +120,7 @@ func TestNewStartStep_StateCheckError(t *testing.T) { ctx := context.Background() vm := testVMToStart() - step := microvm.NewStartStep(vm, microVMService, 1) + step := microvm.NewStartStep(vm, microVMService, bootTimeInSeconds) microVMService. EXPECT(). @@ -141,7 +143,7 @@ func TestNewStartStep_VMIsNotDefined(t *testing.T) { microVMService := mock.NewMockMicroVMService(mockCtrl) ctx := context.Background() - step := microvm.NewStartStep(vm, microVMService, 1) + step := microvm.NewStartStep(vm, microVMService, bootTimeInSeconds) subSteps, err := step.Do(ctx) @@ -158,7 +160,7 @@ func TestNewStartStep_ServiceStartError(t *testing.T) { vm := testVMToStart() ctx := context.Background() - step := microvm.NewStartStep(vm, microVMService, 1) + step := microvm.NewStartStep(vm, microVMService, bootTimeInSeconds) microVMService. EXPECT(). @@ -180,7 +182,7 @@ func TestNewStartStep_unableToBoot(t *testing.T) { vm := testVMToStart() ctx := context.Background() - step := microvm.NewStartStep(vm, microVMService, 1) + step := microvm.NewStartStep(vm, microVMService, bootTimeInSeconds) microVMService. EXPECT(). From 0b2566b34ed6d4bdec8b1db990a0cc9aca384303 Mon Sep 17 00:00:00 2001 From: Balazs Nadasdi Date: Thu, 18 Nov 2021 17:09:47 +0100 Subject: [PATCH 11/11] Apply suggestions from code review Co-authored-by: Claudia --- core/application/reconcile.go | 2 +- core/steps/microvm/start_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/core/application/reconcile.go b/core/application/reconcile.go index ee454e92..ddc7c599 100644 --- a/core/application/reconcile.go +++ b/core/application/reconcile.go @@ -85,7 +85,7 @@ func (a *app) reschedule(ctx context.Context, logger *logrus.Entry, spec *models spec.Status.NotBefore = time.Now().Add(waitTime).Unix() logger.Infof( - "[%d/%d] reconciliation failed, reschedule at %s", + "[%d/%d] reconciliation failed, rescheduled for next attempt at %s", spec.Status.Retry, a.cfg.MaximumRetry, time.Unix(spec.Status.NotBefore, 0), diff --git a/core/steps/microvm/start_test.go b/core/steps/microvm/start_test.go index f76abd1a..dd09a8eb 100644 --- a/core/steps/microvm/start_test.go +++ b/core/steps/microvm/start_test.go @@ -195,10 +195,10 @@ func TestNewStartStep_unableToBoot(t *testing.T) { Return(ports.MicroVMStateUnknown, nil) subSteps, err := step.Do(ctx) - verifyErr := step.Verify(ctx) - - g.Expect(subSteps).To(g.BeEmpty()) g.Expect(err).To(g.BeNil()) + g.Expect(subSteps).To(g.BeEmpty()) + + verifyErr := step.Verify(ctx) g.Expect(verifyErr).To(g.MatchError(internalerr.ErrUnableToBoot)) microVMService.