Simplify vm key update code path #243
Conversation
pkg/virt-handler/vm.go
Outdated
| if fetchedVM.(*v1.VM).Status.MigrationNodeName == d.host { | ||
| return true, nil | ||
| } | ||
| } else if result.Error().(*errors.StatusError).Status().Code != int32(http.StatusNotFound) { |
There was a problem hiding this comment.
Since you are already refactoring, could you change this line to !errors.IsNotFound(result.Error())? You can find errors it in the client-go library.
| func (d *VMHandlerDispatch) isMigrationDestination(vmName string) (bool, error) { | ||
|
|
||
| // If we don't have the VM in the cache, it could be that it is currently migrating to us | ||
| result := d.restClient.Get().Name(vmName).Resource("vms").Namespace(kubeapi.NamespaceDefault).Do() |
There was a problem hiding this comment.
It would probably also become easier to read, when we start using our Kubevirt client. If you want to integrate it, instead of the rest client, you can find the VM core client in pkg/kubecli/kubevirt.go. An instance can be obtained via kubecli.GetKubevirtClient() (instead of the kubecli.GetRESTClient()).
There was a problem hiding this comment.
yup, i addressed this in a second patch.
pkg/virt-handler/vm.go
Outdated
| queue.AddRateLimited(key) | ||
| return | ||
| } else if isDestination { |
There was a problem hiding this comment.
The error handling block returns so this doesn't need to be an "else" block. I'd find the code easier to follow without it, but that's simply personal preference.
pkg/virt-handler/vm.go
Outdated
| @@ -226,6 +189,73 @@ func MapPersistentVolumes(vm *v1.VM, restClient cache.Getter, namespace string) | |||
| return vmCopy, nil | |||
| } | |||
|
|
|||
| func (d *VMHandlerDispatch) processVmUpdate(vm *v1.VM, exists bool) error { | |||
There was a problem hiding this comment.
How would you feel about reversing the tense of the "exists" boolean passed into this function? I think it might convey more meaning as a function parameter if it were named something along the lines of "delete" or "shouldDelete" etc...
There was a problem hiding this comment.
yep, makes sense to me.
davidvossel
force-pushed
the
vm-execute-refactor
branch
3 times, most recently
from
70f45d2
to
3abe722
Compare
|
Okay, patch has been updated to address feedback and fix the regression test failure. I opted not to include the work I began on replacing the restClient with the virt core client. That work started to become more involved than just a refactor. I may submit it in a later pull request. |
|
I was able to get 27/27 of the functional tests pass as well with this change :-) |
|
Edit: Sorry go ahead. A test case failed that I'd never seen before so I just wanted to verify it was a transient. Nice work @davidvossel |
|
@stu-gott I've noticed a few transient failures with console related functional tests. |
|
👍 |
…update-email MAINTAINERS: Update @tomdee email address
Hopefully this kind of patch is welcome :-)
I encountered this during a code walk through. I'm trying to become familiar with some of the more critical code paths so I'll have a better overall understanding of how the system performs.
The vm Execute function was pretty difficult for me to follow. Functionally this patch does not change anything. I do believe it is a worthwhile code quality improvement though.