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: rewrite is_valid_port() #7337

Merged
merged 1 commit into from Sep 16, 2017

Conversation

Projects
None yet
6 participants
@murrant
Copy link
Member

murrant commented Sep 14, 2017

Convert is_valid_port to Config
Several Config optimizations
Update documentation

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

refactor: speed up is_valid_port()
Convert is_valid_port to Config
Several Config optimizations
Update documentation
@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Sep 14, 2017

Thank you for submitting a PR @murrant! We have found the following @laf, @zarya and @Rosiak based on the history of these files to review this PR.

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Sep 14, 2017

The inspection completed: 5 updated code elements

empty_ifdescr: false # allow empty ifDescr
bad_if: # ifDescr (substring, case insensitive)
- lp0
bad_if_regexp: # ifDescr (regex, case insensitive)

This comment has been minimized.

@laf

laf Sep 14, 2017

Member

Is it not case insensitive depending on the regex? Same for the other two _regexp ones

This comment has been minimized.

@murrant

murrant Sep 16, 2017

Author Member

Previous code appended i on the end of the regex, so I left it this way.

@laf

This comment has been minimized.

Copy link
Member

laf commented Sep 14, 2017

Testing now

@laf

This comment has been minimized.

Copy link
Member

laf commented Sep 14, 2017

After the docs confirmation then this LGTM

@laf laf merged commit 3160741 into librenms:master Sep 16, 2017

2 of 3 checks passed

code-review/reviewable 5 files, 1 discussion left (laf)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@laf laf deleted the murrant:is-port-config branch Sep 16, 2017

@peelman

This comment has been minimized.

Copy link
Contributor

peelman commented Sep 17, 2017

This, or something akin to it, seems to be affecting our production config. After tonight's Daily.sh run, we began discovering all interfaces, including ones explicitly ignored in config.php.

Specifically:

$config['bad_ifname_regexp'][] = '/^(Ont|ont) \d{1,3}.*/';
$config['bad_ifname_regexp'][] = '/^Dslport.*/';

So I just went from a from at most a manageable polling window to an untenable one.

laf added a commit that referenced this pull request Sep 17, 2017

if (!isset($config['os'][$os][$key])) {
if (!str_contains($key, '.')) {
return $default;
}

This comment has been minimized.

@network-guy

network-guy Sep 17, 2017

Contributor

This is causing issues. It's failing validation for the $key value and not returning it. Commenting out lines 141-143 fixes the issue. Is there a reason it's looking for a period in the string, since str_contains doesn't do regex checking?

This comment has been minimized.

@murrant

murrant Sep 18, 2017

Author Member

Yes, if the string doesn't contain a period, we don't need to try to split it apart and walk the config array.

Can you give an example of what setting(s) are set that is causing the issues?

This comment has been minimized.

@laf

laf Sep 18, 2017

Member

It's anything for the is_port_valid(). I tried $config['bad_ifname_regexp'][] = '/^Dslport.*/';

It's because it gets the str_contains bit int he first place. That config above will always match the !isset and so move on to returning the default.

This comment has been minimized.

@peelman

peelman Sep 18, 2017

Contributor

For me it was anything. Even the default ignored interfaces were not being ignored. I had a ton of ATM and VMware vSwitch interfaces show up that didn't exist before 1am Sunday.

The two I really cared most about though were:

$config['bad_ifname_regexp'][] = '/^(Ont|ont) \d{1,3}.*/';
$config['bad_ifname_regexp'][] = '/^Dslport.*/';

As we have a lot of Calix equipment and our Libre box started shouting alerts into Slack for ONT ports and DSL ports being down, (as people's equipment was rebooting, etc), polling was taking 15-20 minutes per machine, and the load on the server was in the triple digits.

so yeah...

I've taken us out of the daily master update and moved us to the release cycle. Libre is becoming too critical for us to stay on the bleeding edge I guess.

@network-guy

This comment has been minimized.

Copy link
Contributor

network-guy commented Sep 17, 2017

@murrant @laf

Added a comment to the functions.php code. the following code is the culprit for @peelman's comment.

if (!str_contains($key, '.')) { return $default; }

Once I commented it out my alerts have normalized.

laf added a commit that referenced this pull request Sep 17, 2017

Revert "refactor: rewrite is_valid_port()" (#7355)
* Revert "refactor: Added detection of vlan name changes (#7348)"

This reverts commit 4ad8fae.

* Revert "refactor: Rewrite is_valid_port() (#7337)"

This reverts commit 3160741.
@laf

This comment has been minimized.

Copy link
Member

laf commented Sep 17, 2017

Thanks. I've reverted it as it's got at least another place that's breaking it depending on user config

@lock

This comment has been minimized.

Copy link

lock bot commented May 17, 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 17, 2018

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