Skip to content
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

VirtualMachine.IsRunning and VirtualMachine.IsStopped unconditionally rely on fields that VirtualMachine may not have #139

Closed
brandonllocke opened this issue Mar 30, 2024 · 3 comments

Comments

@brandonllocke
Copy link
Contributor

Starting this issue with the full disclosure that I know enough Go and enough Proxmox to be dangerous, but not much more than that.

It seems that the VirtualMachine type is written in such a way that it is not necessarily expected to always get a value for QMPStatus:

QMPStatus      string `json:"qmpstatus,omitempty"`

But the VirtualMachine.IsRunning() and VirtualMachine.IsStopped() functions (and likely others) essentially require QMPStatus to be set to work.

func (v *VirtualMachine) IsRunning() bool {
	return v.Status == StatusVirtualMachineRunning && v.QMPStatus == StatusVirtualMachineRunning
}

As such (for reasons not clear to me), I have VMs on my Proxmox server that are not returning anything for QMPStatus, so IsRunning() always returns false despite the fact that the VMs are running.

Should this be checking for empty values before comparing QMPStatus? If so, I am willing to try to make a PR for that. I Just wanted to make sure I'm not misunderstanding what is happening before I attempt that.

Thanks!

@brandonllocke brandonllocke changed the title VirtualMachine.IsRunning and VirtualMachine.IsStopped rely on fields that VirtualMachine may not have VirtualMachine.IsRunning and VirtualMachine.IsStopped unconditionally rely on fields that VirtualMachine may not have Mar 30, 2024
@justinclift
Copy link

I have VMs on my Proxmox server that are not returning anything for QMPStatus

It kind of sounds like that might really be the core problem which needs fixing? 😄

@brandonllocke
Copy link
Contributor Author

I can't say much for certain as the documentation on what that value actually means is not very clear, but it does appear QMPStatus is optional, so it being required to determine if a VM is running/stopped/etc. feels like a bug. I'm open to being wrong.

@luthermonson
Copy link
Owner

ya so that's leaning towards the client having a bug and we shouldnt check QMPStatus in that func and others

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants