Check the VM status using virtctl instead of the SSH protocol #916
Conversation
ebattat
force-pushed
the
add_vm_ssh_retry
branch
2 times, most recently
from
039c08b to
545ae77
Compare
ebattat
changed the title
ebattat
force-pushed
the
add_vm_ssh_retry
branch
from
545ae77 to
b6dc9db
Compare
| ssh_status = self._oc.get_vm_ssh_status(vm_name, node_ip, vm_node_port) | ||
| vm_ssh = 1 if ssh_status == 'True' else 0 | ||
| ssh_status = self._oc.get_vm_virtctl_status(vm_name) | ||
| vm_ssh = 1 if ssh_status == 'True' else 0 |
There was a problem hiding this comment.
Why do you need this separately from ssh_status? You can always use int(vm_ssh) for the same purpose.
There was a problem hiding this comment.
In case of error we return the error message:
virtualmachineinstance.kubevirt.io "windows-vm-65265214-0" not found
kex_exchange_identification: Connection closed by remote host
There was a problem hiding this comment.
That's fine, but why does vm_ssh need to be distinct from ssh_status?
There was a problem hiding this comment.
Oh, I see...it would be cleaner to return the boolean status and the message as two return values:
if ok:
return (True, '')
else:
return (False, 'This is why it failed')
There was a problem hiding this comment.
We need 2 parameters:
- vm_ssh: True/False - if ssh passed successfully (Filtering in Grafana only error msg)
- ssh_status: error msg in case of error
There was a problem hiding this comment.
Exactly; don't overload them.
There was a problem hiding this comment.
I prefer to keep it separately for Grafana dashboard
vm_ssh: int
ssh_status: str
There was a problem hiding this comment.
Can't you convert it only when you need to insert it into grafana, rather than carrying around two separate variables that could get out of sync?
There was a problem hiding this comment.
@RobertKrawitz I agree.
Use the 'virtctl_status' string variable (instead of 'ssh_status') with the value 'True' if successful, or an error message if it fails.
Additionally, add the 'virtctl_vm' integer value (instead of 'vm_ssh') only for Grafana.
ebattat
force-pushed
the
add_vm_ssh_retry
branch
3 times, most recently
from
599c028 to
1be1d2f
Compare
ebattat
changed the title
ebattat
changed the title
ebattat
force-pushed
the
add_vm_ssh_retry
branch
from
1be1d2f to
861154a
Compare
|
@RobertKrawitz, |
Type of change
Note: Fill x in []
Description
Change the VM login method from SSH protocol to Virtctl, as this will provide clearer error messages.
Additionally, Since the VM could be in the middle of a migration, add a retry mechanism that makes 5 attempts with a 10-second pause between each retry.
For security reasons, all pull requests need to be approved first before running any automated CI