Uploaded image for project: 'Observium'
  1. Observium
  2. OBS-471

Preliminary support for Proxmox VE guest monitoring

Details

    Description

      This patch against r4386 adds preliminary support for
      monitoring guests running on Proxmox VE platform. Both
      OpenVZ containers and KVM virtual machines are supported.

      Poller connects to Proxmox node via Proxmox's PVE2 API
      and polls stats from all guests on all hosts in a cluster.
      Stores RRD files under proxmox subdir for each device.

      Included PVE2 API client code used by poller is taken from
      https://github.com/CpuID/pve2-api-php-client.

      It also adds "Proxmox VE Guests" tab on the device page
      that displays CPU, Memory, Disk Usage, Disk I/O and Network
      Traffic statistics for each guest.

      Attachments

        Issue Links

          Activity

            [OBS-471] Preliminary support for Proxmox VE guest monitoring

            Well, then ... good luck with your coding style. I have donated the code. It's yours. Use it as is, change it, toss it ... makes no difference to me. I am writting extensions to Observium for my own purposes. In the process, I am more than willing to share, but I have no intention of becoming full time Observium developer, nor will I follow clearly flawed logic. People go to great lengths to reduce the expensive database access - you indirectly force developers to use it, even when it's not neccessary. Have you ever consideded your project might be used in companies with tens or even hundreds of engineers/support agents accessing Observium simultaneosly ? How many unnecessary queries are made as a result of your rules ?

            Anyway ... I'll keep offering my patches, you'll keep rejecting them. But, perhaps, somewhere in between, someone on the list might find them useful and benefit from them even if they don't comply with your coding style.

            marko Marko Dinic added a comment - Well, then ... good luck with your coding style. I have donated the code. It's yours. Use it as is, change it, toss it ... makes no difference to me. I am writting extensions to Observium for my own purposes. In the process, I am more than willing to share, but I have no intention of becoming full time Observium developer, nor will I follow clearly flawed logic. People go to great lengths to reduce the expensive database access - you indirectly force developers to use it, even when it's not neccessary. Have you ever consideded your project might be used in companies with tens or even hundreds of engineers/support agents accessing Observium simultaneosly ? How many unnecessary queries are made as a result of your rules ? Anyway ... I'll keep offering my patches, you'll keep rejecting them. But, perhaps, somewhere in between, someone on the list might find them useful and benefit from them even if they don't comply with your coding style.

            You're probably missing the part where I won't commit anything that doesn't follow our coding style.

            adama Adam Armstrong added a comment - You're probably missing the part where I won't commit anything that doesn't follow our coding style.

            I'm passing all that to avoid accessing the database again. I already have to access the database and iterate over all discovered guests when im generating graph rows. Why not simply pass and re-use that data instead of making additional queries to generate RRD file paths, graph titles, etc ? With thousands of guests in the cluster that would make thousands of SELECTs while page is loading. Or am I missing something painfully obvious ?

            marko Marko Dinic added a comment - I'm passing all that to avoid accessing the database again. I already have to access the database and iterate over all discovered guests when im generating graph rows. Why not simply pass and re-use that data instead of making additional queries to generate RRD file paths, graph titles, etc ? With thousands of guests in the cluster that would make thousands of SELECTs while page is loading. Or am I missing something painfully obvious ?

            + $graph_array['device'] = $device['device_id'];
            + $graph_array['group'] = $group;
            + // - our custom parameters that will be passed to graph.php
            + // and subsequently to our graph generating code when
            + // invoked from <img src="graph.php? ..."/> tag
            + $graph_array['clustername'] = $guest['cluster_name'];
            + $graph_array['guestid'] = $guest['guest_id'];
            + $graph_array['guestname'] = $guest['guest_name'];
            + $graph_array['guesttype'] = $guest['guest_type'];
            + // Generate <img src="graph.php? ..."/> tags on current page

            Why are you passing all of this?

            Please follow the existing coding style and pass the minimum required to identify the entity. The Observium standard is to pass a database-specific id, with an option to find the id if entity-specific data is passed (see the ports graphs, generally we pass a port_id, but they can also be accessed via ifName or ifIndex)

            adama Adam Armstrong added a comment - + $graph_array ['device'] = $device ['device_id'] ; + $graph_array ['group'] = $group; + // - our custom parameters that will be passed to graph.php + // and subsequently to our graph generating code when + // invoked from <img src="graph.php? ..."/> tag + $graph_array ['clustername'] = $guest ['cluster_name'] ; + $graph_array ['guestid'] = $guest ['guest_id'] ; + $graph_array ['guestname'] = $guest ['guest_name'] ; + $graph_array ['guesttype'] = $guest ['guest_type'] ; + // Generate <img src="graph.php? ..."/> tags on current page Why are you passing all of this? Please follow the existing coding style and pass the minimum required to identify the entity. The Observium standard is to pass a database-specific id, with an option to find the id if entity-specific data is passed (see the ports graphs, generally we pass a port_id, but they can also be accessed via ifName or ifIndex)

            Thanks a bunch! I'll keep an eye on other things I might have missed, ofc.

            marko Marko Dinic added a comment - Thanks a bunch! I'll keep an eye on other things I might have missed, ofc.

            Marko just confirming your patch still works for standalone proxmox installs

            anthonysomerset Anthony Somerset added a comment - Marko just confirming your patch still works for standalone proxmox installs

            I'm generally a Perl coder. PHP is not really my thing, but I'm getting used to it. In Perl, that would make a sound input params check, plus I generally like to be extra careful with input parameters. If you think it is unnecessary, I will remove it.

            marko Marko Dinic added a comment - I'm generally a Perl coder. PHP is not really my thing, but I'm getting used to it. In Perl, that would make a sound input params check, plus I generally like to be extra careful with input parameters. If you think it is unnecessary, I will remove it.

            You are totally right. I have unintentionally discriminated against people running standalone Proxmox servers Your fix sounds about right so I've added it to my original patch. It's basically the same as yours, but without duplicated code. I don't have standalone Proxmox to test it on, tho ...

            marko Marko Dinic added a comment - You are totally right. I have unintentionally discriminated against people running standalone Proxmox servers Your fix sounds about right so I've added it to my original patch. It's basically the same as yours, but without duplicated code. I don't have standalone Proxmox to test it on, tho ...

            + // All params passed down from graph.php
            + // have to be defined and valid
            + if (isset($vars['group']) && !empty($vars['group']) &&
            + isset($vars['clustername']) && !empty($vars['clustername']) &&
            + isset($vars['guestid']) && !empty($vars['guestid']) &&
            + isset($vars['guestname']) && !empty($vars['guestname']) &&
            + isset($vars['guesttype']) && !empty($vars['guesttype']))
            + {

            Porque?

            adama Adam Armstrong added a comment - + // All params passed down from graph.php + // have to be defined and valid + if (isset($vars ['group'] ) && !empty($vars ['group'] ) && + isset($vars ['clustername'] ) && !empty($vars ['clustername'] ) && + isset($vars ['guestid'] ) && !empty($vars ['guestid'] ) && + isset($vars ['guestname'] ) && !empty($vars ['guestname'] ) && + isset($vars ['guesttype'] ) && !empty($vars ['guesttype'] )) + { Porque?

            replacing patch as it was malformed

            anthonysomerset Anthony Somerset added a comment - replacing patch as it was malformed
            anthonysomerset Anthony Somerset added a comment - - edited

            extended original patch to handle standalone nodes still - rolled against revision 4471

            anthonysomerset Anthony Somerset added a comment - - edited extended original patch to handle standalone nodes still - rolled against revision 4471

            People

              adama Adam Armstrong
              marko Marko Dinic
              Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: