feat: add control loop framework for microvm reconciliation #75
Conversation
richardcase
changed the title
richardcase
force-pushed
the
control_loop
branch
2 times, most recently
from
3af7628 to
c59c6f2
Compare
richardcase
changed the title
The adds the control loop that is used to kick off microvm reconciliation. The actual reconciliation is empty and will be implemented in later changes. This change also introduces a value object `VMID` that i sused to represent the identity of a microvm. Signed-off-by: Richard Case <[email protected]>
richardcase
force-pushed
the
control_loop
branch
from
c59c6f2 to
251f73a
Compare
| @@ -7,7 +7,7 @@ import "github.com/weaveworks/reignite/core/ports" | |||
| type App interface { | |||
| ports.MicroVMCommandUseCases | |||
| ports.MicroVMQueryUseCases | |||
| // ports.ReconcileMicroVMsUseCase | |||
There was a problem hiding this comment.
This has been uncommented.
core/models/vmid_test.go
Outdated
| data, err := json.Marshal(id1) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| t.Logf("marshalled id to %s", string(data)) |
There was a problem hiding this comment.
I don't think you need to leave these in, but if you do, %s will stringify the data, without string(data).
There's another one of these below.
core/models/microvm.go
Outdated
| ID string `json:"id"` | ||
| // Namespace is the namespace for the microvm. | ||
| Namespace string `json:"namespace"` | ||
| ID *VMID `json:"id"` |
There was a problem hiding this comment.
Does this need to be a pointer?
Do you need to be able to differentiate between unset "" and set to empty string "" ?
There was a problem hiding this comment.
Not using a pointer would be preferable. I could add an Empty function instead.
There was a problem hiding this comment.
Added a function to indicate the ID is empty.
| ) | ||
|
|
||
| // VMID represents the identifier for a microvm. | ||
| type VMID struct { |
There was a problem hiding this comment.
Could this be an alias for types.NamespacedName ?
There was a problem hiding this comment.
Yes it could but i'm trying not to add any reference to Kubernetes in reignite. Plus i'm not 100% certain that it will stay as ns/name
There was a problem hiding this comment.
I've left this as is.
| var name, namespace string | ||
| switch v := envelope.Event.(type) { | ||
| case *events.MicroVMSpecCreated: | ||
| created, _ := envelope.Event.(*events.MicroVMSpecCreated) |
There was a problem hiding this comment.
You don't need to do this, v is of the type in the case https://tour.golang.org/methods/16
So, you could simplify this to:
case *events.MicroVMSpecCreated:
name = v.ID
namespace = v.NamespaceI wonder if there's not an interface for Events missing here, maybe with Name() and Namespace() or possibly (because namespace/name are indivisible) NamespacedName() ?
With an interface, you wouldn't need the type switch?
There was a problem hiding this comment.
I did consider this and its probably worth thinking about it again. I wasn't sure whether the fields would expand.
There was a problem hiding this comment.
In the future, it's likely the list of events that will cause a reconciliation will be added to and some will be outside our control (i.e. containerd events).
using v was more for the default case but it could be removed.
There was a problem hiding this comment.
The v would let you avoid doing the second type-assertion.
| // it will be ignored. | ||
| Enqueue(item interface{}) | ||
| // Dequeue will get an item from the queue. If there are no items on the queue then it will wait. | ||
| Dequeue() (interface{}, bool) |
There was a problem hiding this comment.
Should this accept a timeout or context?
There was a problem hiding this comment.
The Dequeue will wait. However, if Shutdown is called on the queue, then it will cause this to return. As this is based on the client-go workqueue i kept this functionality the same.
Signed-off-by: Richard Case <[email protected]>
What this PR does / why we need it:
The adds the control loop that is used to kick off microvm reconciliation. The actual reconciliation is empty and will be
implemented in later changes.
This change also introduces a value object
VMIDthat is used to represent the identity of a microvm.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #30
Fixes #76
Special notes for your reviewer:
Checklist:
Release note: