Feature pull client data from object store #549

hardbyte · 2020-04-24T05:20:12Z

Builds on #548

Adds the REST api changes and backend changes to pull binary encoding data from an object store.

joyceyuu

Look good to me. Just few questions

joyceyuu · 2020-04-27T05:10:08Z

azure-pipelines.yml

            api.dbinit.image.repository:$(backendImageName)
            workers.image.repository:$(backendImageName)
+            workers.image.pullPolicy:Always
+            anonlink.objectstore.secure:false


Just wondering how yaml file can recognise anonnlink here?

These overrides are translated into command line arguments to helm template much like setting individual values when deploying a chart (e.g., --set anonlink.objectstore.secure=false). The path in question anonlink.objectstore is defined in the values.yaml file.

You can see the final command here:

/opt/hostedtoolcache/helm/2.16.3/x64/linux-amd64/helm template /home/vsts/work/1/s/deployment/entity-service --name azk09d6034 --namespace test-azure --set global.postgresql.postgresqlPassword=notaproductionpassword --set api.ingress.enabled=false --set workers.replicaCount=4 --set api.www.image.repository=data61/anonlink-nginx --set api.www.image.pullPolicy=Always --set api.app.image.repository=data61/anonlink-app --set api.app.image.pullPolicy=Always --set api.dbinit.image.repository=data61/anonlink-app --set workers.image.repository=data61/anonlink-app --set workers.image.pullPolicy=Always --set anonlink.objectstore.secure=false --set anonlink.objectstore.uploadEnabled=true --set anonlink.objectstore.uploadAccessKey=testUploadAccessKey --set anonlink.objectstore.uploadSecretKey=testUploadSecretKey --set minio.accessKey=testMinioAccessKey --set minio.secretKey=testMinioSecretKey

joyceyuu · 2020-04-27T05:11:20Z

backend/entityservice/api_def/openapi.yaml

+        The caller must have both the `project_id` and a valid `upload_token` in order to contribute data,
+        both of these are generated when a project is created.
+        This endpoint can directly accept uploads up to several hundred MiB, and can pull encoding data from
+        an object store for larger uploads.


joyceyuu · 2020-04-27T05:15:43Z

backend/entityservice/api_def/openapi.yaml

+          - type: integer
+
+
+    BlockMap:


Is BlockMap compulsory? What if there are only encodings from external object. Would BlockMap become arrays of same blockid?

No it isn't required, see above where it is referenced - only the encodings are required:

EncodingUpload: description: Object that contains one data provider's encodings type: object required: [encodings] properties: encodings: ... blocks: oneOf: - $ref: '#/components/schemas/BlockMap'

What if there are only encodings from external object. Would BlockMap become arrays of same blockid?

blocks (schema/type BlockMap) would be the same regardless of whether the encodings are externally provided or directly provided. The blocks are a map from encoding id to block ids:

{ "1": ["block1", "block2"], "2": ["block2"] }

joyceyuu · 2020-04-27T06:35:22Z

deployment/entity-service/requirements.yaml

  - name: postgresql
-    version: 8.3.0
-    repository: https://kubernetes-charts.storage.googleapis.com
+    version: 8.9.1


How often would you recommend us to check the version of dependent charts? Would there be similar dependency bot for charts?

I would do it anytime you change the kubernetes deployment template. I haven't seen an automatic dependabot like thing for helm chart dependencies.

joyceyuu · 2020-04-27T06:36:17Z

deployment/entity-service/templates/NOTES.txt

 1. Get the entity service URL by running:

-{{- if contains "NodePort" .Values.api.service.type }}
+{{- if eq .Values.api.service.type "NodePort" "ClusterIP" }}


Why do we wrap code in double curly bracket? Is it some Javascript?

No it is helm template specific - a combination of the Go template language with some extra functions and helm deployment data. See the docs here

deployment/entity-service/templates/NOTES.txt

wilko77

The goal for this data pull from object store feature is that we can support population sized linkages. Without blocking, this is not really achievable. After all the hard work you put in we are so close now to achieve this. Why did you decide to not handle blocking information as well?

wilko77 · 2020-04-27T12:33:25Z

backend/entityservice/api_def/openapi.yaml

