Store deployments in a common bucket #1424

amarthadan · 2022-09-07T09:32:36Z

So sorry about the massive commit 😞 I was changing stuff all over the place and didn't really split it into some meaningful pieces.

I would suggest reviewing the code in airnode-deployer/src/infrastructure as a whole, not looking at the diff, as those files were pretty much rewritten.

I'm still missing tests (and the current ones are broken) but I wanted to open a draft PR for review while I finish them.

martinkolenic · 2022-09-08T11:40:41Z

Hi @amarthadan, I've run multiple deployments with different stages both on GCP and AWS and it seems the changes are working fine. I was able to complete a request and the shared bucket now replaces the set of buckets we had previously.

amarthadan · 2022-09-08T11:48:37Z

Hi @amarthadan, I've run multiple deployments with different stages both on GCP and AWS and it seems the changes are working fine. I was able to complete a request and the shared bucket now replaces the set of buckets we had previously.

Just to be clear, you tried multiple deployments at the same time, right? Have you also tried multiple deployments with a different Airnode address? That's important as well.
Also, you now shouldn't be able to redeploy if you change the region in the configuration file. Please, test that as well.

martinkolenic · 2022-09-08T11:55:06Z

Hi @amarthadan, I've run multiple deployments with different stages both on GCP and AWS and it seems the changes are working fine. I was able to complete a request and the shared bucket now replaces the set of buckets we had previously.

Just to be clear, you tried multiple deployments at the same time, right?

If running the deploy commands shortly one after another counts, then yes.

Have you also tried multiple deployments with a different Airnode address? That's important as well.

No, I haven't - will do.

Also, you now shouldn't be able to redeploy if you change the region in the configuration file. Please, test that as well.

Will do.

amarthadan · 2022-09-08T12:33:05Z

If running the deploy commands shortly one after another counts, then yes.

No, I mean having actually 2 or more Airnodes deployed (with different stage names) at the same time, not being deployed at the same time. So they are running and have resources in the cloud at the same time. That's the main thing.

Siegrift

Some minor comments, but I've only did a shallow review. Will try the feature properly later.

packages/airnode-deployer/src/infrastructure/aws.ts

packages/airnode-validator/src/config/config.ts

Siegrift · 2022-09-09T05:49:40Z

packages/airnode-deployer/src/utils/files.ts

      cloudProvider: config.nodeSettings.cloudProvider as CloudProvider,
      stage: config.nodeSettings.stage,
      nodeVersion: config.nodeSettings.nodeVersion,
-      timestamp,
+      timestamp: new Date().toISOString(),


Mhhh, this will make the timestamp on cloud provider different to what is in the receipt. Shouldn't those two be the same?

I was thinking about that and didn't know what approach would be better. These two can vary by up to 15 min (GCP so slooow). I think the timestamp in the receipt should be of the time the deployment ended. But I guess that's up to interpretation. It can be the time it started 🤷‍♂️ But that's odd, isn't it?

Won't users identify a particular deployment by that timestamp?

You mean in the cloud? I don't think they will. First of all, they should not need to deal with the cloud resources manually. And second, the deployment is distinguished by Airnode address and stage. And you're currently deployed version is the one with the latest timestamp, no matter what's in your receipt. But I get what you're saying, I can sync those two timestamps, I don't have a strong preference.

packages/airnode-deployer/src/utils/infrastructure.ts

packages/airnode-deployer/src/infrastructure/aws.ts

packages/airnode-deployer/src/infrastructure/gcp.ts

aquarat

I find this change exciting and it's a lot of work. A very interesting read 🔥.
I enjoyed the path traversal reduce and comments around it.
I don't see any major issues, just questions and perhaps enhancements that may save us pain in future.

packages/airnode-deployer/src/cli/commands.ts

packages/airnode-deployer/src/infrastructure/index.ts

packages/airnode-deployer/src/utils/infrastructure.ts

packages/airnode-deployer/terraform/gcp/main.tf

packages/airnode-deployer/test/fixtures/index.ts

Siegrift

I encountered an issue when running this which @amarthadan reproduced, will take another look when he debugs the issue.

