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

Added Vlan discovery on LAG ports, Jetstream OS #13007

Merged
merged 8 commits into from Jul 11, 2021
Merged

Added Vlan discovery on LAG ports, Jetstream OS #13007

merged 8 commits into from Jul 11, 2021

Conversation

Npeca75
Copy link
Contributor

@Npeca75 Npeca75 commented Jul 7, 2021

Vlans discovery on LAG ports on TP-LINK Jetstram OS was missing

DO NOT DELETE THE UNDERLYING TEXT

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.

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.

@CLAassistant
Copy link

CLAassistant commented Jul 7, 2021

CLA assistant check
All committers have signed the CLA.

@Jellyfrog Jellyfrog added the Device 🖥️ New or added device support label Jul 7, 2021
@PipoCanaja
Copy link
Contributor

Hi @Npeca75
Please check the style tests and update your code accordingly :
https://github.styleci.io/analyses/Vrpa53?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

And please provide test files (snmprec and JSON) as required.

Thanx

@Npeca75
Copy link
Contributor Author

Npeca75 commented Jul 8, 2021

Hi @PipoCanaja

sorry for mess, i am totally noob on GITHUB
looks like code check is OK
attached the snmpwalk from Jetsream switch with LAG's
and a screenshot, clearly visible that Vlans are added on Po1, Po2 ...

Screenshot_20210708_164311

Jetstream-LAG.walk.txt

unset($result);

if ($pos = strpos($var, ',LAG')) { // if input string contains 'LAG'
$lag = str_replace('LAG', '', substr($var, $pos + 1)); // get the LAG ports in form of '1,2,4-8'
Copy link
Member

Choose a reason for hiding this comment

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

This code is really hard to read, could it perhaps be done with a regex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @murrant

sorry, my "native" language is assembler on embedded devices, so i am not familiar with PHP
if you have any suggestion ...
TP-LINK made own way of representing Vlans in snmp
for example:
STRING: "1/0/1-2,1/0/4-6,1/0/25,LAG1-2,LAG4"
as you see, port 1,2,4,5,6,25 are tagged
and LAG1,2,4 are tagged
so, the information about ports and LAG's are in one line
it need to be splitted on two parts, first for ports, second for LAG's
then DASH need to be replaced with corresponding numbers
4-6 -> 4,5,6
same technique for PORTS (left) part of string
and same for LAG's (right) part of string

anyway, if you could / want to rewrite this i will be happy

Copy link
Member

Choose a reason for hiding this comment

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

Try using a site like regex101.com https://regex101.com/r/7utMDz/1

Sample code:

$string = '1/0/1-2,1/0/4-6,1/0/25,LAG1-2,LAG4';
preg_match_all('/LAG(\d+)(?:-(\d+))?/', $string, $matches);
foreach ($matches[1] as $index => $start) {
    $end = $matches[2][$index] ?: $start;
    for ($i = $start; $i <= $end; $i++) {
        dump((int)$i);
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @murrant
following your advice , the whole function was rewritten ...

function jetstreamExpand($var)
{
    unset($result);

    preg_match_all('/LAG(\d+)(?:-(\d+))?/', $var, $lags);

    foreach ($lags[1] as $index => $start) {
        $end = $lags[2][$index] ?: $start;
        for ($i = $start; $i <= $end; $i++) {
            $result[] = 'LAG' . $i; //need to be in form LAGx
        }
    }

    preg_match_all('/1\/0\/(\d+)(?:-(\d+))?/', $var, $ports);
    foreach ($ports[1] as $index => $start) {
        $end = $ports[2][$index] ?: $start;
        for ($i = $start; $i <= $end; $i++) {
            $result[] = '1/0/' . $i; //need to be in form 1/0/x
        }
    }

    return $result;
}

@murrant murrant added the Needs Tests 🦄 https://docs.librenms.org/Developing/os/Test-Units/ label Jul 9, 2021
@label-actions
Copy link

label-actions bot commented Jul 9, 2021

Please add test data so we can ensure your change is not broken in the future.

Read the docs to find out how: https://docs.librenms.org/Developing/os/Test-Units

@Npeca75
Copy link
Contributor Author

Npeca75 commented Jul 9, 2021

Hi @Npeca75

And please provide test files (snmprec and JSON) as required.

Ok, i hope these files are enough for testing

jetstream.snmprec.txt
jetstream.snmpwalk.txt

@Jellyfrog
Copy link
Member

Please add test data so we can ensure your change is not broken in the future.

Read the docs to find out how: https://docs.librenms.org/Developing/os/Test-Units

Please see this on how to generate it. And it needs to be commited also

@Npeca75
Copy link
Contributor Author

Npeca75 commented Jul 11, 2021

hi @Jellyfrog

Please add test data so we can ensure your change is not broken in the future.

ok
./scripts/collect-snmp-data.php -h 1 -v t1600g-28ts

and pushed to GIT

@murrant
Copy link
Member

murrant commented Jul 11, 2021

Looks much better @Npeca75 I've generated the test data for you and uploaded it. Here is the command (since vlans is not enabled by default)
./scripts/save-test-data.php -o jetstream -v t1600g-28ts -m vlans

}
} else {
$result[] = $element;
preg_match_all('/LAG(\d+)(?:-(\d+))?/', $var, $lags);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, we could combine these two regex. :)

@Npeca75
Copy link
Contributor Author

Npeca75 commented Jul 11, 2021

hi @murrant

i get this error running snmpsim

./scripts/save-test-data.php -o jetstream -v t1600g-28ts -m vlans

Starting snmpsim listening on 127.1.6.1:1161...
OS: jetstream
Module: vlans
Variant: t1600g-28ts

jetstream_t1600g-28ts: Could not connect to 127.1.6.1, please check the snmp details and snmp reachability

Copy link
Member

@murrant murrant left a comment

Choose a reason for hiding this comment

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

Nice :)

@murrant
Copy link
Member

murrant commented Jul 11, 2021

@Npeca75 it requires you have a working snmpsim on your system.

@murrant murrant merged commit 1b68273 into librenms:master Jul 11, 2021
@murrant murrant removed the Needs Tests 🦄 https://docs.librenms.org/Developing/os/Test-Units/ label Jul 11, 2021
@librenms-bot
Copy link

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

https://community.librenms.org/t/21-7-0-changelog/16425/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

6 participants