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

Begin moving to postgres spilo + adding pgvector #8309

Merged
merged 25 commits into from
Nov 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions .github/workflows/ci-server.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,19 @@ jobs:
NX_REJECT_UNKNOWN_LOCAL_CACHE: 0
services:
postgres:
image: twentycrm/twenty-postgres
image: twentycrm/twenty-postgres-spilo
env:
POSTGRES_PASSWORD: postgres
POSTGRES_USER: postgres
PGUSER_SUPERUSER: postgres
PGPASSWORD_SUPERUSER: twenty
ALLOW_NOSSL: "true"
SPILO_PROVIDER: "local"
ports:
- 5432:5432
options: >-
--health-cmd pg_isready
--health-interval 10s
--health-timeout 5s
--health-retries 5
redis:
image: redis
ports:
Expand Down Expand Up @@ -63,6 +70,11 @@ jobs:
- name: Server / Write .env
if: steps.changed-files.outputs.any_changed == 'true'
run: npx nx reset:env twenty-server
- name: Server / Create DB
if: steps.changed-files.outputs.any_changed == 'true'
run: |
PGPASSWORD=twenty psql -h localhost -p 5432 -U postgres -d postgres -c 'CREATE DATABASE "default";'
PGPASSWORD=twenty psql -h localhost -p 5432 -U postgres -d postgres -c 'CREATE DATABASE "test";'
Comment on lines +73 to +77
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Database creation step should be moved before worker run but after environment setup to ensure proper initialization order

- name: Worker / Run
if: steps.changed-files.outputs.any_changed == 'true'
run: npx nx run twenty-server:worker:ci
Expand Down Expand Up @@ -109,12 +121,19 @@ jobs:
needs: server-setup
services:
postgres:
image: twentycrm/twenty-postgres
image: twentycrm/twenty-postgres-spilo
env:
POSTGRES_PASSWORD: postgres
POSTGRES_USER: postgres
PGUSER_SUPERUSER: postgres
PGPASSWORD_SUPERUSER: twenty
ALLOW_NOSSL: "true"
SPILO_PROVIDER: "local"
ports:
- 5432:5432
options: >-
--health-cmd pg_isready
--health-interval 10s
--health-timeout 5s
--health-retries 5
Comment on lines 122 to +136
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Missing database creation step in server-integration-test job that's present in server-setup job

redis:
image: redis
ports:
Expand Down
37 changes: 31 additions & 6 deletions .github/workflows/ci-test-docker-compose.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ concurrency:

jobs:
test:
timeout-minutes: 10
timeout-minutes: 30
runs-on: ubuntu-latest
steps:
- name: Checkout
Expand Down Expand Up @@ -36,32 +36,57 @@ jobs:
yq eval 'del(.services.db.image)' -i docker-compose.yml
yq eval '.services.db.build.context = "../../"' -i docker-compose.yml
yq eval '.services.db.build.dockerfile = "./packages/twenty-docker/twenty-postgres/Dockerfile"' -i docker-compose.yml
yq eval '.services.db.build.dockerfile = "./packages/twenty-docker/twenty-postgres-spilo/Dockerfile"' -i docker-compose.yml
echo "Setting up .env file..."
cp .env.example .env
echo "Generating secrets..."
echo "# === Randomly generated secrets ===" >>.env
echo "APP_SECRET=$(openssl rand -base64 32)" >>.env
echo "POSTGRES_ADMIN_PASSWORD=$(openssl rand -base64 32)" >>.env
echo "PGPASSWORD_SUPERUSER=$(openssl rand -base64 32)" >>.env
echo "Starting server..."
docker compose up -d
echo "Docker compose up..."
docker compose up -d || {
echo "Docker compose failed to start"
docker compose logs
exit 1
}
docker compose logs db server -f &
pid=$!
Comment on lines 54 to 55
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Background process pid is never killed if containers start successfully - potential resource leak

