Benchmark cache #955

jessevz · 2023-07-25T10:33:59Z

made a cache for the benchmarks so that benchmarks can be reused and the benchmark proces can be skipped when the benchmark has already been cached.
Key components:

Added a benchmark model. The benchmark model got a field for all the dependencies of the benchmark output. These are the benchmark type (speed or runtime), the cracker binary, the hardware that is used, the attack parameters and the hashmode.
Agents are split up into hardware groups, so that benchmark can be linked to a hardwaregroup and not a single agent.
Benchmarks get invalidated by a time to live.
frontend for the benchmarks in /benchmark.php to show what has been cached and to manually remove values.

…d hardwareGroup

…ke use of benchmark caching

…e of bug

s3inlc · 2023-08-07T14:51:17Z

Thanks for the PR :)
There are a few additional things I noticed:

The code style was not consistently applied, there are quite a few functions which have the curly brackets on new lines after function definitions.
The new feature should also be documented in doc/changelog.md
Probably the TTL should be configurable as different users may have different times they would like to have benchmark results cached (ideally, even add that e.g. if it is set to 0, the caching will be disabled).
I tried to test the feature, but it seems that the upgrade process is somehow broken, during initialization it fails with the following error:

hashtopolis-backend     | Start initialization process...
hashtopolis-backend     | PHP Fatal error:  Uncaught PDOException: SQLSTATE[42000]: Syntax error or access violation: 1075 Incorrect table definition; there can be only one auto column and it must be defined as a key in /var/www/html/install/updates/update_v0.14.x_v0.14.0.php:10
hashtopolis-backend     | Stack trace:
hashtopolis-backend     | #0 /var/www/html/install/updates/update_v0.14.x_v0.14.0.php(10): PDO->query()
hashtopolis-backend     | #1 /var/www/html/install/updates/update.php(53): include('...')
hashtopolis-backend     | #2 /var/www/html/inc/load.php(162): include('...')
hashtopolis-backend     | #3 {main}
hashtopolis-backend     |   thrown in /var/www/html/install/updates/update_v0.14.x_v0.14.0.php on line 10

and when accessing the benchmark page it is missing the tables which are commented out in the update script.

s3inlc · 2023-08-07T13:51:36Z

src/install/updates/update_v0.14.x_v0.14.0.php

While the release is not fixed and in preparation, the update files typically are named ./update_v0.14.x_v0.x.x.php.

s3inlc · 2023-08-07T13:51:57Z

src/install/hashtopolis.sql

 ALTER TABLE `Agent`
  ADD PRIMARY KEY (`agentId`),
  ADD KEY `userId` (`userId`);
+  -- ADD KEY `hardwareGroupId` (`hardwareGroupId`);


why is this commented out?

s3inlc · 2023-08-07T13:52:09Z

src/install/hashtopolis.sql

@@ -1300,6 +1331,7 @@ ALTER TABLE `AccessGroupUser`

 ALTER TABLE `Agent`
  ADD CONSTRAINT `Agent_ibfk_1` FOREIGN KEY (`userId`) REFERENCES `User` (`userId`);
+  -- ADD CONSTRAINT `Agent_ibfk_2` FOREIGN KEY (`hardwareGroupId`) REFERENCES `HardwareGroup` (`hardwareGroupId`);


why is this commented out?

s3inlc · 2023-08-07T13:54:23Z

src/install/updates/update_v0.14.x_v0.14.0.php

+<?php /** @noinspection SqlNoDataSourceInspection */
+use DBA\Factory;
+
+if (!Util::databaseTableExists("HardwareGroup")) {


Typically, we wrap upgrade steps also with in a check of a unique key which is stored in database to determine if the update part was already executed or not. Of course, if databaseTableExists() is checked, it would not execute it twice anyway. But with having a key stored in the database, it's also easier to look up in case of issues and to keep consistency overall, we should always have this wrap with a key (as seen in the other update files).

s3inlc · 2023-08-07T13:55:56Z

src/install/updates/update_v0.14.x_v0.14.0.php

+  Factory::getAgentFactory()->getDB()->query("ALTER TABLE `HardwareGroup` ADD PRIMARY KEY (`hardwareGroupId`);");
+}
+
+//change Agent table


I guess this should not be commented out? Otherwise I think this would break existing installations as the Agent table would not be as expected with the benchmark caching code.

The same probably applies for the Benchmark table and the adding of the foreign key constraints.

…rect format

jessevz · 2023-08-11T11:35:58Z

@s3inlc thanks for the review! I have changed my updatescript and I think it works properly now. But there is still something that i do not understand, and that is when I delete my docker volumes to make Hashtopolis run my install script, it will also run my update script afterwards which will fail since it will try to make the same tables for the second time. What should I change so that the updatescript doesnt get run after an install?

… when creating new agent

…l when creating new agent

s3inlc · 2023-08-24T12:13:47Z

Thanks for the changes and sorry for the waiting time. I tested it more in detail now, there are a few things left which should be fixed which I'll put into the review. Regarding your question for the update script, essentially it's double check, but normally we do the check with the key (as you implemented now) and inside that we do again a check if the table already exists. This then covers all cases of setups from a release, from pulled master, etc.

When I tested the benchmark caching one small point came up. When caching the benchmark it is independent from the agent specific parameters. Though this can have a large influence, e.g. if someone decides to use a machine with multiple GPUs, but e.g. runs multiple agents with one using one of the GPUs each (using -d ). Maybe that is something to consider, that there may be agent specific parameters which should be taken into account as part of the benchmark cache CMD?

s3inlc

I went over the changes in detail and made comments where things need to be addressed.

s3inlc · 2023-08-24T12:16:34Z

src/benchmark.php

+  if (!$agent) {
+    UI::printError("ERROR", "Agent not found!");
+  } else {
+  $qF = new QueryFilter("agentId", $agent->getId(), "="); 


indentation needed for line 33 and 34

s3inlc · 2023-08-24T12:20:40Z

src/benchmark.php

+
+if (isset($_GET['id'])) {
+  //go to detail page
+  // Template::loadInstance("benchmark/detail");


why is the ID which is retrieved here used as the agent ID? Shouldn't that be the benchmark ID which is queried here and then shown?
When being accessed with benchmark.php?id=xy it will produce internal server errors, because the benchmark table does not have an agentId column.

s3inlc · 2023-08-24T12:30:52Z

src/dba/models/HardwareGroupFactory.class.php

+   * @return HardwareGroup
+   */
+  function createObjectFromDict($pk, $dict) {
+    $o = new HardwareGroup($dict['hardwareGroupId'], $dict['devices'], $dict['benchmarkId']);


This seems to be an inconsistency between the model and what is expected here. Maybe this should be re-generated again, as benchmarkId is not existing in the HardwareGroup model.

s3inlc · 2023-08-24T12:32:04Z

src/inc/api/APIUpdateClientInformation.class.php

    Factory::getAgentFactory()->mset($this->agent, [
-        Agent::DEVICES => htmlentities(implode("\n", $devices), ENT_QUOTES, "UTF-8"),
+        //change to hardware group


as this gets removed, I would propose to delete it. It will not be needed again later.

s3inlc · 2023-08-24T12:32:26Z

src/inc/defines/accessControl.php

@@ -26,6 +26,8 @@ class DAccessControl {
  const SERVER_CONFIG_ACCESS        = "serverConfigAccess";
  const USER_CONFIG_ACCESS          = "userConfigAccess";
  const MANAGE_ACCESS_GROUP_ACCESS  = "manageAccessGroupAccess";
+  const VIEW_BENCHMARK_ACCESS  = "ViewBenchmarkAccess";


I would indent the same as with all the other values.

s3inlc · 2023-08-24T12:51:14Z

src/templates/benchmarks/index.template.html

+      <thead>
+        <tr>
+          <th>id</th>
+          <th>Cmd</th>


I would write CMD

s3inlc · 2023-08-24T12:51:26Z

src/templates/benchmarks/index.template.html

+    <table class="table table-bordered table-sm" id="benchmarks">
+      <thead>
+        <tr>
+          <th>id</th>


I would write ID

s3inlc · 2023-08-24T12:51:37Z

src/templates/benchmarks/index.template.html

+          <th>CPU/GPU</th>
+          <th>Benchmark value</th>
+          <th>ttl</th>
+          <th>action</th>


I would write Action

src/benchmark.php

s3inlc · 2023-08-24T13:01:21Z

src/templates/struct/menu.template.html

 							<a class="dropdown-item[[menu.isActive('agents_status')]]" href="agentStatus.php">Agent Status</a>
 						{{ENDIF}}
+            			{{IF [[accessControl.hasPermission([[$DAccessControl::VIEW_BENCHMARK_ACCESS]])]]}}
+              				<a class="dropdown-item[[menu.isActive('Benchmark_cache')]]" href="benchmark.php">Benchmark cache</a>


to keep it consistent, change the value of the menu entry active name to benchmark_cache

s3inlc · 2023-08-25T12:00:52Z

Thanks for quickly updating all the requested parts. It seems that there now are only some issues with the tests remaining.
One of the agent tests probably fails, because there was the change with the devices: https://github.com/hashtopolis/server/actions/runs/5974589110/job/16211674722?pr=955#step:6:96 and https://github.com/hashtopolis/server/actions/runs/5974589110/job/16211674722?pr=955#step:6:102

And the other one is most likely caused by a missing use statement in the BenchmarkUtils class: https://github.com/hashtopolis/server/actions/runs/5974589110/job/16211674722?pr=955#step:6:121

jessevz · 2023-08-26T11:08:48Z

I think everything works now, if there are still any improvements that needs to be made please let me know.

what's good to know is that how the attackparameters are parsed is still a bit POC-ish. For example i currently used a static map that parses a few short list parameters and turns them into the long list parameter to make sure the attackparameters are in 1 single format. Ideally we want to have a dynamic map were the cracker binary gives all the possible short and long list parameters, but I thought that would require so many adjustments to the code that it probably had to be a separate story.

s3inlc · 2023-08-26T11:45:47Z

Thanks for all the updates and addressing all the requested changes. So far it looks now good for me. I will have a final look with zyronix regarding the PR and if we don't see anything else, it should be fine to merge.

Regarding your statement about the way you implemented the parsing of the parameters. I see that the perfect way would be much more complicated with parsing the possible short/long parameter combinations, also if any other cracker comes into place. For the moment I think your solution does cover already most of the cases and we may miss just some cases where it does not use the cached value, but it should not effect functionality, just that a benchmark may be requested where it would not be needed.

zyronix · 2023-10-03T07:42:30Z

@jessevz There are some merge conflicts left before we can merge this one. Could you take a look into this one?

jessevanzutphen7@hotmail.com added 30 commits April 27, 2023 12:55

changed database structure for benchmarks. Added a Benchmark table an…

323b113

…d hardwareGroup

Added the models for benchmark caching

2f5cd16

Changed getchunk, sendbenchmark and updateclientinformation API to ma…

d5885e6

…ke use of benchmark caching

Added benchmarkUtils and HardwareGroupUtils

174e791

Changed Agent templates to use hardwareGroups

57483af

Added frontend to view the cached benchmarks

6f53747

fixed bug in benchmarkUtils

75cedf4

fixed bug in registerAgent, wasnt updated to hardwaregroup

a5cf430

updated install/hashtopolis.sql for benchmark cache

0c3f561

Added update script for database update

0c2509b

Fixed errors in hashtopolis.sql file

e06fb1f

fixed bug in hashtopolis.sql

f01d790

fixed bug in hashtopolis.sql again

3a2d0db

fixed bug in hashtopolis.sql, again

588e1ea

fixed another bug in hashtopolis.sql, again

6049f50

added hashmode and benchmarktype to Benchmark object in hashtopolis.sql

468a50b

fixed bug in hashtopolis.sql, creat table benchmark didnt work

bd7043e

updated the update script

a1e8b7b

fixed bug in hashtopolis.sql, defined hardwareGroup primary key twice

df1f34c

fixed typo in hashtopolis.sql

99d9aec

Made hardwareGroupId in agent table nullable

31991fd

updated cache code to latest version

19c9f4c

Updated benchmark.utils

8f64838

removed foreignkey from agent to hardwareGroup becasue of bug

9b23f5b

updated frontend

d173c1c

uncommented the ttl

4194533

Removed foreignkey from agent to hardwareGroup in updatescript becaus…

17c8956

…e of bug

Added crackerBinary to cached benchmarks

cf390fd

Formatted the hardware of the benchmark prettier in the frontend

579c8f9

Added the crackerbinary to the benchmark frontend

164bc95

jessevz and others added 2 commits July 25, 2023 12:54

Merge branch 'master' into master

a4ec3fc

Removed old comments

85ade75

s3inlc reviewed Aug 7, 2023

View reviewed changes

jessevanzutphen7@hotmail.com added 4 commits August 11, 2023 03:27

Updated codestyle in benchmarkUtils

d15498c

Added docStrings in benchmarkUtils

ecff047

Made the update script working

502dbe1

removed comments from install script and renamed update script to cor…

a1b04d6

…rect format

jessevanzutphen7@hotmail.com added 6 commits August 16, 2023 05:17

Made benchmarkcache ttl conifgurable by using the configs

2d13640

Added efault hardwaregroup so that foreignkey constraint wouldnt fail…

5b769de

… when creating new agent

Added default hardwaregroup so that foreignkey constraint wouldnt fai…

7b31337

…l when creating new agent

Added the option to disable caching by setting ttl to 0

864693e

Added the changes to the changelog

580a08d

cleaned up benchmark handler

3849695

s3inlc requested changes Aug 24, 2023

View reviewed changes

jessevanzutphen7@hotmail.com added 2 commits August 25, 2023 04:54

Updated code based on code review

cdcecc3

Added accidently removed code

4798414

jessevanzutphen7@hotmail.com added 3 commits August 26, 2023 04:37

Made the benchmark cache also use the agent specific CMD parameters

5cca11a

Fixed benchmarktests

5d30ad2

Fixed agent API

18dbd90

s3inlc approved these changes Aug 26, 2023

View reviewed changes

Added code to make benchmark working with new ui and API v2

dd03d7d

jessevz mentioned this pull request Aug 30, 2023

Added frontend components for benchmarkcache hashtopolis/web-ui#12

Open

zyronix changed the base branch from master to dev September 8, 2023 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Benchmark cache #955

Benchmark cache #955

jessevz commented Jul 25, 2023 •

edited

Loading

s3inlc commented Aug 7, 2023 •

edited

Loading

s3inlc Aug 7, 2023

s3inlc Aug 7, 2023

s3inlc Aug 7, 2023

s3inlc Aug 7, 2023

s3inlc Aug 7, 2023

jessevz commented Aug 11, 2023

s3inlc commented Aug 24, 2023

s3inlc left a comment

s3inlc Aug 24, 2023

s3inlc Aug 24, 2023

s3inlc Aug 24, 2023

s3inlc Aug 24, 2023

s3inlc Aug 24, 2023

s3inlc Aug 24, 2023

s3inlc Aug 24, 2023

s3inlc Aug 24, 2023

s3inlc Aug 24, 2023

s3inlc commented Aug 25, 2023

jessevz commented Aug 26, 2023

s3inlc commented Aug 26, 2023

zyronix commented Oct 3, 2023

Benchmark cache #955

Are you sure you want to change the base?

Benchmark cache #955

Conversation

jessevz commented Jul 25, 2023 • edited Loading

s3inlc commented Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessevz commented Aug 11, 2023

s3inlc commented Aug 24, 2023

s3inlc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s3inlc commented Aug 25, 2023

jessevz commented Aug 26, 2023

s3inlc commented Aug 26, 2023

zyronix commented Oct 3, 2023

jessevz commented Jul 25, 2023 •

edited

Loading

s3inlc commented Aug 7, 2023 •

edited

Loading