martinkolenic · 2022-09-12T12:00:52Z

Hi @amarthadan and @Siegrift, it seems that despite my best effort I did something wrong during testing and I cannot reproduce the functioning results now. I'm currently investigating it.

amarthadan · 2022-09-12T12:42:33Z

@Siegrift It should be fixed now.

martinkolenic · 2022-09-12T12:55:11Z

@Siegrift It should be fixed now.

I managed to deploy and send a request now, it seems to be working correctly now. The fact that I did not catch the issue might be because of a human error on my side, launching something different.

Siegrift

TLDR:

All basic ops seems to be working, but there were some unexpected failures along the way. I wrote notes for the full journey, maybe it helps... Will take a look at the code once more tomorrow.

Notes

Deployed Airnode with both gateways
- success
I forgot to specify HTTP trigger, so I redeployed with modified config (same stage)
- failed (because it was already there) and autoremove removed the original Airnode
- this is an awkward side effect of autoremove, but it’s the technically correct thing
- BUG: This should not have happened (but later this exact thing works again)

Deployed again
- success, both gateways work and RRP request works
Changed stage, deployed again
- failed, because gateway keys already exist, but autoremove was sucessfull
- the error is kinda annoying, but we I don't think we can do anything about it
- previous airnode works
Changed secrets and redeployed
- failed, got message “Specified ReservedConcurrentExecutions for function decreases account's UnreservedConcurrentExecution below its minimum value of [100].”
- The error is correct. The account unreserved concurency is 189, the example reserves 100 for RRP and 20 per gateway = 140
- TODO(new task): Decrease examples reserved concurrency to 1
Changed “maxConcurrency” to 1
- success, gateways + RRP works, aws folder structure is correct
- I actually also changed the mnemonic so it deployed a completely different Airnode

Created new secrets again and changed stage, but preserved mnemonic, deployed again
- success, everything works

Changed mnemonic to the old Airnode, but preserved the same stage (and forgot to update gateway keys)
- wanted to stop deployment by quickly pressing ctrl+z, deployer got stuck so I killed it the hard way (stopped the docker)
- TODO(task): The docker (or the spinner) should not get frozen when pressing ctrl+z
- the original airnode is still operational (both gateways, didn’t try RRP)

Created new gateway keys, trying to redeploy
- failed, error: “Failed to copy file '0x5691ceD2b03593a8d67BdfF4Fe96107bfe603825/dev3/1663007611553/default.tfstate' to file '0x5691ceD2b03593a8d67BdfF4Fe96107bfe603825/dev3/1663007887640/default.tfstate' within S3 bucket 'airnode-6d46164e9ddd': NoSuchKey: The specified key does not exist.”
- No idea what it means, autoremove worked
Trying again the same thing
- both gateways work, aws structure is correct
removing the last airnode (and lost the receipts for others)
- success
deploying again
- success
deploying again (to verify re-deployment bug)
- this time success

trying to deploy again 2x
- success, success
change region, deploy
- deploy fail, autoremove fail (which is actually a success)

changed config.json by disabling http gateway, deploy
- deploy fail, autoremove success (so it removed the airnode)

changed the config back, successfully redeployed, changed the config to disable one of the gateways, deployed again
- worked

packages/airnode-deployer/src/utils/infrastructure.ts

amarthadan · 2022-09-13T06:38:43Z

@Siegrift Thanks for the thorough test.

As far as I can tell these are the issues that need to be investigated/fixed:

The docker (or the spinner) should not get frozen when pressing ctrl+c
Failed to copy file issue - This most likely happened because you forcefully stopped the deployment. But I agree that we can prevent at least some cases.
changed config.json by disabling http gateway, deploy -> deploy fail - I have no idea what this is, will investigate.

As for the topmost issue (screenshot), I don't think that happened because the Airnode was already there. From the screenshot, it looks like a network error. There's not much we can do about that. Or are they two different issues?

If you're ok with it I would address these in separate PRs. Also, do these need to be fixed for 0.9 to be released? That's more of a question for @aquarat I guess. I would like to address them but if we're waiting for 0.9 they are not critical and can be fixed later.

Siegrift · 2022-09-13T07:00:09Z

If you're ok with it I would address these in separate PRs.

Yeah. Btw. as you can see I only tested the AWS part. Leaving the GCP testing to @martinkolenic .
I'll take a one more look at code and give the final review...

Siegrift

Nice work. I've found a few comments and typos and I am not sure about updating using a different nodeVersion...

Siegrift · 2022-09-13T07:01:37Z

packages/airnode-deployer/package.json

@@ -15,7 +15,7 @@
    "clean": "rimraf -rf *.tsbuildinfo ./dist ./.webpack ./build *.tgz",
    "cli": "ts-node bin/deployer.ts",
    "compile": "tsc --build tsconfig.json",
-    "copy:terraform": "copyfiles terraform/**/**/**/*.tf terraform/**/**/**/*.tpl  dist/",
+    "copy:terraform": "copyfiles terraform/**/**/*.tf terraform/**/**/*.tpl  dist/",


Mhh, is the pattern "/**/" (x2) necessary? The ** stands for any nested directory so / feels redundant.

You would think so but it doesn't work that way for copyfiles. I can doublecheck but I'm pretty sure I had to go as deep as the directory structure was in order to not miss any files.

Yes, it needs to be like this 🤷‍♂️

packages/airnode-deployer/src/infrastructure/aws.test.ts

packages/airnode-deployer/src/infrastructure/index.ts

amarthadan · 2022-09-13T09:30:49Z

Hi @amarthadan, with the newest commit fa7340ca, running yarn build in the root directory of the repo results in a failure to build airnode-deployer, see screenshot.

That was just a temporary state as I accepted the suggested changes. Should be fixed now.

Siegrift

👍 LGTM, but haven't tested the GCP side.

btw. thx for not squashing the last commits.

martinkolenic · 2022-09-16T12:21:10Z

Hi, @amarthadan, my test results (GCP):

deploy Airnode with both gateways enabled - PASS
redeploy with a changed stage (and new API keys) - PASS
redeploy with a changed mnemonic (same stage) - FAIL
- just like @Siegrift, I forgot to change the API keys so I tried to stop the deployment with Ctrl+Z, which did not work (the spinner kept spinning though) and so I killed the process after about 34 seconds.
- I hit the quota for regional per-project gateways, screenshot below
- Autoremove succeeded though; only the latest Airnode things were removed

The folder structure was correct, though, all Airnodes were kept in the same bucket and all the functions in another bucket. That error I encountered was only due to a quota set on GCP's side so that's not us.

amarthadan requested review from aquarat, Siegrift and a team September 7, 2022 09:32

amarthadan self-assigned this Sep 7, 2022

Siegrift mentioned this pull request Sep 8, 2022

Release v0.8.0 #1428

Merged

Siegrift requested changes Sep 9, 2022

View reviewed changes

amarthadan force-pushed the deployments-common-bucket branch 4 times, most recently from 62cbf7c to e8276b8 Compare September 12, 2022 07:58

amarthadan marked this pull request as ready for review September 12, 2022 07:58

amarthadan requested a review from Siegrift September 12, 2022 08:02

amarthadan force-pushed the deployments-common-bucket branch 2 times, most recently from 5b587bd to 697d24f Compare September 12, 2022 09:31

aquarat approved these changes Sep 12, 2022

View reviewed changes

Siegrift requested changes Sep 12, 2022

View reviewed changes

amarthadan force-pushed the deployments-common-bucket branch from f1fc7d3 to e82a04a Compare September 12, 2022 12:12

Siegrift reviewed Sep 12, 2022

View reviewed changes

packages/airnode-deployer/src/utils/infrastructure.ts Show resolved Hide resolved

amarthadan added 2 commits September 13, 2022 08:42

Store deployments in a common bucket

4b3db2b

Add validator tests for mnemonic and max concurrency

a6cdf2e

amarthadan force-pushed the deployments-common-bucket branch from e82a04a to da28176 Compare September 13, 2022 06:44

amarthadan force-pushed the deployments-common-bucket branch from da28176 to c82a68b Compare September 13, 2022 07:33

Siegrift requested changes Sep 13, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

amarthadan requested a review from Siegrift September 13, 2022 09:31

Siegrift approved these changes Sep 13, 2022

View reviewed changes

amarthadan added 3 commits September 13, 2022 13:17

Add tests for Airnode deployment, AWS and GCP wrappers

a9dbf80

Change timeout for gateway serverless functions to 30s

a07048e

Add cloud provider-specific comments for bucket creations

4c7b013

amarthadan force-pushed the deployments-common-bucket branch from bb4c8fd to 4c7b013 Compare September 13, 2022 11:17

amarthadan merged commit 5f8f7d4 into master Sep 13, 2022

amarthadan deleted the deployments-common-bucket branch September 13, 2022 11:51

aquarat mentioned this pull request Sep 13, 2022

HTTP gateway times out at cold starts #1386

Open

This was referenced Sep 14, 2022

Gracefully handle missing files of the deployed Airnode #1449

Closed

Fix failing redeployment when the HTTP gatewayes become disabled #1450

Closed

Siegrift mentioned this pull request Sep 14, 2022

Disable concurrency in airnode-examples #1451

Closed

This comment was marked as resolved.

Sign in to view

bbenligiray mentioned this pull request Sep 15, 2022

Parse GCP project ID from gcp.json instead of config.json #1317

Open

This comment was marked as resolved.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store deployments in a common bucket #1424

Store deployments in a common bucket #1424

amarthadan commented Sep 7, 2022

martinkolenic commented Sep 8, 2022 •

edited

Loading

amarthadan commented Sep 8, 2022

martinkolenic commented Sep 8, 2022 •

edited

Loading

amarthadan commented Sep 8, 2022

Siegrift left a comment

Siegrift Sep 9, 2022

amarthadan Sep 9, 2022

Siegrift Sep 9, 2022

amarthadan Sep 9, 2022

aquarat left a comment

Siegrift left a comment

martinkolenic commented Sep 12, 2022

amarthadan commented Sep 12, 2022

martinkolenic commented Sep 12, 2022 •

edited

Loading

Siegrift left a comment

amarthadan commented Sep 13, 2022

Siegrift commented Sep 13, 2022

Siegrift left a comment

Siegrift Sep 13, 2022

amarthadan Sep 13, 2022

amarthadan Sep 13, 2022

This comment was marked as resolved.

amarthadan commented Sep 13, 2022

Siegrift left a comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

martinkolenic commented Sep 16, 2022

Store deployments in a common bucket #1424

Store deployments in a common bucket #1424

Conversation

amarthadan commented Sep 7, 2022

martinkolenic commented Sep 8, 2022 • edited Loading

amarthadan commented Sep 8, 2022

martinkolenic commented Sep 8, 2022 • edited Loading

amarthadan commented Sep 8, 2022

Siegrift left a comment

Choose a reason for hiding this comment

Siegrift Sep 9, 2022

Choose a reason for hiding this comment

amarthadan Sep 9, 2022

Choose a reason for hiding this comment

Siegrift Sep 9, 2022

Choose a reason for hiding this comment

amarthadan Sep 9, 2022

Choose a reason for hiding this comment

aquarat left a comment

Choose a reason for hiding this comment

Siegrift left a comment

Choose a reason for hiding this comment

martinkolenic commented Sep 12, 2022

amarthadan commented Sep 12, 2022

martinkolenic commented Sep 12, 2022 • edited Loading

Siegrift left a comment

Choose a reason for hiding this comment

TLDR:

Notes

amarthadan commented Sep 13, 2022

Siegrift commented Sep 13, 2022

Siegrift left a comment

Choose a reason for hiding this comment

Siegrift Sep 13, 2022

Choose a reason for hiding this comment

amarthadan Sep 13, 2022

Choose a reason for hiding this comment

amarthadan Sep 13, 2022

Choose a reason for hiding this comment

This comment was marked as resolved.

amarthadan commented Sep 13, 2022

Siegrift left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

martinkolenic commented Sep 16, 2022

martinkolenic commented Sep 8, 2022 •

edited

Loading

martinkolenic commented Sep 8, 2022 •

edited

Loading

martinkolenic commented Sep 12, 2022 •

edited

Loading