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

fix: Proxmox input: Changed VM ID from string to int, fixes #9611 #10068

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

atetevoortwis
Copy link
Contributor

Small fix to properly parse JSON data from Proxmox >=7.0 installs.

Required for all PRs:

resolves #9611

  • Updated the VMStat schema to use a Number for ID.
  • Changed getCurrentVMStatus and getVmConfig accordingly.
  • Added case to testdata to check that both string representations and integers work in JSON.

@telegraf-tiger telegraf-tiger bot added the fix pr to fix corresponding bug label Nov 5, 2021
@atetevoortwis atetevoortwis changed the title Fix: Poxmox input: Changed VM ID from string to int, fixes #9611 Fix: Proxmox input: Changed VM ID from string to int, fixes #9611 Nov 5, 2021
- Updated the VMStat schema to use a Number for ID.
- Changed getCurrentVMStatus and getVmConfig accordingly.
- Added case to testdata to check that both string representations and integers work in JSON.
@atetevoortwis atetevoortwis changed the title Fix: Proxmox input: Changed VM ID from string to int, fixes #9611 fix: Proxmox input: Changed VM ID from string to int, fixes #9611 Nov 5, 2021
@atetevoortwis
Copy link
Contributor Author

@rare-magma, Finally found some time for this, continuing here...

@atetevoortwis
Copy link
Contributor Author

@srebhan With regards to your comment (originially on #9820):

is this a bug that was there from the beginning or does it depend on the proxmox version? If the latter, you need to
unmarshal to the respective type, i.e. you try with an int first and if that fails, you use it as a string...

To be honest I'm not sure: I experienced this bug when upgrading to Proxmox 7.0.0, so I expect it to be related to a change on their API.
I've updated the unit test to both use a string (representation of an int) and int's in the test JSON and that seems to work...
Although I'm completely new to Go, so let me know if this is not the way :-)

@srebhan
Copy link
Contributor

srebhan commented Nov 8, 2021

@atetevoortwis the code looks good so far. Can you please check the documentation to make sure previous IDs were struct numerical and not "abc2347jddh" type of values?

@atetevoortwis
Copy link
Contributor Author

@atetevoortwis the code looks good so far. Can you please check the documentation to make sure previous IDs were struct numerical and not "abc2347jddh" type of values?

As far as I know Proxmox VM ID's always have been integers: https://pve.proxmox.com/pve-docs/api-viewer/#/nodes/{node}/lxc/{vmid} .

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, assuming the backward compatibility is given as discussed. Thanks @atetevoortwis for your work!

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Nov 9, 2021
@reimda reimda merged commit 8a3ba85 into influxdata:master Nov 9, 2021
@reimda
Copy link
Contributor

reimda commented Nov 9, 2021

Thanks @atetevoortwis

powersj pushed a commit that referenced this pull request Nov 17, 2021
VladislavSenkevich pushed a commit to gwos/telegraf that referenced this pull request Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error getting VM curent VM status - Proxmox error
3 participants