Add proxmox monitoring. #1789

Merged
merged 15 commits into from Sep 1, 2015

Projects

None yet

6 participants

@tuxis-ie
Contributor

This enables LibreNMS to create traffic graphs for Proxmox-based VMs.

Please check this for authorization stuff, as I don't know if I did that right. Ultimate goal is to ba able to create traffic bills voor VMs, but this is a good first step.

@laf laf commented on an outdated diff Aug 28, 2015
html/includes/graphs/application/proxmox_traffic.inc.php
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2 dated June,
+ * 1991.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * See http://www.gnu.org/licenses/gpl.txt for the full license
+ */
+
+require 'includes/graphs/common.inc.php';
+
+$mysql_rrd = $config['rrd_dir'].'/proxmox/'.$vars['cluster'].'/'.$vars['vmid'].'_netif_'.$vars['port'].'.rrd';
@laf
laf Aug 28, 2015 Member

This needs to be named more appropriately :)

@laf
Member
laf commented Aug 28, 2015

Just waiting for scrut now. The biggest thing I can spot is the formatting of the if / else statements.

We have docs on how it should be formatted:

http://docs.librenms.org/Developing/Code-Guidelines/

So no if () { } on the same line, } else { should be:

}
else {

etc.

@f0o
Member
f0o commented Aug 28, 2015

Sorry for the spam

@laf laf commented on an outdated diff Aug 29, 2015
html/includes/graphs/application/auth.inc.php
@@ -3,7 +3,11 @@
if (is_numeric($vars['id']) && ($auth || application_permitted($vars['id']))) {
$app = get_application_by_id($vars['id']);
$device = device_by_id_cache($app['device_id']);
- $title = generate_device_link($device);
- $title .= $graph_subtype;
+ if ($app['app_type'] != 'proxmox') {
+ $title = generate_device_link($device);
+ $title .= $graph_subtype;
+ } else {
@laf
laf Aug 29, 2015 Member

Need to split this to:

}
else {

@laf laf commented on an outdated diff Aug 29, 2015
html/pages/apps/proxmox.inc.php
+include('includes/application/proxmox.inc.php');
+$graphs['proxmox'] = array(
+ 'netif'
+);
+
+$pmxcl = dbFetchRows("SELECT DISTINCT(`app_instance`) FROM `applications` WHERE `app_type` = ?", array('proxmox'));
+
+
+print_optionbar_start();
+
+echo "<span style='font-weight: bold;'>Proxmox</span> &#187; ";
+
+unset($sep);
+
+foreach ($pmxcl as $pmxc) {
+ if (isset($sep)) { echo $sep; };
@laf
laf Aug 29, 2015 Member

Needs splitting out to:

if () {

}

@laf laf commented on an outdated diff Aug 29, 2015
html/pages/device/apps/proxmox.inc.php
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * See http://www.gnu.org/licenses/gpl.txt for the full license
+ */
+
+include('includes/application/proxmox.inc.php');
+
+global $config;
+
+if (!isset($config['enable_proxmox']) || !$config['enable_proxmox']) {
+ print_error('Proxmox agent was discovered on this host. Please enable Proxmox in your config.');
+} else {
@laf
laf Aug 29, 2015 Member

Needs splitting out to:

}
else {

@laf laf commented on an outdated diff Aug 29, 2015
includes/polling/applications/proxmox.inc.php
+ $rrd_filename = join('/', array(
+ $pmxcdir,
+ $vmid.'_netif_'.$vmport.'.rrd'));
+
+ if (!is_file($rrd_filename)) {
+ rrdtool_create(
+ $rrd_filename,
+ ' --step 300 \
+ DS:INOCTETS:DERIVE:600:0:12500000000 \
+ DS:OUTOCTETS:DERIVE:600:0:12500000000 '.$config['rrd_rra']);
+ }
+
+ rrdtool_update($rrd_filename, 'N:'.$vmpin.':'.$vmpout);
+ if (proxmox_vm_exists($vmid, $pmxcluster, $pmxcache) === true) {
+ dbUpdate(array('device_id' => $device['device_id'], 'last_seen' => array('NOW()'), 'description' => $vmdesc), 'proxmox', '`vmid` = ? AND `cluster` = ?', array($vmid, $pmxcluster));
+ } else {
@laf
laf Aug 29, 2015 Member

Needs splitting out to:

}
else {

@laf laf commented on an outdated diff Aug 29, 2015
includes/polling/applications/proxmox.inc.php
+ $rrd_filename,
+ ' --step 300 \
+ DS:INOCTETS:DERIVE:600:0:12500000000 \
+ DS:OUTOCTETS:DERIVE:600:0:12500000000 '.$config['rrd_rra']);
+ }
+
+ rrdtool_update($rrd_filename, 'N:'.$vmpin.':'.$vmpout);
+ if (proxmox_vm_exists($vmid, $pmxcluster, $pmxcache) === true) {
+ dbUpdate(array('device_id' => $device['device_id'], 'last_seen' => array('NOW()'), 'description' => $vmdesc), 'proxmox', '`vmid` = ? AND `cluster` = ?', array($vmid, $pmxcluster));
+ } else {
+ $pmxcache[$pmxcluster][$vmid] = dbInsert(array('cluster' => $pmxcluster, 'vmid' => $vmid, 'description' => $vmdesc, 'device_id' => $device['device_id']), 'proxmox');
+ }
+
+ if ($portid = proxmox_port_exists($vmid, $pmxcluster, $vmport) !== false) {
+ dbUpdate(array('last_seen' => array('NOW()')), 'proxmox_ports', '`vm_id` = ? AND `port` = ?', array($pmxcache[$pmxcluster][$vmid], $vmport));
+ } else {
@laf
laf Aug 29, 2015 Member

Needs splitting out to:

}
else {

@laf
Member
laf commented Aug 29, 2015

Hi @tuxis-ie

I've added some notes about } else { and if () { } lines you have, if you could update those quickly then we can merge :)

@f0o f0o commented on an outdated diff Aug 30, 2015
html/pages/apps/proxmox/vm.inc.php
@@ -0,0 +1,35 @@
+<?php
+
+
+global $vars;
@f0o
f0o Aug 30, 2015 Member

This line can be removed, using global outside a function context doesn't really do anything useful

@f0o f0o commented on an outdated diff Aug 30, 2015
html/pages/apps/proxmox/vm.inc.php
@@ -0,0 +1,35 @@
+<?php
+
+
+global $vars;
+
+$vm = proxmox_vm_info(var_get('vmid'), var_get('instance'));
@f0o
f0o Aug 30, 2015 Member

Why not use $vars['vmid'] and $vars['instance'] instead?

@f0o f0o commented on an outdated diff Aug 30, 2015
html/pages/device/apps/proxmox.inc.php
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2 dated June,
+ * 1991.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * See http://www.gnu.org/licenses/gpl.txt for the full license
+ */
+
+include('includes/application/proxmox.inc.php');
+
+global $config;
@f0o
f0o Aug 30, 2015 Member

Can be removed

@f0o f0o commented on an outdated diff Aug 30, 2015
includes/polling/applications/proxmox.inc.php
@@ -0,0 +1,79 @@
+<?php
+
+if (isset($config['enable_proxmox']) && $config['enable_proxmox'] && !empty($agent_data['app']['proxmox'])) {
+ $proxmox = $agent_data['app']['proxmox'];
+}
+
+function proxmox_port_exists($i, $c, $p) {
@f0o
f0o Aug 30, 2015 Member

Please add a DocBlock

@f0o f0o commented on an outdated diff Aug 30, 2015
includes/polling/applications/proxmox.inc.php
@@ -0,0 +1,79 @@
+<?php
+
+if (isset($config['enable_proxmox']) && $config['enable_proxmox'] && !empty($agent_data['app']['proxmox'])) {
+ $proxmox = $agent_data['app']['proxmox'];
+}
+
+function proxmox_port_exists($i, $c, $p) {
+ if ($row = dbFetchRow("SELECT pmp.id FROM proxmox_ports pmp, proxmox pm WHERE pm.id = pmp.vm_id AND pmp.port = ? AND pm.cluster = ? AND pm.vmid = ?", array($p, $c, $i))) {
+ return $row['id'];
+ }
+
+ return false;
+}
+
+function proxmox_vm_exists($i, $c, &$pmxcache) {
@f0o
f0o Aug 30, 2015 Member

Please add a DocBlock

@f0o f0o commented on an outdated diff Aug 30, 2015
html/includes/application/proxmox.inc.php
+ * Copyright (C) 2015 Mark Schouten <mark@tuxis.nl>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2 dated June,
+ * 1991.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * See http://www.gnu.org/licenses/gpl.txt for the full license
+ */
+
+function proxmox_cluster_vms($c) {
@f0o
f0o Aug 30, 2015 Member

Please add a DocBlock (also to the rest)

@f0o f0o commented on an outdated diff Aug 30, 2015
html/pages/apps/proxmox.inc.php
@@ -0,0 +1,54 @@
+<?php
+
+include('includes/application/proxmox.inc.php');
@f0o
f0o Aug 30, 2015 Member

Please use require_once instead

@f0o f0o and 2 others commented on an outdated diff Aug 30, 2015
html/includes/functions.inc.php
@@ -12,6 +12,29 @@
* @copyright (C) 2013 LibreNMS Group
*/
+function var_isset($v) {
@f0o
f0o Aug 30, 2015 Member

Are these 3 functions really necessary? They seem to overly complicate access to $vars

@tuxis-ie
tuxis-ie Aug 30, 2015 Contributor

I introduced the var_ functions after noticing the shitload of php warnings because unset $vars we're being accessed. Var_isset may be a bit over the top, but var_eq and var_get are very useful, IMHO.

Mark Schouten
Tuxis Internet Engineering
mark@tuxis.nl / 0318 200208

On 30 Aug 2015, at 16:36, "Daniel Preussker" notifications@github.com wrote:

In html/includes/functions.inc.php:

@@ -12,6 +12,29 @@

+function var_isset($v) {
Are these really necessary? They seem to overly complicate access to $vars


Reply to this email directly or view it on GitHub.

@paulgear
paulgear Aug 30, 2015 Member

I would definitely prefer to see var_isset gone, since it makes the code slightly less obvious, only saves one character, and adds the overhead of a function call.

@f0o f0o referenced this pull request Aug 30, 2015
Closed

Support for Proxmox VE #1143

@laf
Member
laf commented Aug 31, 2015

@tuxis-ie Tried to catch you on irc last night.

The /apps/app=proxmox/ page needs to be tidied up a little as well, the lists are right up against the left of the page with no spacing / padding.

Also, you need to rebase due to another PR conflicting.

@SaaldjorMike

You don't need the parenthesis around the require_once argument.

@tuxis-ie tuxis-ie Add DocBlock
a608faf
@f0o f0o added the Polling label Sep 1, 2015
@f0o f0o self-assigned this Sep 1, 2015
@f0o f0o merged commit b9bf61b into librenms:master Sep 1, 2015

2 checks passed

Auto-Deploy Build finished.
Details
Scrutinizer 8 new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment