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

Extend global variables with BIOS information #2000

Closed
wants to merge 1 commit into from

Conversation

pszczerx
Copy link
Contributor

  • Add utils to read DMI table using dmidecode.
  • Extend collectd global variables with BIOS information (vendor, version,
    release date and revision).

Signed-off-by: Przemyslaw Szczerbik przemyslawx.szczerbik@intel.com

* Add utils to read DMI table using dmidecode.
* Extend collectd global variables with BIOS information (vendor, version,
  release date and revision).

Change-Id: I505c08f93a91c58b18203251fab00fc950a203eb
Signed-off-by: Przemyslaw Szczerbik <przemyslawx.szczerbik@intel.com>
@rubenk
Copy link
Contributor

rubenk commented Oct 24, 2016

Hi @pszczerx. Can you explain the purpose of this pull request? I'm having a hard time coming up with a use case.

@maryamtahhan
Copy link
Contributor

Hi @rubenk
The use case is that in an NFV deployment we would like to be able to retrieve the BIOS string in the first step to enable us to determine if the platform is suitable for workload placement. the next step would be the bios configuration to determine if platform features are enabled/disabled.

@rubenk
Copy link
Contributor

rubenk commented Oct 25, 2016

Hi @maryamtahhan. I'm sorry but I strongly feel that this is functionality best left to some configuration management system or external script and doesn't belong in core Collectd code.

@maryamtahhan
Copy link
Contributor

Hi @rubenk the idea isn't configuration, it's to relay the current state of a platform in operation. Collectd pushes information about the host today, including uuid for a host or a VM from libvirt, is the BIOS string really that much of a deviation from what is supported today?

@maryamtahhan
Copy link
Contributor

if not in the core collectd code. is a separate plugin more suitable?

@rubenk
Copy link
Contributor

rubenk commented Oct 25, 2016

@maryamtahhan Could you expand a bit on what you mean with "workload placement" and "platform features"? It still sounds a bit abstract to me. I take it you have some code that is going to use these variables somehow? Perhaps posting that will further the discussion.

@maryamtahhan
Copy link
Contributor

maryamtahhan commented Oct 25, 2016

@rubenk What I meant by workload placement and platform features is really about being able to identify a suitable server for a VNF/application by checking server capabilities first; building on what OpenStack Nova Scheduler does today (and this is where we are looking at integrating this kind of information). The Nova Scheduler matches the server capabilities with the required VM characteristics for placement, and with most of the advanced technologies being embedded in processors and chip-sets integrated on server boards, more often than not, these capabilities are only configurable through BIOS. All we want to do is report if the capability is enabled/disabled. for example Intel VT-d need to be enabled through BIOS before an application that relies on it can be placed on that server. So we want to share this type of information with the Scheduler. This is an example of some of the ground work that has gone in https://01.org/sites/default/files/page/openstack-epa_wp_fin.pdf. But there is more to come. We'd like to use collectd as the "agent" or collector for platform relevant infomation, metrics and events that will help enable Service Assurance, failure prediction and workload placement in an NFV environment.
Also, if this is preferred to be in a separate plugin, we can have a look at that approach

@rubenk
Copy link
Contributor

rubenk commented Oct 25, 2016

Thanks for the explanation. What you need goes beyond collectds original purpose, that is collecting system and application performance metrics. It's also a fairly niche use-case to extend the core for. Is there any reason why this functionality needs to be in collectd and you can't get this with say something like factor, ohai or even a shell script that grabs it from /sys/class/dmi? It's all fairly static data that doesn't change often.

@maryamtahhan
Copy link
Contributor

Thanks @rubenk, we wanted to take advantage of the publishing plugins collectd already has. Different management systems will have a different input bus, the use of collectd allows us seamless integration with a number of frameworks. We were really looking at using it as the central agent that collects all the relevant platform information.
What about using a separate plugin for this in collectd that uses the init function and uninit functions without the read, and dispatching an information based notification... or are you saying this you don't see this as ever being part of collectd even if a separate plugin in used? If not we will pursue a different option.
Thanks in advance

@rubenk
Copy link
Contributor

rubenk commented Oct 25, 2016

The main difference is that all the current publishing plugins and collectd's data format are for transporting metrics instead of information, and as such, I think they are an ill fit for your purpose.
I think even SNMP is better suited for this kind of stuff.

Normally we are quite liberal in what we accept as plugins go, as long as there's some benefit to at least multiple users that outweighs the cost of the added maintenance. To be frank, I don't see that for your use case, where the plugin is essentially an implementation detail of your larger system.

