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

added basic os for EricssonLG ES switches #7289

Merged
merged 9 commits into from Sep 25, 2017

Conversation

Projects
None yet
4 participants
@tslytsly
Contributor

tslytsly commented Sep 5, 2017

Used new-os script to add basic support for EricssonLG iPECS ES network switch.

There was already basic support for EricssonLG iPECS UCP PBX, but the files were all called ericsson-es.
So I renamed them to ericsson-ucp so that I could use ericsson-es for the ES switches.

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


This change is Reviewable

@laf

Is ericsson-es the right name now, should we be calling it ericsson-lg or ericsson-switchos?

You'll need to re-add the test file for ericsson-es (or whatever it's new name).

We will also need to notify users that this is changing.

Show outdated Hide outdated includes/definitions/ericsson-es.yaml
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 5, 2017

Member

Thanks, few changes needed but should be good after that :)

Member

laf commented Sep 5, 2017

Thanks, few changes needed but should be good after that :)

@tslytsly

This comment has been minimized.

Show comment
Hide comment
@tslytsly

tslytsly Sep 6, 2017

Contributor

Is ericsson-es the right name now, should we be calling it ericsson-lg or ericsson-switchos?

Not sure, technically both the PBX and network switches are Ericsson LG iPECs devices. the PBX is the UCP and the switch is the ES. As far as I know ELG don't have an official name for the OS, so I just called it switchos.
Not sure what naming convention you want to follow?

You'll need to re-add the test file for ericsson-es (or whatever it's new name).

Yep, when I pushed the commit I didn't realise I needed to do this, but I looked in the docs and will make sure both are correct.

Contributor

tslytsly commented Sep 6, 2017

Is ericsson-es the right name now, should we be calling it ericsson-lg or ericsson-switchos?

Not sure, technically both the PBX and network switches are Ericsson LG iPECs devices. the PBX is the UCP and the switch is the ES. As far as I know ELG don't have an official name for the OS, so I just called it switchos.
Not sure what naming convention you want to follow?

You'll need to re-add the test file for ericsson-es (or whatever it's new name).

Yep, when I pushed the commit I didn't realise I needed to do this, but I looked in the docs and will make sure both are correct.

@tslytsly

This comment has been minimized.

Show comment
Hide comment
@tslytsly

tslytsly Sep 6, 2017

Contributor

Hmm, I added test files and got these errors.

Not sure how to read them.

Here are both test files:
ericsson-ucp:

1.3.6.1.2.1.1.1.0|4|Ericsson-LG,UCP100,UCP-R2.1.42&Boot Version-1.0Ea MAR/17
1.3.6.1.2.1.1.2.0|6|1.3.6.1.4.1.572.16838

ericsson-es:

1.3.6.1.2.1.1.1.0|4|ES-2026P Advanced Smart FE POE Switch
1.3.6.1.2.1.1.2.0|6|1.3.6.1.4.1.572.17389.107
Contributor

tslytsly commented Sep 6, 2017

Hmm, I added test files and got these errors.

Not sure how to read them.

Here are both test files:
ericsson-ucp:

1.3.6.1.2.1.1.1.0|4|Ericsson-LG,UCP100,UCP-R2.1.42&Boot Version-1.0Ea MAR/17
1.3.6.1.2.1.1.2.0|6|1.3.6.1.4.1.572.16838

ericsson-es:

1.3.6.1.2.1.1.1.0|4|ES-2026P Advanced Smart FE POE Switch
1.3.6.1.2.1.1.2.0|6|1.3.6.1.4.1.572.17389.107
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 6, 2017

Member

Not sure, technically both the PBX and network switches are Ericsson LG iPECs devices. the PBX is the UCP and the switch is the ES. As far as I know ELG don't have an official name for the OS, so I just called it switchos.

If they use the same OS then we should potentially keep them under on os discovery for us. Any reason why we wouldn't do that?

Hmm, I added test files and got these errors.

It's not matching the data properly at the moment. Let's work on getting the OS names right and fix that later.

Member

laf commented Sep 6, 2017

Not sure, technically both the PBX and network switches are Ericsson LG iPECs devices. the PBX is the UCP and the switch is the ES. As far as I know ELG don't have an official name for the OS, so I just called it switchos.

If they use the same OS then we should potentially keep them under on os discovery for us. Any reason why we wouldn't do that?

Hmm, I added test files and got these errors.

It's not matching the data properly at the moment. Let's work on getting the OS names right and fix that later.

@tslytsly

This comment has been minimized.

Show comment
Hide comment
@tslytsly

tslytsly Sep 6, 2017

Contributor

If they use the same OS then we should potentially keep them under on os discovery for us. Any reason why we wouldn't do that?

