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

Feature: Generic discovery and poller tests #7873

Merged
merged 34 commits into from Dec 20, 2017

Conversation

Projects
None yet
3 participants
@murrant
Member

murrant commented Dec 7, 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

Show outdated Hide outdated scripts/save-test-data.php Outdated
@laf

You probably already know but needs docs + help info in the script file.

Trying to start snmpsimd using the args from the script:


-bash-4.2$ snmpsimd.py --data-dir=./tests/snmpsim --agent-udpv4-endpoint=127.1.6.1:1161 --logging-method=file:/tmp/snmpsimd.log
Traceback (most recent call last):
  File "/usr/lib/python3.4/site-packages/snmpsim-0.3.0-py3.4.egg/snmpsim/record/search/database.py", line 106, in create
    oid, tag, val = self.__textParser.grammar.parse(line)
  File "/usr/lib/python3.4/site-packages/snmpsim-0.3.0-py3.4.egg/snmpsim/grammar/snmprec.py", line 39, in parse
    raise error.SnmpsimError('broken record <%s>' % line)
snmpsim.error.SnmpsimError: broken record <b'1.3.6.1.2.1.25.3.2.1.3.196608||Intel Xeon E3-12xx v2 (Ivy Bridge)\n'>

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/bin/snmpsimd.py", line 4, in <module>
    __import__('pkg_resources').run_script('snmpsim==0.3.0', 'snmpsimd.py')
  File "/usr/lib/python3.4/site-packages/pkg_resources/__init__.py", line 746, in run_script
    self.require(requires)[0].run_script(script_name, ns)
  File "/usr/lib/python3.4/site-packages/pkg_resources/__init__.py", line 1501, in run_script
    exec(code, namespace, namespace)
  File "/usr/lib/python3.4/site-packages/snmpsim-0.3.0-py3.4.egg/EGG-INFO/scripts/snmpsimd.py", line 1199, in <module>
    snmpContext
  File "/usr/lib/python3.4/site-packages/snmpsim-0.3.0-py3.4.egg/EGG-INFO/scripts/snmpsimd.py", line 836, in configureManagedObjects
    dataFile = DataFile(fullPath, textParser).indexText(forceIndexBuild)
  File "/usr/lib/python3.4/site-packages/snmpsim-0.3.0-py3.4.egg/EGG-INFO/scripts/snmpsimd.py", line 217, in indexText
    self.__recordIndex.create(forceIndexBuild, validateData)
  File "/usr/lib/python3.4/site-packages/snmpsim-0.3.0-py3.4.egg/snmpsim/record/search/database.py", line 114, in create
    raise error.SnmpsimError('Data error at %s:%d: %s' % (self.__textFile, lineNo, exc))
