Update VM state pre migration #127
Conversation
stu-gott
changed the title
pkg/virt-controller/watch/pod.go
Outdated
| queue.AddRateLimited(key) | ||
| return true | ||
| } | ||
| if putVm(vm, restClient, queue, key) {return true} |
There was a problem hiding this comment.
I would prefer
if err := putVM(); err != nil {
return true
}
pkg/virt-controller/watch/pod.go
Outdated
|
|
||
| } | ||
| logger.V(3).Info().Msg("Enqueuing VM again.") | ||
| queue.AddRateLimited(key) |
There was a problem hiding this comment.
Could you move the queue logic out here back to the controller loop?
There was a problem hiding this comment.
So we don't need to re-try on failure for the "Starting migration" case?
There was a problem hiding this comment.
We do. But I would prefer if you return the error and do the queue.AddRateLimited in the main controller loop.
pkg/virt-controller/watch/pod.go
Outdated
| } else if vm.Status.Phase == corev1.Running { | ||
| vmCopy := corev1.VM{} | ||
| model.Copy(&vmCopy, vm) | ||
| vmCopy.Status.MigrationNodeName = vm.Status.NodeName |
There was a problem hiding this comment.
That needs to be vmCopy.Status.MigrationNodeName = pod.Status.NodeName
pkg/virt-controller/watch/pod.go
Outdated
| logger.Info().Msgf("VM successfully scheduled to %s.", vmCopy.Status.NodeName) | ||
| } else if vm.Status.Phase == corev1.Running { | ||
| vmCopy := corev1.VM{} | ||
| model.Copy(&vmCopy, vm) |
There was a problem hiding this comment.
In the case of coping from one object to another object of the same type you can use the kubernetes copier api.Scheme.Copy(). It does the same thing but looks nicer ;)
pkg/virt-controller/watch/pod.go
Outdated
| model.Copy(&vmCopy, vm) | ||
| vmCopy.Status.MigrationNodeName = vm.Status.NodeName | ||
| logger := logging.DefaultLogger() | ||
| // TODO: the migration should be started here |
There was a problem hiding this comment.
It should be started after the VM put.
pkg/virt-controller/watch/pod.go
Outdated
|
|
||
| } | ||
| logger.V(3).Info().Msg("Enqueuing VM again.") | ||
| queue.AddRateLimited(key) |
There was a problem hiding this comment.
We do. But I would prefer if you return the error and do the queue.AddRateLimited in the main controller loop.
pkg/virt-controller/watch/pod.go
Outdated
| } | ||
| logger.V(3).Info().Msg("Enqueuing VM again.") | ||
| queue.AddRateLimited(key) | ||
| return nil |
|
One test is currently failing. I've changed something with respect to the original pod scheduling behavior. |
pkg/virt-controller/watch/pod.go
Outdated
| } | ||
|
|
||
| } | ||
| return fmt.Errorf("failed to set vm state: %v", err) |
There was a problem hiding this comment.
Can't you just return the initial err here?
pkg/virt-controller/watch/pod.go
Outdated
| obj, err := kubeapi.Scheme.Copy(vm) | ||
| if err != nil { | ||
| logger.Error().Reason(err).Msg("could not copy vm object") | ||
| // FIXME: should the VM be re-enqueued? failure to copy is a pretty big deal |
There was a problem hiding this comment.
Not sure myself here. Probably rate limitting a few times and then dismiss the key and forget about it's enqueue history ... (queue.Forget())
pkg/virt-controller/watch/pod.go
Outdated
| queue.AddRateLimited(key) | ||
| return true | ||
| } | ||
| // TODO: the migration should be started here | ||
| logger.Info().Msgf("VM successfully scheduled to %s.", vmCopy.Status.NodeName) |
stu-gott
force-pushed
the
migrate-vm-status
branch
from
8841461
to
9666d6e
Compare
stu-gott
changed the title
|
Failing test was caused by erroneously passing a pointer by reference |
stu-gott
force-pushed
the
migrate-vm-status
branch
from
9666d6e
to
8dc699f
Compare
Change default pull policy to IfNotPresent to support offline demoing.
* Add 1.15.1 provider * Fix nodes startup script * Remove outdated comments * Add shasum
@rmohr