@rpv-tomsk
Copy link
Contributor

rpv-tomsk commented Oct 26, 2016

Hi Maryam,

If not we will pursue a different option.

What are your goals and benefits from the inclusion of this feature (plugin or patch) to Collectd core?
If you really need this feature, that is not hard to make this as separate plugin and distribute it within your local Collectd packages or as addon package to common OS Collectd package.

If you need some changes to core to make these features work that can be discussed, I think.


IMHO: A more general purpose functions are preferred.

As @rubenk said before,

It's all fairly static data that doesn't change often.

I dislike #1936 PR where 'power supply presence' tried to be added - it is very static data too.
Such metric is unuseful on charts which Collectd is intended for (initially).

But Collectd can be expanded with 'data transmission' function, where:

  • Collectd agent transmits some data (including string values) to hub Collectd.
  • Collectd Hub collects and stores received data in its cache (RAM), possible with expiration time declared in DataTypesDB.
  • Collectd hub provides collected data to external systems in some way like GETVAL of unixsock (poll) or using 'write' plugins (push).
  • Transmitted data is identified by host, plugin, plugin instance, type, type_instance like generic metrics. (?) (plugin and plugin_instance shoulde be read as service and service_instance instead).
  • These data metrics are not placed to charts (i.e. other than generic write callback is required to data metrics (?) ).
  • Notifications can be sent on data value change;

Purpose of this: use single Collectd as agent and transport level for other systems (like monitoring) integration.

I.e. there will be 'subsystems' in Collectd:

  • Metrics
  • Notifications
  • Data.

RFC.

@taraschornyi
Copy link
Contributor

Hi @rpv-tomsk @rubenk,

The purpose of this change is to extend available information about host(platform) that write plugins can use. Currently only hostname is available. We are planning to implement write plugins that will require this extended info about host(platform), so it would be good to have it available.
If current implementation(global variables) is not acceptable what about have this as utils API?

@rubenk
Copy link
Contributor

rubenk commented Oct 26, 2016

@taraschornyi can you show us the code for one of those write plugins? That will hopefully give us a better idea about how this should all work. When more plugins need this functionality, it can always be abstracted out later.

Please note that I'm still vehemently opposed to doing this in collectd. It is platform specific (not all platforms have SMBIOS), adds a dependency on dmidecode, and parsing output from a cmdline tool is asking for trouble.

Collectd does one thing pretty good, and we have our hands full with keeping it that way. Turning it into some kind of data gathering agent needs a lot of thought up front, and work. The data format also has to be extended, which will probably be a breaking change.

So far I haven't heard any arguments that convince me that this should be done. I'm not the only one that has a say in this however.

So first, please show us the code :)

@rpv-tomsk
Copy link
Contributor

The purpose of this change is to extend available information about host(platform) that write plugins can use.

You can use <LoadPlugin name> Global true .. syntax and provide your custom functions to your custom plugins.

@rubenk

The data format also has to be extended, which will probably be a breaking change.

I think this can be done in backward-compatible way. In my view this is an additional subsystem which can be built like existing one (metrics).

@vmytnykx
Copy link
Contributor

vmytnykx commented Nov 3, 2016

Hi @taraschornyi, @rpv-tomsk, @rubenk,

I like rpv-tom's idea about using <LoadPlugin name> Global true ...

Currently, collectd includes a plugin called uuid which just replaces global host value with host UUID on init procedure. The plugin reads the UUID value using different ways, including tool like dmidecode.

Would it be acceptable and more safe to just implement similar plugin (e.g. bios) that can provide BIOS information through plugin global variables? So, other plugins can use this information for different purposes.

@maryamtahhan
Copy link
Contributor

Hi @rubenk the plugin that will use it is currently under development, but we'll be able to push it soon for you to have a look.

I agree with @rpv-tomsk this can be done in a backward compatible way and should be an extension of the existing subsystem. Also I like the RFC

@vmytnykx put forward an interesting suggestion. if there's any additional feedback on that, it would be great. Alongside this, there's also the metadata field, which allows us to attach some information to an already existing struct, can we not use the metadata field to pass the BIOS string? to a write plugin that's interested in it? or is it just preferred to wrap up this functionality in the output plugin that wants to use it?

@maryamtahhan maryamtahhan deleted the feat_bios branch February 8, 2017 09:44
@pszczerx pszczerx closed this Feb 8, 2017
@kwiatrox kwiatrox mentioned this pull request Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants