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

Aligning Workers Integration with EFS and DB #263

Merged
merged 1 commit into from
Oct 26, 2021

Conversation

CMCDragonkai
Copy link
Member

@CMCDragonkai CMCDragonkai commented Oct 25, 2021

Description

The integration of js-workers wasn't entirely clean.

The integration style should be following how EFS is integration js-workers.

So there's a bunch of things to remove.

Prior details: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205/#note_712975388

Tasks

  1. - Remove src/workers/WorkerManager.test.ts, src/workers/errors.ts and tests/workers/WorkerManager.test.ts
  2. - Edit src/workers/polykeyWorkerModule.ts to remove encryptWithKey and decryptWithKey
  3. [ ] - ArrayBuffer refactoring for src/workers/polykeyWorkerModule.ts - held off after overall CLI/API review
  4. - Ensure that workerManager can be dynamically set for VaultManager similar to KeyManager
  5. - Change tests/polykeyWorkerModule.test.ts to tests/polykeyWorker.test.ts and update all test structure to match EFS
  6. - PolykeyAgent and KeyManager.test.ts requires updates to how they construct the PolykeyWorkerManagerInterface, this can be turned into a utility function if necessary for the default worker

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@joshuakarp
Copy link
Contributor

As per https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205#note_712994114 and https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205#note_711077414, I'm assuming that the integration of the workerManager into KeyManager (and therefore the EFS) will be handled here.

@CMCDragonkai
Copy link
Member Author

Should centralise all WorkerManager injected tests, all other tests should use no worker manager, explicit null.

Except process tests, but that's a different beast to be tackled in #264.

@CMCDragonkai
Copy link
Member Author

@tegefaulkes please update this issue task list as you go along.

@tegefaulkes
Copy link
Contributor

Do we want the workerManager type to be PolykeyWorkerManagerInterface or should we rename it to WorkerManager?

@CMCDragonkai
Copy link
Member Author

The src/workers/utils.ts createWorkerManager needs to be asynchronous.

The createWorkerManager call is asynchronous. So the full signature is:

async function createWorkerManager({
  cores,
  logger,
}: {
  cores?: number;
  logger?: Logger;
}): Promise<PolykeyWorkerManagerInterface> {
  return await WorkerManager.createWorkerManager<PolykeyWorkerModule>({
    workerFactory: () => spawn(new Worker('./polykeyWorker')),
    cores,
    logger,
  });
}

@CMCDragonkai
Copy link
Member Author

Since the utility function is there, I'm going to move the types in there too from src/types.ts.

@CMCDragonkai
Copy link
Member Author

@tegefaulkes in the future, beware to use import type for things that are types.

@CMCDragonkai
Copy link
Member Author

The workerManager should not be needed by the notifications domain. I think that was vestigial code left in @emmacasolin.

Only 3 places are using worker manager: keys and vaults and the DB imported by js-db.

@CMCDragonkai
Copy link
Member Author

The imports in PolykeyAgent are quite crazy. I'd prefer these to be imported as like workersUtils and networkUtils. That will make it clearer later, but I'll leave that to later piece of work.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Oct 26, 2021

Will leave this ArrayBuffer refactoring for src/workers/polykeyWorkerModule.ts till later as part of the CLI/API review #249 and #268 as those functions may or may not be used, and we may have some general crypto refactoring in #270.

@CMCDragonkai
Copy link
Member Author

@tegefaulkes in the future try to use qualified names:

import * as workersUtils from '@/workers/utils';

When importing utilities from a different domain boundary it's better to always use the qualified import because it will allow the code to be understood when posted as snippets.

@CMCDragonkai
Copy link
Member Author

Noticed no integration tests of using VaultManager with WorkerManager together atm, this is because there wasn't even the ability to setWorkerManager and unsetWorkerManager until now. Will push this to another issue.

Added `setWorkerManager`, `unsetWorkerManager` methods for `VaultManager`
Created `createWorkerManager` utility for creating `PolykeyWorkerManagerInterface`
@CMCDragonkai
Copy link
Member Author