Well that's the thing. They are both branded as iPECS. But the PBX is a different beast from the switch. They do not share anything.
In fact, I suspect that the network switch is a re-badged netgear. 😄

I think they just call them both iPECS to make them easier to sell as a package.

Contributor

tslytsly commented Sep 6, 2017

If they use the same OS then we should potentially keep them under on os discovery for us. Any reason why we wouldn't do that?

Well that's the thing. They are both branded as iPECS. But the PBX is a different beast from the switch. They do not share anything.
In fact, I suspect that the network switch is a re-badged netgear. 😄

I think they just call them both iPECS to make them easier to sell as a package.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 7, 2017

Member

I'm fine with the name change, however I've just noticed why this isn't working. We already have an OS called ipecs which is one you are trying to add here.

Member

laf commented Sep 7, 2017

I'm fine with the name change, however I've just noticed why this isn't working. We already have an OS called ipecs which is one you are trying to add here.

@tslytsly

This comment has been minimized.

Show comment
Hide comment
@tslytsly

tslytsly Sep 7, 2017

Contributor

I'm fine with the name change, however I've just noticed why this isn't working. We already have an OS called ipecs which is one you are trying to add here.

Ah, that will be it then.
I've pushed another commit to try and tidy this up some more. it's clear that it's not that clear. 😄

Contributor

tslytsly commented Sep 7, 2017

I'm fine with the name change, however I've just noticed why this isn't working. We already have an OS called ipecs which is one you are trying to add here.

Ah, that will be it then.
I've pushed another commit to try and tidy this up some more. it's clear that it's not that clear. 😄

tslytsly added some commits Sep 7, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 7, 2017

Member

Unless you've got a genuine need to rename the current OS names then I'd suggest not doing this, we will have to notify users of the change for those that might reference these in device groups or alerting.

Member

laf commented Sep 7, 2017

Unless you've got a genuine need to rename the current OS names then I'd suggest not doing this, we will have to notify users of the change for those that might reference these in device groups or alerting.

@tslytsly

This comment has been minimized.

Show comment
Hide comment
@tslytsly

tslytsly Sep 8, 2017

Contributor

Unless you've got a genuine need to rename the current OS names then I'd suggest not doing this, we will have to notify users of the change for those that might reference these in device groups or alerting.

I think there is a need, the old names were not clear. In fact you could say that they were wrong.
I think that the new names follow the LibreNMS naming convention.
But I'm happy to just adjust the existing ericsson-es file to include the 2000 series switches and abandon the name change. 😄

Contributor

tslytsly commented Sep 8, 2017

Unless you've got a genuine need to rename the current OS names then I'd suggest not doing this, we will have to notify users of the change for those that might reference these in device groups or alerting.

I think there is a need, the old names were not clear. In fact you could say that they were wrong.
I think that the new names follow the LibreNMS naming convention.
But I'm happy to just adjust the existing ericsson-es file to include the 2000 series switches and abandon the name change. 😄

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 8, 2017

Member

If you really want to go down a name change then you'll need to update the notifications.rss feed here - see past commits to the file for an idea on what to put. Give users 5 days notice saying we will be changing the os names from X -> Y and that they will need to adjust alert rules / device groups.

Member

laf commented Sep 8, 2017

If you really want to go down a name change then you'll need to update the notifications.rss feed here - see past commits to the file for an idea on what to put. Give users 5 days notice saying we will be changing the os names from X -> Y and that they will need to adjust alert rules / device groups.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Sep 8, 2017

Member

If these two OS use the same or mostly mibs, likely they should be the same OS.

Also, os names cannot have an _ in them, just change it to -.

Member

murrant commented Sep 8, 2017

If these two OS use the same or mostly mibs, likely they should be the same OS.

Also, os names cannot have an _ in them, just change it to -.

tslytsly added some commits Sep 12, 2017

@tslytsly

This comment has been minimized.

Show comment
Hide comment
@tslytsly

tslytsly Sep 12, 2017

Contributor

sorry for all the extra commits, still getting the hang of this git stuff. 😄

Contributor

tslytsly commented Sep 12, 2017

sorry for all the extra commits, still getting the hang of this git stuff. 😄

@laf

After those two small changes then this is good from me.

@murrant

Show outdated Hide outdated includes/polling/os/elg-ipecs-es.inc.php
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Sep 12, 2017

The inspection completed: No new issues

scrutinizer-notifier commented Sep 12, 2017

The inspection completed: No new issues

@laf

laf approved these changes Sep 12, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 12, 2017

Member

Actually you are missing the core test unit file for elg-ipecs-ucp, basically you need a default test file called elg-ipecs-ucp.snmprec so just rename one of the two to that and it should pass checks.

Member

laf commented Sep 12, 2017

