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

devices - Ciena RLS 6500 #15909

Merged
merged 29 commits into from Apr 1, 2024
Merged

Conversation

h-barnhart
Copy link
Contributor

@h-barnhart h-barnhart commented Mar 26, 2024

Adding the new OS ciena-rls, which adds support for Ciena's 6500 Reconfigurable Line System (RLS) optical networking platform.

This will allow for monitoring of optical line system transmit and receive, optical return loss, amplifier status and state, and temperature monitoring.

YAML discovery was used as much as possible. I had to create PHP for the sensors because Ciena used two byte strings at the end of the OID as the index and as far I could tell the YAML would only accept one byte string as an index.

Adds the appropriate MIB files which also updates CIENA-SMI.

Please note

Please read this information carefully. You can run ./lnms dev:check to check your code before submitting.

  • Have you followed our code guidelines?
  • If my Pull Request does some changes/fixes/enhancements in the WebUI, I have inserted a screenshot of it.
  • If my Pull Request makes discovery/polling/yaml changes, I have added/updated test data.

RLS

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@h-barnhart
Copy link
Contributor Author

Running into an issue with the snmpsim data causing tests to fail in the port discovery. The tests expect ifVLAN to be empty ('') but its getting a null. I'm not sure how to resolve that.

@PipoCanaja PipoCanaja added the Device 🖥️ New or added device support label Mar 27, 2024
@PipoCanaja
Copy link
Contributor

PipoCanaja commented Mar 27, 2024

Hi @h-barnhart
Looking at the mib :

rlsInventoryAmpsEntry OBJECT-TYPE
    SYNTAX      RlsInventoryAmpsEntry
    MAX-ACCESS  not-accessible
    STATUS      current
    DESCRIPTION 
        ""
    INDEX       { rlsInventoryAmpsSlotName, rlsInventoryAmpsName}
    ::= { rlsInventoryAmpsTable 1 }

For instance :
"sensor_oid": ".1.3.6.1.4.1.1271.4.1.1.1.2.1.1.1.48.1.49.7.66.111.111.115.116.101.114",
"sensor_index": "rlsInventoryAmpsInCurrPower.1.49.7.66.111.111.115.116.101.114",
"sensor_descr": "Slot 1 Booster",

That can be decoded as "1 char ASC(49) which is "1" and 7 chars ASC( 66.111.111.115.116.101.114 ) which are "Booster".

So it is 1 byte string, but dual indexed string ... And you are right, current code only support the 1st index string (in that case "1".). Remaining data is ignored.

I lack time these days, so unless a miracle happens, your PHP code will make it. If by chance I can improve the index parsing, you may be able to YAML all :)

@h-barnhart
Copy link
Contributor Author

So it is 1 byte string, but dual indexed string ... And you are right, current code only support the 1st index string (in that case "1".). Remaining data is ignored.

I lack time these days, so unless a miracle happens, your PHP code will make it. If by chance I can improve the index parsing, you may be able to YAML all :)

Right on. The flip slide is MIB does have entries in the table for both parts of the index that make up the byte string (slot ID and slot name), but doesn't allow access to those entries. If they did I probably wouldn't have had to code as much.

@h-barnhart
Copy link
Contributor Author

h-barnhart commented Mar 27, 2024

1) LibreNMS\Tests\YamlSchemaTest::testDiscoveryDefinitionSchema ciena-rls.yaml does not validate. Violations: [modules.sensors.temperature.data[0].num_oid] Does not match the regex pattern ^(\.\d+)+\.?\{\{ \$index \}\}(\.\d+)*$

Are you not allowed to use index_string anymore in YAML discovery? The index value is a string which identifies the slot on the device where the temperature sensor is located. I'm using that in the description of the temp sensor, as there may be multiple versions of the same hardware component in different slots.

If I can't use $index_string, is there a way in the YAML to get the string value of the index? If not I'll code it out in PHP.

@h-barnhart
Copy link
Contributor Author

1) LibreNMS\Tests\YamlSchemaTest::testDiscoveryDefinitionSchema ciena-rls.yaml does not validate. Violations: [modules.sensors.temperature.data[0].num_oid] Does not match the regex pattern ^(\.\d+)+\.?\{\{ \$index \}\}(\.\d+)*$

Are you not allowed to use index_string anymore in YAML discovery? The index value is a string which identifies the slot on the device where the temperature sensor is located. I'm using that in the description of the temp sensor, as there may be multiple versions of the same hardware component in different slots.

If I can't use $index_string, is there a way in the YAML to get the string value of the index? If not I'll code it out in PHP.

I went ahead made a php script to get past the tests. All green now.

@PipoCanaja
Copy link
Contributor

PipoCanaja commented Mar 28, 2024

1) LibreNMS\Tests\YamlSchemaTest::testDiscoveryDefinitionSchema ciena-rls.yaml does not validate. Violations: [modules.sensors.temperature.data[0].num_oid] Does not match the regex pattern ^(\.\d+)+\.?\{\{ \$index \}\}(\.\d+)*$

Are you not allowed to use index_string anymore in YAML discovery? The index value is a string which identifies the slot on the device where the temperature sensor is located. I'm using that in the description of the temp sensor, as there may be multiple versions of the same hardware component in different slots.

If I can't use $index_string, is there a way in the YAML to get the string value of the index? If not I'll code it out in PHP.

Should be, it is still used by some YAML files.

I'll add the fix here hopefully this WE. I have one running in my lab right now.

@PipoCanaja
Copy link
Contributor

PipoCanaja commented Mar 30, 2024

Fixed index_stringfor single string indexed OIDs. Meaning we can restore YAML for temperatures for instance.

@PipoCanaja
Copy link
Contributor

Now, index_string can correctly handle an index like "678.hw87" and convert it to the OID representation of it (with length for each of the 2 subindexes and conversion to DEC).

includes/definitions/ciena-rls.yaml Outdated Show resolved Hide resolved
includes/definitions/ciena-rls.yaml Outdated Show resolved Hide resolved
@PipoCanaja
Copy link
Contributor

@PipoCanaja
Copy link
Contributor

Should be possible to do the states in YAML as well, but tomorrow will be another day.

@librenms-bot
Copy link

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/juniper-loss-not-polling-correctly/16628/3

@PipoCanaja
Copy link
Contributor

All done @h-barnhart
Let me know for the discovery and poller modules, and if we can clean them all, it will be ready to merge.

@PipoCanaja PipoCanaja changed the title New OS - Ciena RLS 6500 devices - Ciena RLS 6500 Apr 1, 2024
@h-barnhart
Copy link
Contributor Author

Those poller and discovery modules aren't used, I was trying to be thorough. I put them back and push a new change.

Thanks for looking into the YAML. If you want me to put the temp sensor back in YAML I can and try out the DBM and State sensors as well. I was hoping to get this in before the next full release though.

@h-barnhart
Copy link
Contributor Author

Those poller and discovery modules aren't used, I was trying to be thorough. I put them back and push a new change.

Thanks for looking into the YAML. If you want me to put the temp sensor back in YAML I can and try out the DBM and State sensors as well. I was hoping to get this in before the next full release though.

Oh, you did all that. Thanks

I re-enabled the poller and discovery modules as requested.

Copy link
Contributor

@PipoCanaja PipoCanaja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanx for the PR.

@PipoCanaja PipoCanaja merged commit d457f92 into librenms:master Apr 1, 2024
8 checks passed
@h-barnhart h-barnhart deleted the os-ciena-rls6500 branch April 12, 2024 13:14
@librenms-bot
Copy link

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/24-4-0-changelog/24157/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Device 🖥️ New or added device support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants