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

refactor: Updated discovery/poller to use numeric sysObjectID #7922

Merged
merged 21 commits into from Jan 7, 2018

Conversation

Projects
None yet
4 participants
@laf
Member

laf commented Dec 19, 2017

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

Makes sysObjectID and sysDescr available straight away so devices now only need 1 disco to find all data.

Same principle as poller.

@murrant

This comment has been minimized.

Member

murrant commented Dec 20, 2017

  • Any concern that the sysObjectID coming from the database will start with "enterprises" and the one you set is fully numeric?
  • I noticed you left the fetch of sysObjectID and sysDescr in the getHost() function... oversight or planned?
  • You missed two calls powerconnect.inc.php in mempools and processors "mib-2.1.2.0" (which is sysObjectID)
@@ -0,0 +1,6 @@
<?php
$snmpdata = snmp_get_multi_oid($device, 'sysName.0 sysObjectID.0 sysDescr.0', '-OUQn', 'SNMPv2-MIB:HOST-RESOURCES-MIB:SNMP-FRAMEWORK-MIB');

This comment has been minimized.

@murrant

murrant Dec 20, 2017

Member

Isn't SNMPv2-MIB the only mib needed here?

This comment has been minimized.

@laf

laf Dec 20, 2017

Member

You're correct. Updated.

@erotel

This comment has been minimized.

Contributor

erotel commented Dec 20, 2017

Discovery new device [sysObjectID] => .1.3.6.1.4.1.259.10.1.22.101

Discovery device after first poll [sysObjectID] => enterprises.259.10.1.42.101

@laf

This comment has been minimized.

Member

laf commented Dec 20, 2017

Any concern that the sysObjectID coming from the database will start with "enterprises" and the one you set is fully numeric?

Not really, we overwrite what's in the db within the $device array in the core.inc.php file. I can update poller as well to use full numerical and store that.

I noticed you left the fetch of sysObjectID and sysDescr in the getHost() function... oversight or planned?

Planned. I realise we re-run the queries but I didn't want to use global variables and rely on them throughout the rest of the code.

You missed two calls powerconnect.inc.php in mempools and processors "mib-2.1.2.0" (which is sysObjectID)

Well spotted, updated.

@murrant

This comment has been minimized.

Member

murrant commented Dec 20, 2017

If you update it in the poller, make sure you update alert rules and other things that rely on the enterprises existing. I think full numeric is probably my preferred over the other.

getHostOS() already accepts $device...

@laf

This comment has been minimized.

Member

laf commented Dec 20, 2017

If you update it in the poller, make sure you update alert rules and other things that rely on the enterprises existing. I think full numeric is probably my preferred over the other.

Updated.

Done alert rules as well.

getHostOS() already accepts $device...

It does but it's only called from disco when the OS is generic and then os.inc.php which can be selected not to run.

@laf

This comment has been minimized.

Member

laf commented Dec 20, 2017

This should also fix a few broken things in polling.

@laf laf added the Schema label Dec 20, 2017

@laf laf changed the title from refactor: Updated discovery to use a core module for sysDescr/sysObjectID to refactor: Updated discovery/poller to use numeric sysObjectID Dec 20, 2017

@murrant

This comment has been minimized.

Member

murrant commented Dec 22, 2017

I think you mis-understand me about getHostOS()
Delete these lines:

    $sysDescr    = snmp_get($device, "SNMPv2-MIB::sysDescr.0", "-Ovq");
    $sysObjectId = snmp_get($device, "SNMPv2-MIB::sysObjectID.0", "-Ovqn");

change getHostOS() and ubnt.inc.php to use $device['sysObjectID'] and $device['sysDescr'].

You'll also need to update testing to pre-populate those I think.

@murrant

This comment has been minimized.

Member

murrant commented Dec 22, 2017

On, second thought, perhaps those changes should be a separate PR. Don't want to make too many changes at once.

@laf

This comment has been minimized.

Member

laf commented Dec 22, 2017

I think you mis-understand me about getHostOS()
Delete these lines:

$sysDescr    = snmp_get($device, "SNMPv2-MIB::sysDescr.0", "-Ovq");
$sysObjectId = snmp_get($device, "SNMPv2-MIB::sysObjectID.0", "-Ovqn");

Those are used within getHostOS() though. And we haven't yet asked for them in core.inc.php.

change getHostOS() and ubnt.inc.php to use $device['sysObjectID'] and $device['sysDescr'].

Updated ubnt.inc.php. Not done getHostOS as per the comment above.

You'll also need to update testing to pre-populate those I think.

Testing uses $sysDescr and $sysObjectID so don't think anything needs updating.

@laf

This comment has been minimized.

Member

laf commented Dec 23, 2017

Testing uses $sysDescr and $sysObjectID so don't think anything needs updating.

Actually I see now that travis broke. I've updated to take that into account now. testing should work ok.

@laf

This comment has been minimized.

Member

laf commented Dec 24, 2017

I've refactored everything from sysObjectId -> sysObjectID as that's what the actual OID + DB column is.

This means that anyone who doesn't clear the OS cache when doing a git pull will have issues. ./daily.sh takes care of this so as always, people should be using that to update.

@murrant

This comment has been minimized.

Member

murrant commented Dec 27, 2017

Looks good, I'll do some testing after the December release.

@murrant murrant added this to the 1.36 milestone Dec 27, 2017

@murrant

This comment has been minimized.

Member

murrant commented Jan 4, 2018

@laf can you rebase this so I can test it?

@laf

This comment has been minimized.

Member

laf commented Jan 4, 2018

Rebased + fixed the remaining few sysObjectId -> sysObjectID that slipped in since this was done.

@murrant

This comment has been minimized.

Member

murrant commented Jan 5, 2018

Looks good so far, you missed the yaml generation in new-os.php ;)

@laf

This comment has been minimized.

Member

laf commented Jan 5, 2018

Looks good so far, you missed the yaml generation in new-os.php ;)

Don't think so:

-bash-4.2$ grep sysObjectId scripts/*.php
-bash-4.2$
@laf

This comment has been minimized.

Member

laf commented Jan 5, 2018

After the recent merges, this is now up to date again.

@murrant

This comment has been minimized.

Member

murrant commented Jan 5, 2018

I've got this on my main install, working fine (after I remembered to delete the os cache).

@laf

This comment has been minimized.

Member

laf commented Jan 6, 2018

I've got this on my main install, working fine (after I remembered to delete the os cache).

Yeah which is the only issue with this, if someone updates manually :(

Maybe we should add the cache clearing to includes/sql-schema/update.php to catch those cases otherwise we have no way to resolve it for people updating manually. Anyone just doing a git pull is asking for trouble until discovery runs to populate the schema anyway.

@murrant

This comment has been minimized.

Member

murrant commented Jan 6, 2018

We could do a post-merge git hook that removes that file.

Fix tests: $device is not pulled from the database before polling
Also, update the db in the core discovery module.
@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Jan 6, 2018

The inspection completed: No new issues

@laf

This comment has been minimized.

Member

laf commented Jan 6, 2018

We could do a post-merge git hook that removes that file.

I didn't think you could commit hooks to repos for them to be automatically run.

@murrant

This comment has been minimized.

Member

murrant commented Jan 7, 2018

I think you are right.

@murrant murrant merged commit 42e5819 into librenms:master Jan 7, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@lock

This comment has been minimized.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.