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

Testnet/Mainnet Deployments Should Contain Version Metadata derived from Polykey-CLI #93

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

amydevs
Copy link
Contributor

@amydevs amydevs commented Jan 11, 2024

Description

This PR aims to add commitHash version metadata for the sake of deployment tracking to PK-CLI deployments.

To do this, Polykey-Network-Status has to poll ECR for new image versions, knowing that the deployment has started when a new image has been detected, and has finished once a reconnection has been detected on the WebSocket connections to Polykey Nodes. This has a slight downside, being that you assume that an ECR image push will always be a deployment, and that an ECR image push will start around the same time as the ECS service deployment.

Untitled-2023-10-23-0424 excalidraw(12)

This option entails defining the commitHash at build time in scripts/build.js:
https://medium.com/@RobertoSilvaZ/adding-environments-vars-to-esbuild-b2a8e11b601e

However, the PK library, which handles the calls to agentStatus, reporting the version of the node, has no way of knowing what the commitHash of whatever is using it is (PK-CLI). Therefore, PK must expose a way for users of the library to define custom rpcMiddlewares to modify the response of agentStatus to include the version.

To get this hash, COMMIT_HASH should be defined as the output of git rev-parse HEAD using cross-env. This needs to be done in both the build script, but all those that can potentially be used to run polykey in local development (test, polykey, ts-node, etc).

The commit hash needs to also be defined as metadata on the image.
This should be done by passing it via --argstr on nix-build.
The labels should be then defined within the config of buildImage:
NixOS/nixpkgs#20852

The labels can be gotten from ECR by obtaining the digest. This involves multiple API calls:
aws/containers-roadmap#178 (comment)

Once polling ECR has returned a new commitHash not listed in our PostgresDB database, we should add information about it into the db. This might include deployment commencement time, etc. A successful deployment will cause PolykeyClients connected to previous deployments on Polykey Network Status to be disconnected. So when we detect that a client has been detected, we want to reconnect to the new deployment, and check for the commitHash returned by an RPC call that contains it. If it matches an deployment entry in our DB created earlier from polling ECR, we can append the deployment finish time to it.

Issues Fixed

Tasks

  • 1. Attach labels to Docker images for deployment to ECR
    These tasks are to be moved to a separate PR in PKNS
- [ ] 2. Write JS code to utilise AWS sdk to get label information from ECR images.
- [ ] 3. Poll ECR (or CI/CD) for new image versions, assuming that each will trigger a new deployment.
- [ ] 4. Store deployments in the database.
- [ ] 5. When Polykey nodes reconnect to the PKNS, we check if the version has updated, and if so, match it to an existing row in the database that we created earlier from polling ECR to update.

Final checklist

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

@ghost
Copy link

ghost commented Jan 11, 2024

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@amydevs
Copy link
Contributor Author

amydevs commented Jan 11, 2024

Where We're At

  • The process.env.COMMIT_HASH variable is now passed to the PolykeyAgent constructor, letting it show it on the agentStatus RPC call.
  • polykey agent status shows the commit hash
  • npm run build will freeze the process.env.COMMIT_HASH variable, and derive from either the COMMIT_HASH env var, or git get-rev HEAD
  • nix-build will derive the correct commit hash if not provided as a argstr
  • CI/CD will pass commitHash argstr to nix-build derived from $CI_COMMIT_SHA.
  • A label will be attached to the produced docker image commitHash, to be compared to that on the agentStatus RPC calls to determine deployment status.

@CMCDragonkai
Copy link
Member

I thought having a generic metadata for PolykeyAgent would have been fine.

And aren't we doing source substitution, not environment variables?

@CMCDragonkai
Copy link
Member

Why not just a generic metadata for the PolykeyAgent that the Polykey-CLI can fill in. Let's just make it possible for new kinds of data to be added to the Polykey core library that only the CLI will have... it makes sense, given that agent status command is asking about the status of Polykey agent, and in some cases, some information will only be available to the PK CLI.

The other option is to enable some form modular extensibility as we mentioned earlier - a sort of fixed point override to our RPC handlers, since separation of responsibility right now means PK CLI has no way of adding new methods to the RPC that is being exported from the Polykey core library.

There was some discussion about this earlier when @tegefaulkes asked about the tradeoffs puttin the RPC definitions into the Polykey Core library.

@amydevs
Copy link
Contributor Author

amydevs commented Jan 16, 2024

Why not just a generic metadata for the PolykeyAgent that the Polykey-CLI can fill in. Let's just make it possible for new kinds of data to be added to the Polykey core library that only the CLI will have... it makes sense, given that agent status command is asking about the status of Polykey agent, and in some cases, some information will only be available to the PK CLI.

The other option is to enable some form modular extensibility as we mentioned earlier - a sort of fixed point override to our RPC handlers, since separation of responsibility right now means PK CLI has no way of adding new methods to the RPC that is being exported from the Polykey core library.

There was some discussion about this earlier when @tegefaulkes asked about the tradeoffs puttin the RPC definitions into the Polykey Core library.

that is what i have done, it is shown as the versionMetadata property on the return of agentStatus. The environment variables are neccessary for the source substitution in ESBuild to know what to substitute it with. The environment variable must be passed in at build time for this reason.

`cliAgentCommitHash` is now available on `CommandStatus` for the sake of
versioning

feat: added commitHash label to docker build CI script

ci: made pipelines work on this branch for testing

fix: addded git to shell.nix

feat: commitHash exposed from `agentStatus` handler via RPC middleware

fix: renamed to cliAgentCommitHash for less ambiguity

chore: lintfix

fix: npm run build now uses the COMMIT_HASH environment variable if already defined

chore: bump polykey to v1.2.1-alpha.45

fix: `CommandStatus` now uses the `versionMetadata` from `agentStatus` RPC call

fix: `pkg` script doesn't require COMMIT_HASH environment variable

fix: `commitHash` is now derived from the `commitHash.nix` derivation

fix: removed testing conditions to CI jobs
@amydevs amydevs force-pushed the feature-commit-hash branch from 267a563 to 261f76f Compare January 16, 2024 00:48
@amydevs amydevs merged commit f143099 into staging Jan 16, 2024
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.

2 participants