Actually you are missing the core test unit file for elg-ipecs-ucp, basically you need a default test file called elg-ipecs-ucp.snmprec so just rename one of the two to that and it should pass checks.

@tslytsly

This comment has been minimized.

Show comment
Hide comment
@tslytsly

tslytsly Sep 12, 2017

Contributor

@laf it failed again: https://p.libren.ms/view/a60a9727

Sorry, I don't really understand what these test files are doing, so I'm not sure what is wrong.
It seems to be still referencing the old os names with the _ in them, but not sure where from.

Contributor

tslytsly commented Sep 12, 2017

@laf it failed again: https://p.libren.ms/view/a60a9727

Sorry, I don't really understand what these test files are doing, so I'm not sure what is wrong.
It seems to be still referencing the old os names with the _ in them, but not sure where from.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 12, 2017

Member

git mv tests/snmpsim/elg-ipecs-ucp100.snmprec tests/snmpsim/elg-ipecs-ucp.snmprec

Member

laf commented Sep 12, 2017

git mv tests/snmpsim/elg-ipecs-ucp100.snmprec tests/snmpsim/elg-ipecs-ucp.snmprec

@tslytsly

This comment has been minimized.

Show comment
Hide comment
@tslytsly

tslytsly Sep 13, 2017

Contributor

thanks @laf that sorted one of the errors, but I'm still getting this:

There were 2 failures:

1) LibreNMS\Tests\OSDiscoveryTest::testOS with data set #111 ('elg-ipecs_es')
No snmprec files found for elg-ipecs_es!

/opt/librenms/tests/OSDiscoveryTest.php:75 

2) LibreNMS\Tests\OSDiscoveryTest::testOS with data set #112 ('elg-ipecs_ucp')
No snmprec files found for elg-ipecs_ucp!

/opt/librenms/tests/OSDiscoveryTest.php:75

I cannot find where it is getting those references to the old os names from...

Contributor

tslytsly commented Sep 13, 2017

thanks @laf that sorted one of the errors, but I'm still getting this:

There were 2 failures:

1) LibreNMS\Tests\OSDiscoveryTest::testOS with data set #111 ('elg-ipecs_es')
No snmprec files found for elg-ipecs_es!

/opt/librenms/tests/OSDiscoveryTest.php:75 

2) LibreNMS\Tests\OSDiscoveryTest::testOS with data set #112 ('elg-ipecs_ucp')
No snmprec files found for elg-ipecs_ucp!

/opt/librenms/tests/OSDiscoveryTest.php:75

I cannot find where it is getting those references to the old os names from...

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 13, 2017

Member

That must be local as it's fine in travis. Remove cache/os_defs.cache and it will rebuild.

Member

laf commented Sep 13, 2017

That must be local as it's fine in travis. Remove cache/os_defs.cache and it will rebuild.

@tslytsly

This comment has been minimized.

Show comment
Hide comment
@tslytsly

tslytsly Sep 14, 2017

Contributor

thanks, that worked.

Contributor

tslytsly commented Sep 14, 2017

thanks, that worked.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 14, 2017

Member

If these two OS use the same or mostly mibs, likely they should be the same OS.

I recently did this for a PR and did two OS' as the differences were enough.

I don't know this vendor / os enough to make that call here. Thoughts @murrant?

Member

laf commented Sep 14, 2017

If these two OS use the same or mostly mibs, likely they should be the same OS.

I recently did this for a PR and did two OS' as the differences were enough.

I don't know this vendor / os enough to make that call here. Thoughts @murrant?

@laf laf requested a review from murrant Sep 16, 2017

@tslytsly

This comment has been minimized.

Show comment
Hide comment
@tslytsly

tslytsly Sep 18, 2017

Contributor

I recently did this for a PR and did two OS' as the differences were enough.

Just FYI:
iPECS-UCP is a PBX, IP phone system and iPECS-ES are L2 and L3 smart network switches.

Contributor

tslytsly commented Sep 18, 2017

I recently did this for a PR and did two OS' as the differences were enough.

Just FYI:
iPECS-UCP is a PBX, IP phone system and iPECS-ES are L2 and L3 smart network switches.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 18, 2017

Member

I'm fine with the split. @murrant?

Member

laf commented Sep 18, 2017

I'm fine with the split. @murrant?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Sep 20, 2017

Member

Ready for merging now on the 25th of Sept

Member

laf commented Sep 20, 2017

Ready for merging now on the 25th of Sept

@laf laf added Blocker 🚫 and removed Blocker 🚫 labels Sep 20, 2017

@laf laf merged commit c07daa3 into librenms:master Sep 25, 2017

2 checks passed

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

@tslytsly tslytsly deleted the tslytsly:elg-switch branch Feb 12, 2018

@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.