Some full test failures:

 FAIL  tests/nodes/NodeGraph.test.ts (5.352 s)
  NodeGraph
    ✓ finds correct node address (12 ms)
    ✓ unable to find node address (6 ms)
    ✓ adds a single node into a bucket (5 ms)
    ✕ adds multiple nodes into the same bucket (21 ms)
    ✓ adds a single node into different buckets (8 ms)
    ✓ deletes a single node (and removes bucket) (7 ms)
    ✕ deletes a single node (and retains remainder of bucket) (20 ms)
    ✓ enforces k-bucket size, removing least active node when a new node is discovered (91 ms)
    ✓ retrieves all buckets (in expected lexicographic order) (16 ms)
    ✓ refreshes buckets (1710 ms)
    ✓ finds a single closest node (4 ms)
    ✓ finds 3 closest nodes (10 ms)
    ✓ finds the 20 closest nodes (98 ms)
    ✓ updates node (5 ms)

  ● NodeGraph › adds multiple nodes into the same bucket

    expect(received).toEqual(expected) // deep equality

    Expected: {"address": {"ip": "6.6.6.6", "port": 6666}, "lastUpdated": Any<Date>}
    Received: undefined

      222 |         lastUpdated: expect.any(Date),
      223 |       });
    > 224 |       expect(bucket[newNode3Id]).toEqual({
          |                                  ^
      225 |         address: { ip: '6.6.6.6', port: 6666 },
      226 |         lastUpdated: expect.any(Date),
      227 |       });

      at Object.<anonymous> (tests/nodes/NodeGraph.test.ts:224:34)

  ● NodeGraph › deletes a single node (and retains remainder of bucket)

    expect(received).toEqual(expected) // deep equality

    Expected: {"address": {"ip": "6.6.6.6", "port": 6666}, "lastUpdated": Any<Date>}
    Received: undefined

      305 |         lastUpdated: expect.any(Date),
      306 |       });
    > 307 |       expect(bucket[newNode3Id]).toEqual({
          |                                  ^
      308 |         address: { ip: '6.6.6.6', port: 6666 },
      309 |         lastUpdated: expect.any(Date),
      310 |       });

      at Object.<anonymous> (tests/nodes/NodeGraph.test.ts:307:34)

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Oct 26, 2021

The test failure in tests/nodes/NodeGraph.test.ts is a regression in this branch. It is passing in master when ran with npm test -- tests/nodes, but in this branch it fails. The nodes domain doesn't have any usage of WorkerManager directly.

Running directly npm test -- tests/nodes/NodeGraph.test.ts does work however.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Oct 26, 2021

@joshuakarp I noticed there's a FIXME that says that the tests fail randomly. Is this related to this problem? And is there an issue for resolving these random failures?

// FIXME, some of these tests fail randomly.
describe('NodeGraph', () => {

@CMCDragonkai
Copy link
Member Author

The test failures don't seem to be related to this branch's changes. Using npm test -- tests/nodes --runInBand works. These tests are concerning given that they take humongous time relative to other tests and they have random failures. I've commented about this in #264 to be resolved afterwards.

@CMCDragonkai CMCDragonkai merged commit 5633724 into master Oct 26, 2021
@CMCDragonkai CMCDragonkai deleted the fixingworkersintegration branch October 26, 2021 10:44
@joshuakarp
Copy link
Contributor

That FIXME tag was added by @tegefaulkes, and it was in the same commit as the async-init patterns being integrated into js-polykey (a month ago). What was the the original reason for it @tegefaulkes?

This might be related to https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205#note_695753874. I have some nodes test utils that generate node IDs for specific bucket indexes, and they may be causing random test failures (where the util function isn't creating a node ID for the expected bucket). Will create an issue for it.

@tegefaulkes
Copy link
Contributor

I think that's what it was related to. there are quite a few TODOs and FIXMEs still in the code that should be reviewed and likely removed in most cases. I can make an issue for it and get around to it.

@joshuakarp
Copy link
Contributor

All good - already made an issue for this specific NodeGraph test failure: #274

Re the TODOs and FIXMEs, there's another comment here regarding this in case you haven't seen it @tegefaulkes https://gitlab.com/MatrixAI/Employees/matrix-team/-/issues/8#note_714211497

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.

3 participants