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

Initial pull request for the speedest plugin #2298

Closed
wants to merge 4 commits into from

Conversation

mihakralj
Copy link

passes style tests, includes copyright licenses.

passes style tests, includes copyright licenses
public function testAction($serverid = 0)
{
$backend = new Backend();
$response = trim($backend->configdRun("speedtest run ${serverid}"));
Copy link
Member

Choose a reason for hiding this comment

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

you can pass parameters using ->configdpRun() e.g.

(new Backend())->configdpRun("speedtest run", [$serverid]);

This helps to make the distinction between commands and parameters more explicit and escapes parameters in the process.

Copy link
Author

Choose a reason for hiding this comment

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

Do you want me to refactor the controller for this release or schedule the changes for the next update? This jello works at the moment, I will create a new fork and start on optimization and refactoring.

@AdSchellevis
Copy link
Member

just a small question, wouldn't it be simpler to control speedtest-cli via configd? I guess it would save a lot of plumbing and might help simplify quite some code.

@mihakralj
Copy link
Author

is there a good example of a plugin (or a core service) that I can look at?

@lattera
Copy link

lattera commented Mar 23, 2021

I'd probably go with speedtest-cli as well. Or, at the very least, turn benchmarks/speedtest/src/opnsense/scripts/OPNsense/speedtest/speedtest.py into a port, using that instead of directly embedding the script into OPNsense. But, it seems using speedtest-cli would probably be the easier route.

@mihakralj
Copy link
Author

btw, the CLI version of speedtest is sunsetting and slowly replaced by socket-based speedtest binary. That's why I included both...

@AdSchellevis
Copy link
Member

I would try to keep things simple. py-speedtest-cli is being build in our ports anyways and works like a charm as far as I know. (https://github.com/opnsense/ports/tree/master/net/py-speedtest-cli)

@mihakralj
Copy link
Author

mihakralj commented Mar 23, 2021

I would be happy to pull both speedtest binary and speedtest.py out from the plugin package and add them as an external dependencies - but I need some help here; how can I nudge pkg process to install py-speedtest-cli dependency (which is in ports but not in any of the default repositories) and how can I instruct pkg to install speedtest binary (which is available as a binary on ookla's site but not in any repo that I know)? With that, I can just declare dependencies and let BSD pkg installer handle the rest...

From: https://www.speedtest.net/apps/cli#freebsd:
sudo pkg add "https://bintray.com/ookla/download/download_file?file_path=ookla-speedtest-1.0.0-freebsd.pkg"

@mimugmail
Copy link
Member

py-speedtest-cli is already in repo ( https://pkg.opnsense.org/FreeBSD:12:amd64/21.1/MINT/21.1.3_3/OpenSSL/All/py37-speedtest-cli-2.1.2.txz ). When there is a pkg in any repo you can use plugin depends, otherwise you need to include it in the plugin, but this is highly unusual since noone can verify the real source if it.

@mihakralj
Copy link
Author

you named it py37-speedtest-cli? Gosh! OK, this one is resolved and I'll pull it out from the plugin. :-) How about Ookla's binary?

@AdSchellevis
Copy link
Member

I don't think we ship Ookla's binary, my advise is to stick to what's available first.

@mihakralj
Copy link
Author

the binary speedtest (that uses sockets) delivers different (better?) results. Don't ask me to rip it out from the plugin...

@AdSchellevis
Copy link
Member

If you plan to make it available in the OPNsense plugins repo, that's likely the question yes. We're not going to ship binaries.

@mihakralj
Copy link
Author

mihakralj commented Mar 23, 2021

I started the question on OPNsense forum: what would be a process to fire pkg add as part of package install? If I add it to +POST_INSTALL, pkg complains that the DB is locked

@fichtner
Copy link
Member

It does not matter where the binary is from or if it is available as a package somewhere else. We need a FreeBSD port in source code to add as a dependency to be part of the whole build process.

@mihakralj
Copy link
Author

ok, I'll cut it out. Are you ok that I keep instructions on how users can swap out CLI and replace it with binary?

@mihakralj
Copy link
Author

This is a significant rewrite of the initial PR code:

  • no speedtest packages included; also no dependencies during the install; if no speedtest found, UI provide a message how to manually install either HTTP speedtest from ports or Ookla binary
  • a helping script install_speedtest.sh helps the user to automate installation and change of speedtest package
  • accordingly, the Python wrapper code is redesigned to recognize what speedtest package is used and ensures consistent results despite different parameters and output from each speedtest package
  • streamlined APIs that consistently present results regardless of the type of speedtest package used
  • UI displays the type/version of speedtest underneath the box

@mihakralj mihakralj closed this Apr 4, 2021
@mihakralj
Copy link
Author

Let's not waste time on this review as I already refactored some portions...

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

Successfully merging this pull request may close these issues.

None yet

5 participants