echo "Waiting for database to start..."
count=0
while [ ! $(docker inspect --format='{{.State.Health.Status}}' twenty-db-1) = "healthy" ]; do
sleep 1;
count=$((count+1));
if [ $(docker inspect --format='{{.State.Status}}' twenty-db-1) = "exited" ]; then
echo "Database exited"
docker compose logs db
exit 1
fi
if [ $count -gt 300 ]; then
echo "Failed to start database after 5 minutes"
docker compose logs db
exit 1
fi
echo "Still waiting for database... (${count}/60)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Message shows '(${count}/60)' but timeout is 300 - inconsistent messaging

done
echo "Waiting for server to start..."
count=0
while [ ! $(docker inspect --format='{{.State.Health.Status}}' twenty-server-1) = "healthy" ]; do
sleep 1;
count=$((count+1));
if [ $(docker inspect --format='{{.State.Status}}' twenty-server-1) = "exited" ]; then
echo "Server exited"
docker compose logs server
exit 1
fi
if [ $count -gt 300 ]; then
echo "Failed to start server"
echo "Failed to start server after 5 minutes"
docker compose logs server
exit 1
fi
echo "Still waiting for server... (${count}/300s)"
done
working-directory: ./packages/twenty-docker/
21 changes: 16 additions & 5 deletions .github/workflows/ci-website.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,19 @@ jobs:
runs-on: ubuntu-latest
services:
postgres:
image: twentycrm/twenty-postgres
image: twentycrm/twenty-postgres-spilo
env:
POSTGRES_PASSWORD: twenty
POSTGRES_USER: twenty
PGUSER_SUPERUSER: postgres
PGPASSWORD_SUPERUSER: twenty
ALLOW_NOSSL: "true"
SPILO_PROVIDER: "local"
ports:
- 5432:5432
options: >-
--health-cmd pg_isready
--health-interval 10s
--health-timeout 5s
--health-retries 5
steps:
- uses: actions/checkout@v4
with:
Expand All @@ -37,16 +44,20 @@ jobs:
if: steps.changed-files.outputs.changed == 'true'
uses: ./.github/workflows/actions/yarn-install

- name: Server / Create DB
if: steps.changed-files.outputs.any_changed == 'true'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Condition uses any_changed but previous steps use changed - this inconsistency could cause the database creation to run when it shouldn't or vice versa

run: PGPASSWORD=twenty psql -h localhost -p 5432 -U postgres -d postgres -c 'CREATE DATABASE "default";'
Comment on lines +47 to +49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Database creation step should include error handling in case database already exists


- name: Website / Run migrations
if: steps.changed-files.outputs.changed == 'true'
run: npx nx database:migrate twenty-website
env:
DATABASE_PG_URL: postgres://twenty:twenty@localhost:5432/default
DATABASE_PG_URL: postgres://postgres:twenty@localhost:5432/default
- name: Website / Build Website
if: steps.changed-files.outputs.changed == 'true'
run: npx nx build twenty-website
env:
DATABASE_PG_URL: postgres://twenty:twenty@localhost:5432/default
DATABASE_PG_URL: postgres://postgres:twenty@localhost:5432/default

- name: Mark as VALID
if: steps.changed-files.outputs.changed != 'true' # If no changes, mark as valid
Expand Down
4 changes: 0 additions & 4 deletions .vscode/twenty.code-workspace
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@
"name": "packages/twenty-emails",
"path": "../packages/twenty-emails"
},
{
"name": "packages/twenty-postgres",
"path": "../packages/twenty-postgres"
},
{
"name": "packages/twenty-server",
"path": "../packages/twenty-server"
Expand Down
22 changes: 15 additions & 7 deletions Makefile
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
postgres-on-docker:
docker run \
--name twenty_postgres \
-e POSTGRES_USER=postgres \
-e POSTGRES_PASSWORD=postgres \
-e POSTGRES_DB=default \
-v twenty_db_data:/var/lib/postgresql/data \
docker run -d \
--name twenty_pg \
-e PGUSER_SUPERUSER=postgres \
-e PGPASSWORD_SUPERUSER=twenty \
Comment on lines +4 to +5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: PGUSER_SUPERUSER and PGPASSWORD_SUPERUSER values don't match the test instructions in PR description (which uses 'twenty' as the username)

-e ALLOW_NOSSL=true \
-v twenty_db_data:/home/postgres/pgdata \
-p 5432:5432 \
twentycrm/twenty-postgres:latest
twentycrm/twenty-postgres-spilo:latest
@echo "Waiting for PostgreSQL to be ready..."
@until PGPASSWORD=twenty psql -h localhost -p 5432 -U postgres -d postgres \
-c 'SELECT pg_is_in_recovery();' 2>/dev/null | grep -q 'f'; do \
sleep 1; \
done
Comment on lines +11 to +14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Health check loop should have a timeout to prevent infinite waiting if database fails to start

PGPASSWORD=twenty psql -h localhost -p 5432 -U postgres -d postgres \
-c "CREATE DATABASE \"default\" WITH OWNER postgres;" \
-c "CREATE DATABASE \"test\" WITH OWNER postgres;"
Comment on lines +15 to +17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Database creation commands should check if databases already exist to prevent errors on container restart


redis-on-docker:
docker run -d --name twenty_redis -p 6379:6379 redis/redis-stack-server:latest
2 changes: 1 addition & 1 deletion install.sh
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ fi
echo "# === Randomly generated secrets ===" >>.env
echo "APP_SECRET=$(openssl rand -base64 32)" >>.env
echo "" >>.env
echo "POSTGRES_ADMIN_PASSWORD=$(openssl rand -base64 32)" >>.env
echo "PGPASSWORD_SUPERUSER=$(openssl rand -hex 16)" >>.env

echo -e "\t• .env configuration completed"

Expand Down
3 changes: 2 additions & 1 deletion packages/twenty-docker/.env.example
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
TAG=latest

# POSTGRES_ADMIN_PASSWORD=replace_me_with_a_strong_password
#PGUSER_SUPERUSER=postgres
#PGPASSWORD_SUPERUSER=replace_me_with_a_strong_password
Comment on lines +3 to +4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: These variables should not be commented out by default since they are required for Spilo to function properly


PG_DATABASE_HOST=db:5432
REDIS_URL=redis://redis:6379
Expand Down
11 changes: 0 additions & 11 deletions packages/twenty-docker/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@ prod-build:
prod-run:
@docker run -d -p 3000:3000 --name twenty twenty

prod-postgres-build:
@cd ../.. && docker build -f ./packages/twenty-docker/twenty-postgres/Dockerfile --tag twenty-postgres . && cd -

prod-postgres-run:
@docker run -d -p 5432:5432 -e POSTGRES_USER=postgres -e POSTGRES_PASSWORD=postgres --name twenty-postgres twenty-postgres
Comment on lines 7 to 8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: This target tries to run the twenty-postgres image which no longer exists after removing prod-postgres-build. Should be updated to use twenty-postgres-spilo with appropriate environment variables.


Expand All @@ -15,11 +12,3 @@ prod-website-build:

prod-website-run:
@docker run -d -p 3000:3000 --name twenty-website twenty-website

release-postgres:
@cd ../.. && docker buildx build \
--push \
--no-cache \
--platform linux/amd64,linux/arm64 \
-f ./packages/twenty-docker/twenty-postgres/Dockerfile -t twentycrm/twenty-postgres:$(version) -t twentycrm/twenty-postgres:latest . \
&& cd -
19 changes: 11 additions & 8 deletions packages/twenty-docker/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ services:
- "3000:3000"
environment:
PORT: 3000
PG_DATABASE_URL: postgres://twenty:twenty@${PG_DATABASE_HOST}/default
PG_DATABASE_URL: postgres://${PGUSER_SUPERUSER:-postgres}:${PGPASSWORD_SUPERUSER:-twenty}@${PG_DATABASE_HOST:-db:5432}/default
SERVER_URL: ${SERVER_URL}
FRONT_BASE_URL: ${FRONT_BASE_URL:-$SERVER_URL}
REDIS_URL: ${REDIS_URL:-redis://localhost:6379}
REDIS_URL: ${REDIS_URL:-redis://redis:6379}

ENABLE_DB_MIGRATIONS: "true"

Expand All @@ -52,10 +52,10 @@ services:
image: twentycrm/twenty:${TAG}
command: ["yarn", "worker:prod"]
environment:
PG_DATABASE_URL: postgres://twenty:twenty@${PG_DATABASE_HOST}/default
PG_DATABASE_URL: postgres://${PGUSER_SUPERUSER:-postgres}:${PGPASSWORD_SUPERUSER:-twenty}@${PG_DATABASE_HOST:-db:5432}/default
SERVER_URL: ${SERVER_URL}
FRONT_BASE_URL: ${FRONT_BASE_URL:-$SERVER_URL}
REDIS_URL: ${REDIS_URL:-redis://localhost:6379}
REDIS_URL: ${REDIS_URL:-redis://redis:6379}

ENABLE_DB_MIGRATIONS: "false" # it already runs on the server

Expand All @@ -73,13 +73,16 @@ services:
restart: always

db:
image: twentycrm/twenty-postgres:${TAG}
image: twentycrm/twenty-postgres-spilo:${TAG}
volumes:
- db-data:/bitnami/postgresql
- db-data:/home/postgres/pgdata
environment:
POSTGRES_PASSWORD: ${POSTGRES_ADMIN_PASSWORD}
PGUSER_SUPERUSER: ${PGUSER_SUPERUSER:-postgres}
PGPASSWORD_SUPERUSER: ${PGPASSWORD_SUPERUSER:-twenty}
ALLOW_NOSSL: "true"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: consider security implications of ALLOW_NOSSL=true in production environments

SPILO_PROVIDER: "local"
healthcheck:
test: pg_isready -U twenty -d default
test: pg_isready -U ${PGUSER_SUPERUSER:-postgres} -h localhost -d postgres
interval: 5s
timeout: 5s
retries: 10
Expand Down
8 changes: 4 additions & 4 deletions packages/twenty-docker/k8s/manifests/deployment-db.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ spec:
image: twentycrm/twenty-postgres:latest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: image still references twentycrm/twenty-postgres:latest but should be twentycrm/twenty-postgres-spilo:latest based on PR description

imagePullPolicy: Always
env:
- name: POSTGRES_PASSWORD
- name: PGUSER_SUPERUSER
value: "postgres"
- name: PGPASSWORD_SUPERUSER
value: "twenty"
Comment on lines +33 to 36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: consider making these credentials configurable via secrets rather than hardcoding them in the deployment manifest

- name: BITNAMI_DEBUG
value: "true"
ports:
- containerPort: 5432
name: tcp
Expand All @@ -48,7 +48,7 @@ spec:
stdin: true
tty: true
volumeMounts:
- mountPath: /bitnami/postgresql
- mountPath: /home/postgres/pgdata
name: twentycrm-db-data
dnsPolicy: ClusterFirst
restartPolicy: Always
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ spec:
- name: FRONT_BASE_URL
value: "https://crm.example.com:443"
- name: "PG_DATABASE_URL"
value: "postgres://twenty:[email protected]/default"
value: "postgres://postgres:[email protected]/default"
Comment on lines 42 to +43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider using a less privileged database user instead of the postgres superuser for the application connection

- name: "REDIS_URL"
value: "redis://twentycrm-redis.twentycrm.svc.cluster.local:6379"
- name: ENABLE_DB_MIGRATIONS
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ spec:
- name: FRONT_BASE_URL
value: "https://crm.example.com:443"
- name: PG_DATABASE_URL
value: "postgres://twenty:[email protected]/default"
value: "postgres://postgres:[email protected]/default"
Comment on lines 33 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Ensure this connection string matches the credentials configured in the Spilo database deployment. The superuser password should be securely stored and accessed via a secret rather than hardcoded.

- name: ENABLE_DB_MIGRATIONS
value: "false" # it already runs on the server
- name: STORAGE_TYPE
Expand Down
2 changes: 1 addition & 1 deletion packages/twenty-docker/k8s/terraform/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ To make configuration changes to how this doc is generated, see `./.terraform-do
| <a name="input_twentycrm_app_hostname"></a> [twentycrm\_app\_hostname](#input\_twentycrm\_app\_hostname) | The protocol, DNS fully qualified hostname, and port used to access TwentyCRM in your environment. Ex: https://crm.example.com:443 | `string` | n/a | yes |
| <a name="input_twentycrm_pgdb_admin_password"></a> [twentycrm\_pgdb\_admin\_password](#input\_twentycrm\_pgdb\_admin\_password) | TwentyCRM password for postgres database. | `string` | n/a | yes |
Comment on lines 51 to 52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: check if pgdb_admin_password variable needs to be renamed to match new Spilo environment variable PGPASSWORD_SUPERUSER

| <a name="input_twentycrm_app_name"></a> [twentycrm\_app\_name](#input\_twentycrm\_app\_name) | A friendly name prefix to use for every component deployed. | `string` | `"twentycrm"` | no |
| <a name="input_twentycrm_db_image"></a> [twentycrm\_db\_image](#input\_twentycrm\_db\_image) | TwentyCRM image for database deployment. This defaults to latest. | `string` | `"twentycrm/twenty-postgres:latest"` | no |
| <a name="input_twentycrm_db_image"></a> [twentycrm\_db\_image](#input\_twentycrm\_db\_image) | TwentyCRM image for database deployment. This defaults to latest. | `string` | `"twentycrm/twenty-postgres-spilo:latest"` | no |
| <a name="input_twentycrm_db_pv_capacity"></a> [twentycrm\_db\_pv\_capacity](#input\_twentycrm\_db\_pv\_capacity) | Storage capacity provisioned for database persistent volume. | `string` | `"10Gi"` | no |
| <a name="input_twentycrm_db_pv_path"></a> [twentycrm\_db\_pv\_path](#input\_twentycrm\_db\_pv\_path) | Local path to use to store the physical volume if using local storage on nodes. | `string` | `""` | no |
| <a name="input_twentycrm_db_pvc_requests"></a> [twentycrm\_db\_pvc\_requests](#input\_twentycrm\_db\_pvc\_requests) | Storage capacity reservation for database persistent volume claim. | `string` | `"10Gi"` | no |
Expand Down
2 changes: 1 addition & 1 deletion packages/twenty-docker/k8s/terraform/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ variable "twentycrm_server_image" {

variable "twentycrm_db_image" {
type = string
default = "twentycrm/twenty-postgres:latest"
default = "twentycrm/twenty-postgres-spilo:latest"
description = "TwentyCRM image for database deployment. This defaults to latest."
}

Expand Down
10 changes: 5 additions & 5 deletions packages/twenty-docker/twenty-postgres-spilo/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ ARG SPILO_VERSION=3.2-p1
ARG WRAPPERS_VERSION=0.2.0

# Build the mysql_fdw extension
FROM debian:bookworm as build-mysql_fdw
FROM debian:bookworm AS build-mysql_fdw
ARG POSTGRES_VERSION

ENV DEBIAN_FRONTEND noninteractive
ENV DEBIAN_FRONTEND=noninteractive
RUN apt update && \
apt install -y \
build-essential \
Expand All @@ -17,14 +17,14 @@ RUN apt update && \

# Install mysql_fdw
RUN git clone https://github.com/EnterpriseDB/mysql_fdw.git
WORKDIR mysql_fdw
WORKDIR /mysql_fdw
RUN make USE_PGXS=1


# Build libssl for wrappers
FROM ubuntu:22.04 as build-libssl
FROM ubuntu:22.04 AS build-libssl

ENV DEBIAN_FRONTEND noninteractive
ENV DEBIAN_FRONTEND=noninteractive
RUN apt update && \
apt install -y \
build-essential \
Expand Down
Loading
Loading