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

Fixed ifType being removed when a port is down #9493

Merged
merged 9 commits into from Dec 12, 2018

Conversation

PipoCanaja
Copy link
Contributor

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.

Hello,

This PR is adding ifType to table_base_oids. Without it, a port down looses its type, making it difficult to filter on this criteria. It seems that ifType is not a costly OID so it should not be to bad to the performance.
I run this change on my setup for a while, but this is of course not significant considering the large amount of devices around. At least on a few Cisco switches and mostly Huawei devices, it did not make a change to add it.

Waiting for feedbacks ;)

Bye
PipoCanaja

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.

@murrant
Copy link
Member

murrant commented Nov 30, 2018

Perhaps it would be better to prevent ifType from being deleted when a port is down (I'm assuming selective port polling)?

@PipoCanaja
Copy link
Contributor Author

Assuming right for selective port polling.
The idea would be to get ifType from discovery process, and to avoid deleting it during polling ? I'll have a look at this approach, which makes sense.
Bye for now

@PipoCanaja
Copy link
Contributor Author

PipoCanaja commented Nov 30, 2018

Quick update, seems that the discovery process does not put ifType in DB. So it is not as easy as I first hoped ;)
I suppose we should first add ifType into the discovery process (which means it will be at least updated every occurence of discovery) and keep the value in between (I have a patch for this already).

This could even be chosen as the default behaviour for this OID cause ifType is a very stable value I would say, stable enough to leave poller and be only in discovery ... What do you think ?

@laf
Copy link
Member

laf commented Nov 30, 2018

I suppose we should first add ifType into the discovery process (which means it will be at least updated every occurence of discovery) and keep the value in between (I have a patch for this already).

This makes more sense than increasing polling times.

@laf laf added this to the 1.47 milestone Nov 30, 2018
@PipoCanaja
Copy link
Contributor Author

PipoCanaja commented Dec 1, 2018

Polling time unchanged in current state of the PR. Instead, the data already collected during the discovery process is saved or updated, and the polling keeps the values unchanged if selective polling is enabled.
Of course, tests are now broken (more data is saved during discovery).

@murrant
Copy link
Member

murrant commented Dec 5, 2018

Tests can be fixed my running ./scripts/save-test-data.php -m ports, this will cause a lot of modified files. If you want I can run it and push it to your branch...

murrant
murrant previously approved these changes Dec 5, 2018
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.

Looks good, just needs test data update.

@PipoCanaja
Copy link
Contributor Author

Hi @murrant
I would agree to your proposition to run the tests. I am not comfortable doing it.
PipoCanaja

@laf
Copy link
Member

laf commented Dec 5, 2018

I've pushed the tests up.

@laf laf added the Bug 🕷️ label Dec 5, 2018
@laf laf changed the title Adding ifType to base_oids Fixed ifType being removed when a port is down Dec 5, 2018
@murrant murrant merged commit b67634b into librenms:master Dec 12, 2018
@PipoCanaja PipoCanaja deleted the ifTypeToBase branch December 12, 2018 11:38
@lock lock bot locked as resolved and limited conversation to collaborators Feb 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants