Add jobs for database migration and auto-registration #294

peterbroadhurst · 2021-10-26T18:26:23Z

Adds a couple of simple jobs, with configuration options to enable them:

config.postgresMigrationJob: true
- Creates a k8s Job to perform the migrations, which runs on each new FireFly tag deployed
config.registrationJob: true
- Creates a k8s Job to ensure registration is performed, which runs once (unless the org name changes)

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

nguyer

I can't speak for the actual kubernetes things going on here, but everything going on inside it in terms of migrations and registration looks good to me.

onelapahead · 2021-10-26T18:43:34Z

deploy/charts/firefly/templates/core/migration-job.yaml

+        - sh
+        - -ce
+        - |
+          # Install deps


You could make this a proper script in a scripts folder and then template it into this job like so (or something along these lines):

{{ (.Files.Glob "scripts/ff-db-migrations.sh") | indent 2 }}

that way when folks are editing this script they can have their editor help them w/ the shell syntax rather than dealing w/ writing shell w/in yaml

Looks great!

Took a few extra changes to get this right @hfuss - but it's there now.
Asked for a re-review if you have a min to double check.

I also found through testing that we should handle the situation where the org is registered, but the node isn't.

The common case is as follows:

You run the job, while the events are syncing with the chain

The job times and out and re-runs

Eventually all is caught up - the original Org registration works

The script ties to re-register the org, and fails - which it doesn't need to do

You should see this handled in the script now.

onelapahead

lgtm, was going to say we could use the go-migrate image but I see we're taking advantage of the fact that the sql migrations are bundled into the core FF container 👍🏻

Perhaps we could make a firefly-utility image on top of the core image that has postgres-client, jq, and go-migrate preinstalled.

peterbroadhurst · 2021-10-26T18:46:12Z

Perhaps we could make a firefly-utility image on top of the core image that has postgres-client, jq, and go-migrate preinstalled.

Think this is a great suggestion, although I might need to leave the next step on that one with you or @nguyer

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

codecov-commenter · 2021-10-26T19:04:09Z

Codecov Report

Merging #294 (283b4a2) into main (ce74fe3) will not change coverage.
The diff coverage is n/a.

❗ Current head 283b4a2 differs from pull request most recent head ebc9922. Consider uploading reports for the commit ebc9922 to get more accurate results

@@            Coverage Diff            @@
##              main      #294   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          224       224           
  Lines        12387     12387           
=========================================
  Hits         12387     12387

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce74fe3...ebc9922. Read the comment docs.

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

onelapahead

Only asking for removing the unnecessary env var and renaming the container in the registration job manifest, everything else is speculation or a suggestion.

onelapahead · 2021-10-27T02:51:09Z

deploy/charts/firefly/scripts/ff-db-migrations.sh

+PSQL_URL_NO_DB=`echo ${PSQL_URL} | sed "s/\/${DB_NAME}//"`
+
+# Check we can connect to the PSQL Server
+while ! psql -c "SELECT 1;" ${PSQL_URL_NO_DB}; do


Could do until rather than while ! to avoid the negation logic. It's six or half a dozen but I always prefer to avoid negations for readability.

Unless there's no until in sh and its bashism 🤷🏻‍♂️

confirmed until is in BusyBox

onelapahead · 2021-10-27T02:52:19Z

deploy/charts/firefly/templates/_helpers.tpl

 org:
  name: {{ .Values.config.organizationName }}
-  identity: {{ .Values.config.organizationIdentity }}
+  key: {{ .Values.config.organizationIdentity }}


Should we rename the value to match? i.e. organizationKey ?

In eth this is an address right? But for fabric its something else... a channel? And I guess the rename was to avoid confusion w/ the new identity plugin?

onelapahead · 2021-10-27T02:53:09Z

deploy/charts/firefly/templates/core/registration-job.yaml

+        env:
+        - name: FF_URL
+          value: "http://{{ include "firefly.fullname" . }}:{{ .Values.core.service.httpPort }}"
+        - name: PSQL_URL


Don't believe this one is needed for this job / script

onelapahead · 2021-10-27T02:53:54Z

deploy/charts/firefly/templates/core/registration-job.yaml

+  template:
+    spec:
+      containers:
+      - name: migration


Suggested change

- name: migration

- name: registration

onelapahead · 2021-10-27T02:57:18Z

deploy/charts/firefly/scripts/ff-registration.sh

+  echo "Registering organization"
+  HTTP_CODE=`curl --silent --output /dev/stderr --write-out "%{http_code}" \
+    -X POST -d '{}' -H 'Content-Type: application/json' \
+    "${FF_URL}/api/v1/network/organizations/self?confirm"`


Is this API synchronous now?

I remember when we wrote the original internal registration job, mimicking what was in ff-cli, we'd poll after org registration to ensure that the org had be published before going onto the node registration.

I think this was answered by #294 (comment) but could use confirmation from @peterbroadhurst

Is this API synchronous now?

Yep, with the confirm option

onelapahead · 2021-10-27T02:59:06Z

deploy/charts/firefly/scripts/ff-db-migrations.sh

@@ -0,0 +1,40 @@
+# Install deps


Clearly doesn't affect the actual job from running but to make the script executable locally would ask the shebang get added.

onelapahead · 2021-10-27T02:59:51Z

deploy/charts/firefly/scripts/ff-registration.sh

+
+apk add curl jq
+
+while ! STATUS=$(curl ${FF_URL}/api/v1/status); do


Same here for until if you care

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

nguyer · 2021-10-28T14:12:38Z

Perhaps we could make a firefly-utility image on top of the core image that has postgres-client, jq, and go-migrate preinstalled.

Think this is a great suggestion, although I might need to leave the next step on that one with you or @nguyer

Sounds like a good idea. Scope for a different PR though?

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

peterbroadhurst · 2021-10-29T01:11:13Z

Great review comments, thanks @hfuss
All addressed now.

onelapahead

thanks for addressing all the particulars and answering my questions!

peterbroadhurst added 2 commits October 26, 2021 14:21

Add jobs for database migration and auto-registration

6a48497

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

Remove DB hardcode

fd73f3b

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

peterbroadhurst requested review from awrichar and nguyer as code owners October 26, 2021 18:26

peterbroadhurst added 2 commits October 26, 2021 14:32

Add note on registration

eac309d

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

Job only re-runs on org name change

9d80743

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

nguyer approved these changes Oct 26, 2021

View reviewed changes

onelapahead reviewed Oct 26, 2021

View reviewed changes

onelapahead approved these changes Oct 26, 2021

View reviewed changes

Use scripts

b9bded1

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

peterbroadhurst added 2 commits October 26, 2021 17:39

Use Get for simplicity

c2b0816

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

Move to env vars into scripts not templating

84922c9

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

peterbroadhurst requested a review from onelapahead October 26, 2021 21:44

peterbroadhurst added 2 commits October 26, 2021 17:48

Move to non-deprecated field

0977afd

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

Check org registration separate to node registration

1368d75

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

onelapahead suggested changes Oct 27, 2021

View reviewed changes

Fix check for status

40ba6a9

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

peterbroadhurst added 2 commits October 28, 2021 18:15

Merge branch 'main' of github.com:hyperledger/firefly into helm-jobs

0e29d1a

Close out review comments

ebc9922

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>

onelapahead approved these changes Oct 29, 2021

View reviewed changes

peterbroadhurst merged commit 638711f into hyperledger:main Oct 29, 2021


		apk add curl jq

		while ! STATUS=$(curl ${FF_URL}/api/v1/status); do

Add jobs for database migration and auto-registration #294

Add jobs for database migration and auto-registration #294

Uh oh!

Conversation

peterbroadhurst commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nguyer 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!

peterbroadhurst Oct 26, 2021 • 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!

onelapahead left a comment

Choose a reason for hiding this comment

Uh oh!

peterbroadhurst commented Oct 26, 2021

Uh oh!

codecov-commenter commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

onelapahead left a comment • 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!

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!

nguyer commented Oct 28, 2021

Uh oh!

peterbroadhurst commented Oct 29, 2021

Uh oh!

onelapahead left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

peterbroadhurst commented Oct 26, 2021 •

edited

Loading

peterbroadhurst Oct 26, 2021 •

edited

Loading

codecov-commenter commented Oct 26, 2021 •

edited

Loading

onelapahead left a comment •

edited

Loading