-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Live migration #29
Conversation
Codecov Report
@@ Coverage Diff @@
## main #29 +/- ##
==========================================
- Coverage 35.72% 31.13% -4.59%
==========================================
Files 3 5 +2
Lines 949 1275 +326
==========================================
+ Hits 339 397 +58
- Misses 560 818 +258
- Partials 50 60 +10
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
ffcc138
to
60822eb
Compare
pkg/daemon/vm_controller.go
Outdated
} | ||
case virtv1alpha1.VirtualMachineMigrationSent: | ||
if vm.Status.Migration.TargetNodeName == r.NodeName { | ||
// TODO handle VmNotCreated error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solve this TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't know the VM is creating or it will never be created(maybe something wrong). raise the error might be a better choice now.
and now we just raise an error during VM is migrating instead of return a ctrl.Result
with requeue. maybe we need another PR to fix all these questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can capture certain type of errors in the main reconcile func and convert it to a requeue. But yeah, it can be another commit
37875ee
to
93e4c72
Compare
Recorder record.EventRecorder | ||
} | ||
|
||
// +kubebuilder:rbac:groups=virt.virtink.smartx.com,resources=virtualmachinemigrations,verbs=get;list;watch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full CRUD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the controller doesn't need create/update/delete VMM. keep the permissions less.
93e4c72
to
8e9aead
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.