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

NaN in Proxmox 8.0.4 #5

Open
jordipalet opened this issue Aug 9, 2023 · 9 comments
Open

NaN in Proxmox 8.0.4 #5

jordipalet opened this issue Aug 9, 2023 · 9 comments
Assignees

Comments

@jordipalet
Copy link

I applied manually all the patches and included all the sensors that I can get via sensors -j.

The "bar visual" works, however, instead of something like:
CPU temp 39ºC (crit: 100ºC)
I see:
CPU temp NaN%

It looks like the calculation for the bar length size is done correctly, but not for the values display?

Or I'm missing something ?

Tks!

This is for example my CPU:

"coretemp-isa-0000":{
"Adapter": "ISA adapter",
"Package id 0":{
"temp1_input": 42.000,
"temp1_max": 100.000,
"temp1_crit": 100.000,
"temp1_crit_alarm": 0.000
},

which seems to be the default as per the patch files, so I didn't changed anything there.

so Nodes.pm is
cputemp => {
jsonpath => ['coretemp-isa-0000', 'Package id 0'],
valkey => 'temp1_input',
critkey => 'temp1_crit',
},

and I also kept pvemanagerlib.js:

    {
        itemId: 'cputemp',
        iconCls: 'fa fa-fw fa-thermometer-half',
        title: gettext('CPU temp'),
        valueField: 'cputemp',
        maxField: 'cputemp',
        renderer: Proxmox.Utils.render_node_temp,
    },
@alexleigh
Copy link
Owner

Did you also apply the patch to proxmoxlib.js? If you did add the render_node_temp function to that file, but still getting this result, then most likely, something changed in how proxmox-widget-toolkit works in PVE 8. In PVE 7, Proxmox.Utils.render_node_temp refers to the function I added to proxmox-widget-toolkit which formats the temperature string. It's possible this has changed in PVE 8. I don't have a working PVE 8 node right now, but I will try to spin one up soon and test out to make these patches work with PVE 8.

@jordipalet
Copy link
Author

Yes applied the 3 patches, just didn't tried the mobile one ... will do later.

So yes, I guess there is some minor change in PVE 8 ...

Tks!

@jordipalet
Copy link
Author

The mobile didn't worked either, however, as this is a testing machine when I restarted it this morning, it seems the other 3 patches are working (I see greyed the "CPU temp 39ºC (crit: 100ºC)" for each core (I've done that for every sensor in sensor -j). So a restart was needed for some reason, not just the "systemctl restart pveproxy.service "

But even if the sensors are displayed correctly, it stays as "Loading" in that part of the Summary panel, not sure how I can debug where is the error (which yesterday was not there ...)

@jordipalet
Copy link
Author

Mystery resolved, at least for the normal patch (the mobile one not working yet). It seems that V8 needs a reboot, also make sure that you don't have any empty line in the .js ...

@microcrash
Copy link

I'm running Proxmox 8.03 however I had a slightly different issue. After making the adjustments, nothing would load in the browser. I traced this down to the function and adjusted it as follows:

render_node_temp: function(record) {
    let currenttemp = record.used.toFixed(1);
    let maxtemp = record.total.toFixed(1);
    return `${currenttemp} °C (crit: ${maxtemp} °C)`;
},

Once I did that, all I had to do was "systemctl restart pveproxy.service" and everything worked. no reboot required.

@alexleigh
Copy link
Owner

I created a PVE 8.0.4 node and was able to apply the changes without any modifications. I created a new set of patch files based on the PVE 8.0.4 base files here: https://github.com/alexleigh/pve-mods/tree/main/v8.0.4-pwt4.0.6/patches

Could you try these new set of patches and see if they resolved your issues?

Something else I noticed is that, if any of the configured outputs in the JavaScript does not match an emitted dictionary in the Perl API, then the entire PVE manager display would be greyed out, but the working senosrs would still have a value. This may help you figure out which sensors are working and which aren't: the sensors which do display a value are working, while the sensors which show no value are causing the JavaScript to crash. In this case please double check the Nodes.pm code to make sure the configured JSON paths match exactly what is shown when running sensors -j.

@jordipalet
Copy link
Author

for me seems to be working fine, however not yet in the mobile one. I can see CPU temp, then the title of the next value (in my case Core0 temp) but not the value. I tried removing all the cores, and leaving only nvmetemp, same result.

Tried with Safari and Chrome (in iOS).

@alexleigh alexleigh self-assigned this Aug 17, 2023
@alexleigh
Copy link
Owner

That's strange. I checked the behavior on the mobile version and it actually has a safety check. If the property cannot be retrieved, it should just render a dash, instead of throwing an exception. It was actually the desktop version that was lacking this check, which is what causes the JavaScript to crash and a grey loading area to be displayed when a temperature property could not be read. I just updated the patch files to included this check on the desktop side. Now, on either the mobile page or the desktop page, if a temperature property could not be read, a dash is displayed, and should not interfere with the loading of the properties which could be read.

If you are still having issues on the desktop side, you can try updating to the latest patch for 8.0.4 and see which probes are displaying the dash. If your issue is on the mobile side only, and it is not just that some probes display as a dash, could you share a screenshot of what the mobile site looks like for you? Also, if you wouldn't mind, could you also upload the patched versions of the files on your system, and I will try to reproduce the error.

@microcrash
Copy link

I just wanted to give an update from my last post. I updated to 8.0.04 (from 8.0.03) and everything works and loads for me on my computer browser and mobile side. Did not have to make the modification like I did before.

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