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

WIP - Rewrite Wireless Controller polling #13198

Open
wants to merge 48 commits into
base: master
Choose a base branch
from

Conversation

ottorei
Copy link
Contributor

@ottorei ottorei commented Sep 7, 2021

Modernize old code for wireless controllers polling. Work in progress.

TODO:

  • Update web-page to allow the use of the new module
  • Update all database queries to Eloquent in the poller module
  • Implement poller functions for Aruba WLC-devices
  • Implement poller functions for Cisco WLC-devices
  • Implement poller functions for VRP WLC-devices
  • Implement syncmodels for DB
  • Save WLC and AP RRD-data
  • Automatic cleanup option, user configurable
  • Existing alerting breaks for deleted access points with this, figure out something

Notice: Each AP is uniquely distinguished by it's MAC-address + radionumber. This should work for most devices and would allow the AP to be moved to another controller without issues on the NMS.

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 13198
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.

app/Models/AccessPoint.php Outdated Show resolved Hide resolved
@murrant
Copy link
Member

murrant commented Sep 8, 2021

@ottorei Excited to see this, let us know if you have any questions :D

@ottorei ottorei closed this Sep 8, 2021
@ottorei ottorei deleted the accesspoint-duplicates branch September 8, 2021 14:43
@ottorei ottorei restored the accesspoint-duplicates branch September 8, 2021 15:04
@ottorei ottorei reopened this Sep 8, 2021
@ottorei ottorei force-pushed the accesspoint-duplicates branch 2 times, most recently from e78a6dd to b083381 Compare September 8, 2021 18:19
@ottorei
Copy link
Contributor Author

ottorei commented Sep 9, 2021

I am having some issues with the syncmodel as for some reason it can not sync on dev machine. I think this has something do to with the DB being empty.

Call to a member function getCompositeKey() on null
{"exception":"[object] (Error(code: 0):
Call to a member function getCompositeKey() on null at /opt/librenms/LibreNMS/DB/SyncsModels.php:43)"}

app/Models/AccessPoint.php Outdated Show resolved Hide resolved
return "$this->mac_addr-$this->radio_number";
}

public function setOffline()
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this implemented, I would just need to use withThrashed to get all aps?

Copy link
Member

Choose a reason for hiding this comment

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

@ottorei correct

Copy link
Member

Choose a reason for hiding this comment

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

and it stores the deleted date, so we can do a cleanup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and it stores the deleted date, so we can do a cleanup.

Yes, with a user configurable amount of days.

@ottorei
Copy link
Contributor Author

ottorei commented Sep 9, 2021

I am having some issues with the syncmodel as for some reason it can not sync on dev machine. I think this has something do to with the DB being empty.

Call to a member function getCompositeKey() on null
{"exception":"[object] (Error(code: 0):
Call to a member function getCompositeKey() on null at /opt/librenms/LibreNMS/DB/SyncsModels.php:43)"}

With the latest commit, APs can now be saved and the error message is no longer present after polling.

@ottorei
Copy link
Contributor Author

ottorei commented Oct 29, 2021

@ottorei what do you need to continue with this?

Would need to convert the existing wireless controllers code use this new module. The Aruba one seems to be working to some extent but I have not started the work with others yet.

@Jellyfrog Jellyfrog closed this Nov 2, 2021
@Jellyfrog Jellyfrog reopened this Nov 2, 2021
@Jellyfrog
Copy link
Member

(Rerunning tests)

@Jellyfrog Jellyfrog marked this pull request as draft November 2, 2021 23:12
@ottorei
Copy link
Contributor Author

ottorei commented Nov 5, 2021

Converted the Cisco WLC to the new module. Not really tested yet.

@ottorei ottorei marked this pull request as ready for review November 5, 2021 18:44
@ottorei
Copy link
Contributor Author

ottorei commented Nov 5, 2021

Still not quite sure what to do with the soft deleted models. The existing alert rules for "deleted" APs will break with this PR if the deleted-field is removed from the DB. What kind of approach should I take here to still allow users to make alert rules with the query builder for these @murrant ? A macro...?

@Jellyfrog
Copy link
Member

Jellyfrog commented Nov 5, 2021

Please note we got stricter php linting now, it's 99% about type hinting. But the tests will tell you what to do :)

@murrant
Copy link
Member

murrant commented Nov 6, 2021

Are you sure the existing rule will break? Type juggling might make them work still. Also, see #13480 to merge all files to one RRD scheme.

@ottorei ottorei changed the title WIP - Modernize Wireless Controller polling WIP - Rewrite Wireless Controller polling Nov 28, 2021
@ottorei
Copy link
Contributor Author

ottorei commented Nov 28, 2021

Are you sure the existing rule will break? Type juggling might make them work still. Also, see #13480 to merge all files to one RRD scheme.

Yes, since the existing database column is named "deleted". But I remember that alert was not on the list of predefined rules.

@murrant
Copy link
Member

murrant commented Nov 29, 2021

@ottorei for alerting, we have in the past updated existing alert rules in the db.

@ottorei
Copy link
Contributor Author

ottorei commented Dec 2, 2021

@ottorei for alerting, we have in the past updated existing alert rules in the db.

You mean the sample rules?

@murrant
Copy link
Member

murrant commented Dec 2, 2021

No, I mean direct in the database ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants