feat: Add retry counter on failed reconciliation #255

yitsushi · 2021-11-12T14:48:15Z

What this PR does / why we need it:

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.

Which issue(s) this PR fixes:
Fixes #232
Fixes #178

Special notes for your reviewer:

Checklist:

squashed commits into logical changes
includes documentation
adds unit tests
adds or updates e2e tests

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.

If one fails, we can still listen on new requests and reconcile vms, if they are failing always, the retry logic will handle this.

yitsushi · 2021-11-16T10:17:23Z

Did not add test on reconsile.go, we have zero tests for it and I think it's out of scope to write the full test for it, and filed an issue #264

codecov-commenter · 2021-11-16T12:46:13Z

Codecov Report

Merging #255 (339ca11) into main (aa5916e) will increase coverage by 0.24%.
The diff coverage is 45.45%.

@@            Coverage Diff             @@
##             main     #255      +/-   ##
==========================================
+ Coverage   40.29%   40.54%   +0.24%     
==========================================
  Files          46       46              
  Lines        2169     2242      +73     
==========================================
+ Hits          874      909      +35     
- Misses       1236     1272      +36     
- Partials       59       61       +2

Impacted Files	Coverage Δ
core/application/reconcile.go	`0.00% <0.00%> (ø)`
core/plans/microvm_create_update.go	`57.53% <0.00%> (ø)`
infrastructure/controllers/microvm_controller.go	`70.00% <0.00%> (ø)`
pkg/planner/actuator.go	`83.33% <0.00%> (-2.88%)`	⬇️
core/steps/event/publish.go	`100.00% <100.00%> (ø)`
core/steps/microvm/create.go	`100.00% <100.00%> (ø)`
core/steps/microvm/delete.go	`100.00% <100.00%> (ø)`
core/steps/microvm/start.go	`100.00% <100.00%> (ø)`
core/steps/network/interface_create.go	`93.44% <100.00%> (+0.22%)`	⬆️
core/steps/network/interface_delete.go	`91.30% <100.00%> (+0.39%)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e0abc6...339ca11. Read the comment docs.

core/application/reconcile.go

core/steps/microvm/start.go

This reverts commit 741f742.

richardcase

Just a few nit...feel free to resolve them depending on what you think.

core/models/microvm.go

core/steps/microvm/start.go

pkg/planner/planner.go

Callisto13 · 2021-11-18T15:15:06Z

core/steps/microvm/start_test.go

@@ -44,25 +44,32 @@ func TestNewStartStep(t *testing.T) {
 	ctx := context.Background()
 	vm := testVMToStart()

-	step := microvm.NewStartStep(vm, microVMService)
+	step := microvm.NewStartStep(vm, microVMService, 1)


can we var these 1s here? just so it is easy to know at a glance what this param is meant to be

Callisto13 · 2021-11-18T15:16:12Z

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"`
+	// NotBefore tells the system to do not reconsile until given timestamp.


teeny nit: reconcile with a c

core/steps/microvm/start_test.go

core/application/reconcile.go

Callisto13 · 2021-11-18T15:33:21Z

core/application/reconcile.go

+		logger.Info("Wait to emit update")
+		time.Sleep(sleepTime)
+		logger.Info("Emit update")


do we want to be a bit more clear on these logs?

like "waiting to publish update event" or "waiting to reschedule for update"

I don't think we even need these lines, added them while i was working on it.

Callisto13 · 2021-11-18T15:34:27Z

core/application/reconcile.go

+		time.Sleep(sleepTime)
+		logger.Info("Emit update")
+
+		_ = a.ports.EventService.Publish(


do we not care about logging this err? could imagine that being an annoying one to solve if it failed

Yes, intentionally ignored it. This go routine is a steroid on the system, if it fails next resync will do the trick. But we can log. Hopefully ppl don't get confused reading the log because it happens "random" as it's not in the flow (async go routine).

Callisto13 · 2021-11-18T15:38:09Z

core/application/reconcile.go

 	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)


more relevant error message? (ditto line 150)

…32-retry-fail

Co-authored-by: Claudia <[email protected]>

Callisto13

nice work 🎉

yitsushi added the kind/feature New feature or request label Nov 12, 2021

yitsushi added 2 commits November 15, 2021 11:17

Fix test // will do proper test later

e1c3626

yitsushi force-pushed the 232-retry-fail branch from ae70166 to 90b7bed Compare November 15, 2021 10:17

Stop hard failing resync on boot

e48e1d7

If one fails, we can still listen on new requests and reconcile vms, if they are failing always, the retry logic will handle this.

yitsushi force-pushed the 232-retry-fail branch from 90b7bed to e48e1d7 Compare November 15, 2021 10:19

Test

7ec6979

yitsushi force-pushed the 232-retry-fail branch from 6ab67a2 to 7ec6979 Compare November 16, 2021 10:20

yitsushi marked this pull request as ready for review November 16, 2021 10:22

Merge branch 'main' into 232-retry-fail

9b5c3c4

richardcase reviewed Nov 16, 2021

View reviewed changes

core/application/reconcile.go Outdated Show resolved Hide resolved

core/application/reconcile.go Show resolved Hide resolved

core/steps/microvm/start.go Outdated Show resolved Hide resolved

yitsushi added 4 commits November 16, 2021 16:17

typo fix

a1f93d0

Remove sleep from Start

741f742

Revert "Remove sleep from Start"

2c2fd1e

This reverts commit 741f742.

sleep only 5s

6476409

richardcase mentioned this pull request Nov 17, 2021

VM Supervisor #198

Open

4 tasks

richardcase reviewed Nov 17, 2021

View reviewed changes

core/models/microvm.go Outdated Show resolved Hide resolved

core/steps/microvm/start.go Outdated Show resolved Hide resolved

pkg/planner/planner.go Outdated Show resolved Hide resolved

yitsushi added 4 commits November 17, 2021 15:01

address comments

bd7138e

Merge branch 'main' into 232-retry-fail

9f98995

Merge branch 'main' into 232-retry-fail

1634349

Merge branch 'main' into 232-retry-fail

339ca11

Callisto13 reviewed Nov 18, 2021

View reviewed changes

yitsushi and others added 3 commits November 18, 2021 17:08

address some of the comments

5a71a81

Merge branch '232-retry-fail' of github.com:yitsushi/flintlock into 2…

cf3ee93

…32-retry-fail

Apply suggestions from code review

0b2566b

Co-authored-by: Claudia <[email protected]>

yitsushi requested review from richardcase and Callisto13 November 18, 2021 16:10

Callisto13 approved these changes Nov 19, 2021

View reviewed changes

yitsushi merged commit 6ec5c7d into liquidmetal-dev:main Nov 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add retry counter on failed reconciliation #255

feat: Add retry counter on failed reconciliation #255

yitsushi commented Nov 12, 2021 •

edited

Loading

yitsushi commented Nov 16, 2021

codecov-commenter commented Nov 16, 2021 •

edited

Loading

richardcase left a comment

Callisto13 Nov 18, 2021

Callisto13 Nov 18, 2021

Callisto13 Nov 18, 2021

yitsushi Nov 18, 2021

Callisto13 Nov 18, 2021

yitsushi Nov 18, 2021

Callisto13 Nov 18, 2021

Callisto13 left a comment

feat: Add retry counter on failed reconciliation #255

feat: Add retry counter on failed reconciliation #255

Conversation

yitsushi commented Nov 12, 2021 • edited Loading

yitsushi commented Nov 16, 2021

codecov-commenter commented Nov 16, 2021 • edited Loading

Codecov Report

richardcase left a comment

Choose a reason for hiding this comment

Callisto13 Nov 18, 2021

Choose a reason for hiding this comment

Callisto13 Nov 18, 2021

Choose a reason for hiding this comment

Callisto13 Nov 18, 2021

Choose a reason for hiding this comment

yitsushi Nov 18, 2021

Choose a reason for hiding this comment

Callisto13 Nov 18, 2021

Choose a reason for hiding this comment

yitsushi Nov 18, 2021

Choose a reason for hiding this comment

Callisto13 Nov 18, 2021

Choose a reason for hiding this comment

Callisto13 left a comment

Choose a reason for hiding this comment

yitsushi commented Nov 12, 2021 •

edited

Loading

codecov-commenter commented Nov 16, 2021 •

edited

Loading