+            #- $ref: '#/ExternalData'
+
+    EncodingArray:
+      description: Array of encodings, comprising the base64-encoding (and encoding id?).


an API defines the format, not asks for clarification...

Eek, yeah I wasn't sure about taking the extra step at this point to allow the user to provide the encoding ids. I decided it wasn't part of this feature. I'll remove the question from the api spec

wilko77 · 2020-04-27T12:34:01Z

backend/entityservice/api_def/openapi.yaml

+        blocks:
+          oneOf:
+            - $ref: '#/components/schemas/BlockMap'
+            ## TODO may be useful to handle external blocking data too


definitely. With very small blocks, the blocking info is in the same order of size as the encodings.

This will be part of a follow up PR

wilko77 · 2020-04-27T13:08:34Z

backend/entityservice/encoding_storage.py

+    filename = None
+    # Set the state to 'pending' in the uploads table
+    with DBConn() as conn:
+        db.insert_encoding_metadata(conn, filename, dp_id, receipt_token, encoding_count=count, block_count=1)


what's with the filename = None business?

It is just a way of keeping the column in the uploads table set to NULL.

wilko77 · 2020-04-27T13:18:05Z

backend/entityservice/encoding_storage.py

+    # # Now work out if all parties have added their data
+    # if clks_uploaded_to_project(project_id):
+    #     logger.info("All parties data present. Scheduling any queued runs")
+    #     check_for_executable_runs.delay(project_id, serialize_span(parent_span))


where is check_for_executable_runs called in the case of external data?

thanks, that looks like an oversight on my part. Clearly prequeued runs with external data are not covered by a test :-/

hardbyte · 2020-04-27T21:38:09Z

The goal for this data pull from object store feature is that we can support population sized linkages. Without blocking, this is not really achievable. After all the hard work you put in we are so close now to achieve this. Why did you decide to not handle blocking information as well?

I hope this wasn't a surprise given it was mentioned in the design doc. I specified that only encodings would be supported in the first implementation due to the time available. I wasn't sure if we'd have to create a new binary format for blocking info and that didn't seem too related to the core functionality of "pulling data". However, I have developed a branch that supports external blocking information (see #551).

…building docker images

…codings

…pull data from object store

hardbyte · 2020-04-30T01:32:14Z

Changes incorporated in PR #551

hardbyte requested review from joyceyuu and wilko77 April 24, 2020 05:20

hardbyte marked this pull request as ready for review April 26, 2020 21:04

joyceyuu approved these changes Apr 27, 2020

View reviewed changes

wilko77 requested changes Apr 27, 2020

View reviewed changes

hardbyte and others added 11 commits April 28, 2020 17:05

Modify upload endpoint to take information on external encodings.

3dfa25d

Minor: Fix api url in e2e test helper, ignore created log files when …

80d99fa

…building docker images

Simplify abort_if_inconsistent_upload and add support for external en…

db9464f

…codings

Add utility functions to convert to common clknblocks format.

9d18ec4

Catch most common error in status check during startup

8ccd27b

Modify upload endpoint to take external data, add background task to …

a6d3715

…pull data from object store

Use correct endpoint for testing object store uploads

3dbb49b

Explicitly throw a notimplemented error for blocks + external encodings

b54423f

Improve open api spec for encoding upload

1d27bbb

Check for executable runs after handling data upload

f9cc8c3

Fix bug where a null receipt token was entered into db

54eb646

hardbyte force-pushed the feature-data-pull-endpoint branch from 85e1543 to 54eb646 Compare April 28, 2020 05:06

hardbyte requested a review from wilko77 April 28, 2020 23:55

wilko77 approved these changes Apr 30, 2020

View reviewed changes

hardbyte closed this Apr 30, 2020

hardbyte deleted the feature-data-pull-endpoint branch April 30, 2020 01:32

Feature pull client data from object store #549

Feature pull client data from object store #549

Uh oh!

Conversation

hardbyte commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyceyuu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hardbyte Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hardbyte Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wilko77 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hardbyte commented Apr 27, 2020

Uh oh!

hardbyte commented Apr 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hardbyte commented Apr 24, 2020 •

edited

Loading

hardbyte Apr 28, 2020 •

edited

Loading

hardbyte Apr 28, 2020 •

edited

Loading