snmpsim.error.SnmpsimError: Data error at ./tests/snmpsim/linux.snmprec:29: broken record <b'1.3.6.1.2.1.25.3.2.1.3.196608||Intel Xeon E3-12xx v2 (Ivy Bridge)\n'>
-bash-4.2$ snmpsimd.py --data-dir=./tests/snmpsim --agent-udpv4-endpoint=127.1.6.1:1161 --logging-method=file:/tmp/snmpsimd.log^C
-bash-4.2$ grep '1.3.6.1.2.1.25.3.2.1.3.196608' ./tests/snmpsim/*
./tests/snmpsim/linux.snmprec:1.3.6.1.2.1.25.3.2.1.3.196608||Intel Xeon E3-12xx v2 (Ivy Bridge)

Also, are we going to be expecting users to submit this as I feel like it's going to raise the barrier to people contributing PRs.

Definitely a huge step forward and for me and you we can submit them when doing PRs for devices.

murrant added some commits Dec 8, 2017

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Dec 8, 2017

Member

Yeah, requiring testing is a big barrier, I am worried about that too, but I hope to make it dead simple with the script. The benefits of having the tests should be pretty clear :)

snmpsim.error.SnmpsimError: broken record <b'1.3.6.1.2.1.25.3.2.1.3.196608||Intel Xeon E3-12xx v2 (Ivy Bridge)\n'>

I fixed a lot of issues with the save-test-data.php script today, remove the snmprec file and try again.

I'll add some docs after things settle down. save-test-data.php has help text now :D

Member

murrant commented Dec 8, 2017

Yeah, requiring testing is a big barrier, I am worried about that too, but I hope to make it dead simple with the script. The benefits of having the tests should be pretty clear :)

snmpsim.error.SnmpsimError: broken record <b'1.3.6.1.2.1.25.3.2.1.3.196608||Intel Xeon E3-12xx v2 (Ivy Bridge)\n'>

I fixed a lot of issues with the save-test-data.php script today, remove the snmprec file and try again.

I'll add some docs after things settle down. save-test-data.php has help text now :D

murrant added some commits Dec 8, 2017

mark TestCase as abstract
don't run two instances of snmpsim
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Dec 9, 2017

Member

Works better now.

For me running ./scripts/save-test-data.php -d --no-save -h localhost -o linux -m processors it just changes some of the existing data in the json and snmprec, wouldn't this just mean we're constantly changing the data within the file based on people's local setup?

Member

laf commented Dec 9, 2017

Works better now.

For me running ./scripts/save-test-data.php -d --no-save -h localhost -o linux -m processors it just changes some of the existing data in the json and snmprec, wouldn't this just mean we're constantly changing the data within the file based on people's local setup?

@laf laf added this to the 1.35 milestone Dec 9, 2017

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Dec 10, 2017

Member

Yes, but that is why we should use variants. Once all the modules are added. We won't need to update specific os captures much.

Also for devices, the data won't change as wildly as it does for servers.

When merging existing snmprec and new snmprec I could prefer existing data over new? Which do you think is better?

Member

murrant commented Dec 10, 2017

Yes, but that is why we should use variants. Once all the modules are added. We won't need to update specific os captures much.

Also for devices, the data won't change as wildly as it does for servers.

When merging existing snmprec and new snmprec I could prefer existing data over new? Which do you think is better?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Dec 10, 2017

Member

When merging existing snmprec and new snmprec I could prefer existing data over new? Which do you think is better?

Seems better if the OIDs exist in the test data not to overwrite them.

Member

laf commented Dec 10, 2017

When merging existing snmprec and new snmprec I could prefer existing data over new? Which do you think is better?

Seems better if the OIDs exist in the test data not to overwrite them.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Dec 10, 2017

Member

If you can add docs for this, after the overriding of data is fixed then this is good for a merge.

Member

laf commented Dec 10, 2017

If you can add docs for this, after the overriding of data is fixed then this is good for a merge.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Dec 11, 2017

Member

Hmm, for some modules we will need separate data recorded for discovery and polling :/

I think I will make overwriting existing snmprec data an option, otherwise it will be a pain to update

Member

murrant commented Dec 11, 2017

Hmm, for some modules we will need separate data recorded for discovery and polling :/

I think I will make overwriting existing snmprec data an option, otherwise it will be a pain to update

murrant added some commits Dec 11, 2017

refactor to use class
collects all code in one place for reusability
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Dec 15, 2017

Member

I've still not read the code so some of this will be answered in there I'm sure.

./scripts/save-test-data.php -h localhost -o linux -d

Still adds data to the test files, is that expected:

-bash-4.2$ git diff tests/data/linux.json
diff --git a/tests/data/linux.json b/tests/data/linux.json
index 8607a2d7d..4e8107cc1 100644
--- a/tests/data/linux.json
+++ b/tests/data/linux.json
@@ -93,5 +93,9 @@
             ]
         },
         "poller": "matches discovery"
+    },
+    "all": {
+        "discovery": [],
+        "poller": "matches discovery"
     }
 }
\ No newline at end of file
-bash-4.2$ git diff tests/snmpsim/linux.snmprec
diff --git a/tests/snmpsim/linux.snmprec b/tests/snmpsim/linux.snmprec
index fcbd58141..36a074e59 100644
--- a/tests/snmpsim/linux.snmprec
+++ b/tests/snmpsim/linux.snmprec
@@ -1,5 +1,10 @@
 1.3.6.1.2.1.1.1.0|4|Linux some.host.name 4.9.0 #1 SMP Mon Dec 12 14:44:53 CST 2016 x86_64
 1.3.6.1.2.1.1.2.0|6|1.3.6.1.4.1.8072.3.2.10
+1.3.6.1.2.1.1.3.0|67|24144912
+1.3.6.1.2.1.1.4.0|4|<private>
+1.3.6.1.2.1.1.5.0|4|<private>
+1.3.6.1.2.1.1.6.0|4|<private>
+1.3.6.1.2.1.25.1.1.0|67|24146459
 1.3.6.1.2.1.25.3.2.1.1.196608|2|196608
 1.3.6.1.2.1.25.3.2.1.1.196609|2|196609
 1.3.6.1.2.1.25.3.2.1.1.196610|2|196610
@@ -167,3 +172,4 @@
 1.3.6.1.2.1.25.3.8.1.9.28|4|0-1-1,0:0:0.0
 1.3.6.1.2.1.25.3.8.1.9.29|4|0-1-1,0:0:0.0
 1.3.6.1.2.1.25.3.8.1.9.30|4|0-1-1,0:0:0.0
+1.3.6.1.6.3.10.2.1.3.0|2|241449

I do find that I need to run the script twice, the first time snmpsimd doesn't start the first time for some reason.

Member

laf commented Dec 15, 2017

I've still not read the code so some of this will be answered in there I'm sure.

./scripts/save-test-data.php -h localhost -o linux -d

Still adds data to the test files, is that expected:

-bash-4.2$ git diff tests/data/linux.json
diff --git a/tests/data/linux.json b/tests/data/linux.json
index 8607a2d7d..4e8107cc1 100644
--- a/tests/data/linux.json
+++ b/tests/data/linux.json
@@ -93,5 +93,9 @@
             ]
         },
         "poller": "matches discovery"
+    },
+    "all": {
+        "discovery": [],
+        "poller": "matches discovery"
     }
 }
\ No newline at end of file
-bash-4.2$ git diff tests/snmpsim/linux.snmprec
diff --git a/tests/snmpsim/linux.snmprec b/tests/snmpsim/linux.snmprec
index fcbd58141..36a074e59 100644
--- a/tests/snmpsim/linux.snmprec
+++ b/tests/snmpsim/linux.snmprec
@@ -1,5 +1,10 @@
 1.3.6.1.2.1.1.1.0|4|Linux some.host.name 4.9.0 #1 SMP Mon Dec 12 14:44:53 CST 2016 x86_64
 1.3.6.1.2.1.1.2.0|6|1.3.6.1.4.1.8072.3.2.10
+1.3.6.1.2.1.1.3.0|67|24144912
+1.3.6.1.2.1.1.4.0|4|<private>
+1.3.6.1.2.1.1.5.0|4|<private>
+1.3.6.1.2.1.1.6.0|4|<private>
+1.3.6.1.2.1.25.1.1.0|67|24146459
 1.3.6.1.2.1.25.3.2.1.1.196608|2|196608
 1.3.6.1.2.1.25.3.2.1.1.196609|2|196609
 1.3.6.1.2.1.25.3.2.1.1.196610|2|196610
@@ -167,3 +172,4 @@
 1.3.6.1.2.1.25.3.8.1.9.28|4|0-1-1,0:0:0.0
 1.3.6.1.2.1.25.3.8.1.9.29|4|0-1-1,0:0:0.0
 1.3.6.1.2.1.25.3.8.1.9.30|4|0-1-1,0:0:0.0
+1.3.6.1.6.3.10.2.1.3.0|2|241449

I do find that I need to run the script twice, the first time snmpsimd doesn't start the first time for some reason.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Dec 15, 2017

Member

I have not added support for more than one module yet, but I plan to. So I left the option to do all in the script.

I just changed the snmpsim stuff around a little I'll give it some more testing, but I don't remember that happening for me.

Member

murrant commented Dec 15, 2017

I have not added support for more than one module yet, but I plan to. So I left the option to do all in the script.

I just changed the snmpsim stuff around a little I'll give it some more testing, but I don't remember that happening for me.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Dec 15, 2017

Member

You happy with this to be merged then @murrant? I'm happy that it can just be adjusted as we go along if needs be?

Member

laf commented Dec 15, 2017

You happy with this to be merged then @murrant? I'm happy that it can just be adjusted as we go along if needs be?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Dec 15, 2017

Member

Yeah, I think the file format is good. Just needs expansion and bug fixes now.

Member

murrant commented Dec 15, 2017

Yeah, I think the file format is good. Just needs expansion and bug fixes now.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Dec 16, 2017

Member

Yeah, I think the file format is good. Just needs expansion and bug fixes now.

Still not sure if that means it's ready for a merge @murrant :)

I'm trying to fix the travis issue at the moment, think I'm going to resort to just dropping the check for poller-service.py for now.

Member

laf commented Dec 16, 2017

Yeah, I think the file format is good. Just needs expansion and bug fixes now.

Still not sure if that means it's ready for a merge @murrant :)

I'm trying to fix the travis issue at the moment, think I'm going to resort to just dropping the check for poller-service.py for now.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Dec 16, 2017

Member

Travis is now fixed so feel free to just rebase this and merge if you think it's ready.

Member

laf commented Dec 16, 2017

Travis is now fixed so feel free to just rebase this and merge if you think it's ready.

@murrant murrant changed the title from Processor tests to Feature: Generic discovery and poller tests Dec 18, 2017

murrant added some commits Dec 18, 2017

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Dec 20, 2017

The inspection completed: 3 new issues, 41 updated code elements

scrutinizer-notifier commented Dec 20, 2017

The inspection completed: 3 new issues, 41 updated code elements

@murrant murrant merged commit fff66d3 into librenms:master Dec 20, 2017

2 checks passed

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

@murrant murrant deleted the murrant:processor-tests branch Dec 20, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 16, 2018

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

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.