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

Check if MAC address exists before assume it's an Unmanaged device #17506

Draft
wants to merge 8 commits into
base: 10.0/bugfixes
Choose a base branch
from

Conversation

eduardomozart
Copy link
Contributor

@eduardomozart eduardomozart commented Jul 12, 2024

A MAC address should be unique. When running a NetDiscovery task, it may create a new Unmanaged asset with a MAC address that already exists on GLPI, which can lead to duplicate MAC addresses on NetworkPort DB table, so before assuming it's an Unmanaged device, check if there's any item on DB that matches this MAC address. It would be great if GLPI implements an option to cleanup orphans/unmanaged devices from time to time as seen in: https://github.com/glpi-project/glpi-inventory-plugin/blob/e8cd779e0449ab4d402774e39a22f84b69317f3b/hook.php#L357

Q A
Bug fix? yes/no
New feature? yes/no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets glpi-project/glpi-inventory-plugin#530

A MAC address should be unique. When running a NetDiscovery task, it may create a new Unmanaged asset with a MAC address that already exists on GLPI, which can lead to duplicate MAC addresses on NetworkPort DB table, so before assuming it's an Unmanaged device, check if there's any item on DB that matches this MAC address.
When handled here, the existing asset is updates but shows on the "Importation log" tab that it was catch in "Unmanaged import" rule.
Update an Unmanaged item only if it's the only available.
Remove duplicate unmanaged assets from NetDiscovery.
It’s complex to deal with device removal (specially LLDP devices and other scenarios variables, including the NetDiscovery and NetInventory order which then was executed). It would be better to implement a clean up orphans unmanaged devices in GLPI core (it seems that a similar feature was planned for GLPI Inventory plugin as I could see commented on its source code).
@trasher
Copy link
Contributor

trasher commented Aug 2, 2024

Please fix tests and CI. Also, this probably need a specific test case

@trasher
Copy link
Contributor

trasher commented Sep 16, 2024

Hi, are you going to work again on this one or should we close?

Also, no new feature will be accepted on 10.0/bugfixes branch, it must target main.

@eduardomozart
Copy link
Contributor Author

Yes, I will, but I'm not being paid for this like TecLib does, so it's a third party contribution project that I've work with. Any help to fix bugs on your commercial product would be helpful, since it's a bug and not an intended behavior from GLPI Agent and GLPI core itself.

@trasher trasher marked this pull request as draft September 16, 2024 13:01
@trasher
Copy link
Contributor

trasher commented Sep 16, 2024

I'm not convinced this is a bug, and since that can change existing inventory behavior; it should still target main branch.

Anyway, community issues are handled on a on a voluntary basis.

Finally, just for you to know I do not like your remarks.

Best regards.

@trasher trasher removed their request for review September 16, 2024 13:18
@eduardomozart
Copy link
Contributor Author

Yes, it's a voluntary project, but TecLib hire employees to provide commercial support and there's about ~$2M income per year that TechLib does, so there should be interest on TechLib to fix bugs on a product it receives money for instead of expecting voluntaries to do so. TechLib also benefits from works of their volunters, so when you guys ask for tests and do not provide any help to develop them and troubleshoot the issues we report, instead of expecting us to do all the hard work, would be appreciable.

Of course this is a bug and it's simply reproduciable: Do a Network Discovery task, inventory discovered devices using Network Inventory task, and do a Network Discovery task again: you'll see that devices that was already inventories by an Network Inventory task will be shown on "Unmanaged devices" again, overriding the info collected by Network Inventory task. MAC address should be treated as unique and information be merged with the current device instead of simply overriding it all with useless information from Network Discovery task as it does today, and my fix attempts to fix that.

@stonebuzz
Copy link
Contributor

hi @eduardomozart

NetDiscovery and NetInventory are not designed for ESX inventory, but only for SNMP-capable devices.

If the ESX comes up as Unamanged, it's because the agent hasn't been able to find the type from its OIDs (cf https://glpi-agent.readthedocs.io/en/latest/database.html | https://github.com/glpi-project/sysobject.ids/).

Logical, since it doesn't respond to SNMP

That's the real problem! Using SNMP inventory/discovery on non-compatible equipment.

So your fix, as it stands, looks more like a band-aid to help you deal with this particular case, which is the result of bad practice.

Of course, it's always possible to improve this reconciliation, so here are a few comments on this PR

  • It's the Unmanaged (src/Inventory/asset/Unmanaged) class that needs to be improved (it already contains code for reconciliation (cf. rulepassed function)) not MainAsset.
  • Must be done on main branch (as it would be an improvement of the reconciliation method)
  • Unit tests must be added, it can often seem demanding, but clearly mandatory . We have over 150,000 lines of code running tests (25,000 just for inventory!) , guaranteeing increased robustness and optimum process reliability.

To conclude

it's important to remember the foundation of our project: it's open source AND community-based. Although we have an in-house team working on the product, collaboration with the community is at the heart of our vision.

We firmly believe that this synergy betweenTECLIB and volunteer contributors is the strength and richness of our product.

Best regards

@eduardomozart
Copy link
Contributor Author

eduardomozart commented Sep 17, 2024

hi @eduardomozart

NetDiscovery and NetInventory are not designed for ESX inventory, but only for SNMP-capable devices.

If the ESX comes up as Unamanged, it's because the agent hasn't been able to find the type from its OIDs (cf https://glpi-agent.readthedocs.io/en/latest/database.html | https://github.com/glpi-project/sysobject.ids/).

Logical, since it doesn't respond to SNMP

That's the real problem! Using SNMP inventory/discovery on non-compatible equipment.

So your fix, as it stands, looks more like a band-aid to help you deal with this particular case, which is the result of bad practice.

Of course, it's always possible to improve this reconciliation, so here are a few comments on this PR

  • It's the Unmanaged (src/Inventory/asset/Unmanaged) class that needs to be improved (it already contains code for reconciliation (cf. rulepassed function)) not MainAsset.
  • Must be done on main branch (as it would be an improvement of the reconciliation method)
  • Unit tests must be added, it can often seem demanding, but clearly mandatory . We have over 150,000 lines of code running tests (25,000 just for inventory!) , guaranteeing increased robustness and optimum process reliability.

To conclude

it's important to remember the foundation of our project: it's open source AND community-based. Although we have an in-house team working on the product, collaboration with the community is at the heart of our vision.

We firmly believe that this synergy betweenTECLIB and volunteer contributors is the strength and richness of our product.

Best regards

Hello, thank you for your reply.
About the community vision of GLPI, I believe it can be improved. As a volunteer, I provided a lot of contributions reporting bugs and fixing them when possible for years now. But recently with GLPI 10, I’m not sure if the code guidelines changed or something like that, but every bug is treated like “it’s intended behavior” and if we want to fix it, we have to do it ourselves, with little to none support of GLPI team, and when we provide a fix, it isn’t accept if there’s a missing indent somewhere, something easy to fix that GLPI team could simply add the missing indent and commit the code. It seems to me a lot of ill will when us volunteers take hours digging and fixing known bugs that could be explored further with GLPI team help.

About the Unmanaged device instance, it seems to happen with ESXi hosts and also IP phones. I know there’s no reason to do a Network Discovery for a device that was already discovered, but when I report a bug, I always report it as an average user do so, so this bug doesn’t affect me directly, but the average user would expect to input its internal IP range and GLPI discover new devices automagically while keeping the current one as it is. Also, there’s cases of devices that uses DHCP, so there isn’t a simply way to average user without DHCP server access/knowledge to exclude the device from NetDiscovery task even creating an IP range exception because when IP changes the device would pop up again. And there’s legitimate cases where a NetDiscovery should be used on Managed devices IP range: when a device uses DHCP and change its IP address, we need to run a NetDiscovery on IP range to update its IP address before running a NetInventory task on them. There’s other places where this fix could work from, e.g before adding an Unmanaged device from Net Discovery, check if that MAC address exists on GLPI and do not add them as such if it is.

Commentaries like yours really help us to understand GLPI internals and how it was intended to work, instead of simply letting us on our own luck. I would expend hours doing unit tests (which I have difficulties to do because I am not a developer, just someone who wants to contribute and make the GLPI even better) for a fix that would not be approved at all. I will see what I can do to provide a functional code to fix this on Unmanaged asset portion of the code. Thank you.

Copy link

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.

3 participants