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

Improved fdb search #8251

Merged
merged 2 commits into from
Apr 7, 2018
Merged

Improved fdb search #8251

merged 2 commits into from
Apr 7, 2018

Conversation

wiad
Copy link
Contributor

@wiad wiad commented Feb 14, 2018

by filtering and consolidating results + adding ip address search. The object is to only display the switch/port where the device is actually connected and filter out all other switches which may have the mac address in the fdb table.

Further described in community post:

https://community.librenms.org/t/better-output-from-fdb-search/3290

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

@CLAassistant
Copy link

CLAassistant commented Feb 14, 2018

CLA assistant check
All committers have signed the CLA.

@wiad
Copy link
Contributor Author

wiad commented Feb 15, 2018

Right now my sql query only selects ports where the mac address of the device appears as the single device, i.e. no other mac addresses are connected to that port. I realize that we probably rather want it to select the ports where the mac address appears and where the total amount of mac addresses are fewest, so we can filter out port channels and such which contains a lot of mac addresses and still support unmanaged switches sitting between a device and our managed switch. For example, if a mac address appears at portA/deviceA, portB/deviceB and portC/deviceC, and portA has 20 mac addresses, portB has 10 and portC has 3, portC should be selected.

I'll see if I can come up with something.

Copy link
Member

@laf laf left a comment

Choose a reason for hiding this comment

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

Works for me. I'd like a second opinion to be sure so tagging @murrant.

You also need to fix the CLA signing. You should probably just squash all the commits into one and force push as the last user you used, should be fine after that.

@XioNoX
Copy link
Contributor

XioNoX commented Feb 28, 2018

That's useful thanks!
Note that it's sometimes useful for me to see where a specific MAC is learned, across the whole infra.
So ideally this would not replace the current search, but be something additional.

@wiad
Copy link
Contributor Author

wiad commented Feb 28, 2018

This commit effectively changes the original behaviour. If there are use cases where you actually want to see all ports connected to a mac address then maybe this should be optional - a checkbox or something letting the user decide if the results should be filtered or not? Or a form letting the user decide how many occurences of a port_id should be allowed in the filter. In my commit this is hard-coded to '8' to allow devices behind smaller unmanaged switches still be visible, but the hardcoded part really irks me and will probably not fit everyone.

@kkrumm1
Copy link
Member

kkrumm1 commented Feb 28, 2018

Tested on my test box works for me. :)

@murrant
Copy link
Member

murrant commented Feb 28, 2018

This is ok with me.

I wanted to combine, mac, arp, and fdb search together at some point... probably list all ports/devices it is seen on but highlight the likely connected port.

@laf
Copy link
Member

laf commented Mar 5, 2018

@wiad If you want to add an additional flag to allow the original behavior then feel free.

Can you fix the travis issue thouhg: https://travis-ci.org/librenms/librenms/jobs/347145362#L660

@wiad
Copy link
Contributor Author

wiad commented Mar 6, 2018

So I added a filter in the search page so the user can set how many mac addresses per port are acceptable in the output. It's difficult to make the meaning of the option easy to understand though.

laf
laf previously approved these changes Mar 11, 2018
Copy link
Member

@laf laf 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 to me.

@kkrumm1
Copy link
Member

kkrumm1 commented Mar 12, 2018

Tested in my dev environment. Seems to be working

@kkrumm1
Copy link
Member

kkrumm1 commented Mar 16, 2018

Is there anything that's holding this up from merging?

@murrant
Copy link
Member

murrant commented Mar 17, 2018

Ok, got around to testing this. I've got a problem. Looking at my production environment, even when I set to 32 macs per port most of my ports are excluded.

I like the goal of this PR, but I think the implementation could use some refinement.

Perhaps remove the dropdown and visually mark the port with the least macs? An order by field?

@wiad
Copy link
Contributor Author

wiad commented Mar 21, 2018

@murrant Do you have ports associated with your searchphrase which has less than 32 mac addresses associated, which does not show up? Unless its a very small network, most ports will be excluded except the 'endpoint' port if you're using the macperport filter. The reason for having multiple choices in that filter is just to accommodate for different sizes of unmanaged switches. In my environment I only have small 8-port unmanaged switches but in other environments other sizes might be used.

So, if you only want to see the endpoint switchport, use the macperport filter. If you have unmanaged switches, set the filter number to match the unmanaged switch size, otherwise just use '4' or '2'.

That said, I'm not really satisfied with this solution myself. Visually marking the endpoint port is an option, but i dont think I have the sql in me to get to that. I guess I could give it another go...

@murrant
Copy link
Member

murrant commented Mar 22, 2018

That's what I'm saying is most of my ports have more than 32 MACs :)

@wiad
Copy link
Contributor Author

wiad commented Mar 23, 2018

I've done some work with this and have an ORDER BY solution which presents the endpoint port first in the list. I'm now looking into visually marking the entry as well. This solution feels better to me.

@wiad wiad force-pushed the community-3290 branch 2 times, most recently from d846e24 to 1920360 Compare March 23, 2018 10:13
murrant
murrant previously approved these changes Mar 23, 2018
@kkrumm1
Copy link
Member

kkrumm1 commented Mar 24, 2018

good to go to the master?

@murrant
Copy link
Member

murrant commented Mar 29, 2018

If I have approved something but not merged it that means it looks good, but I haven't personally tested it. I generally don't merge something unless I have tested it.

@laf
Copy link
Member

laf commented Mar 29, 2018

Merge conflict though, can you fix that @wiad

…ndpoint switchport first in list. Also add IP search to fdb search
@scrutinizer-notifier
Copy link

The inspection completed: No new issues

Copy link
Member

@laf laf left a comment

Choose a reason for hiding this comment

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

I'm fine with this. @librenms/reviewers?

@laf laf merged commit 2ece844 into librenms:master Apr 7, 2018
@laf laf added the WebUI label Apr 7, 2018
@wiad wiad deleted the community-3290 branch April 11, 2018 05:39
TheMysteriousX pushed a commit to TheMysteriousX/librenms that referenced this pull request May 20, 2018
* sort fdb search by number of mac addresses/port to show most likely endpoint switchport first in list. Also add IP search to fdb search

* fix style check error
@lock lock bot locked as resolved and limited conversation to collaborators Jun 10, 2018
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.

7 participants