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

ci: build minimal and standard images #135

Merged
merged 13 commits into from
Jan 16, 2025
Merged

ci: build minimal and standard images #135

merged 13 commits into from
Jan 16, 2025

Conversation

fcanovai
Copy link
Contributor

@fcanovai fcanovai commented Jan 9, 2025

Build images without barman-cloud, to be used with backup plugins.

Other changes:

  • Implement timestamp-based versioning for images
  • Simplify build workflows for enhanced local testing and contribution
  • Adopt OCI annotations and generate SBOMs for improved transparency

Closes #132

@fcanovai fcanovai force-pushed the dev/bake branch 8 times, most recently from 74b3d82 to 916a3ce Compare January 9, 2025 13:44
@ardentperf
Copy link

I tested out the workflow of creating a container image with an extra extension. Last time I did this, I cloned the repo and used GitHub Actions to do the actual build. It seemed like a fairly complicated process (though maybe there was a simple way I could do it locally, which I didn't know). With this new build workflow based on docker bake, I was able to very easily run the build locally. It seems easier to me.

The BUILD.md documentation is great, especially the notes about how to limit to a specific build (great for testing) and pushing to local and external registries.

I did test using a local registry and that worked for me.

Nonetheless I ended up going back to a personal account at docker.io to test my image, because with a cursory google search I didn't quickly find info about getting KIND to work with a local registry.

Here is the complete end-to-end workflow for using this new build process to create a custom container image with a specific non-default postgres extension I'm interested in.

### postgres-containers repo

git diff
│diff --git a/Dockerfile b/Dockerfile                                                      
│index 548e531..6e9c4e9 100644                                                            
│--- a/Dockerfile                                                                        
│+++ b/Dockerfile                                                                       
│@@ -30,6 +30,7 @@ RUN apt-get update && \                                             
│     apt-get install -y --no-install-recommends locales-all \                        
│       "postgresql-${PG_MAJOR}-pgaudit" \                                           
│       "postgresql-${PG_MAJOR}-pgvector" \                                         
│+      "postgresql-${PG_MAJOR}-pg-hint-plan" \                                    
│       "postgresql-${PG_MAJOR}-pg-failover-slots" && \                           
│     apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false && \
│     rm -rf /var/lib/apt/lists/* /var/cache/* /var/log/*  

docker login
registry=docker.io/ardentperf docker buildx bake --push --set "*.platform=linux/amd64" postgresql-17-standard-bookworm 

### check at dockerhub and confirm the image was uploaded to my personal account

### cnpg-playground repo

scripts/setup.sh 

kubectl ctx kind-k8s-eu

helm repo add cnpg https://cloudnative-pg.github.io/charts
helm upgrade --install cpng --namespace cnpg-system --create-namespace cnpg/cloudnative-pg

cat <<EOF |kubectl apply -f -
apiVersion: postgresql.cnpg.io/v1
kind: Cluster
metadata:
  name: cluster-c
spec:
  instances: 1
  imageName: "docker.io/ardentperf/postgresql-testing:17-standard-bookworm"
  storage:
    size: 1Gi
EOF

kubectl cnpg psql cluster-c -- -c "create extension pg_hint_plan"
│CREATE EXTENSION                                               

kubectl cnpg psql cluster-c -- -c "\d hint_plan.hints"         
│                               Table "hint_plan.hints"       
│      Column      |  Type   | Collation | Nullable |             Default                
│------------------+---------+-----------+----------+---------------------------------- 
│ id               | integer |           | not null | generated by default as identity 
│ query_id         | bigint  |           | not null |                                 
│ application_name | text    |           | not null |                                
│ hints            | text    |           | not null |                               
│Indexes:                                                                          
│    "hints_pkey" PRIMARY KEY, btree (id)                                         
│    "hints_id_and_app" UNIQUE, btree (query_id, application_name)               

@ardentperf
Copy link

Also, after reading through some of the code, the switch to docker bake seems to be very nice. It now requires reading a few more docker docs to learn how bake works, but the simplification here is great. It seems like bake provides all the needed functionality out of the box and CNPG doesn't need to work around any weird gaps, so seems like an ideal solution. From my [non-expert] perspective this seems like a great improvement to the container build overall.

apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false && \
rm -rf /var/lib/apt/lists/* /var/cache/* /var/log/*

RUN usermod -u 26 postgres

Choose a reason for hiding this comment

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

in the debian package postinst script, both the postgres user and the postgres group are created without hardcoding an ID.

https://salsa.debian.org/postgresql/postgresql-common/-/blob/22bf910531252c373a38175904a12f74ce820e86/debian/postgresql-common.postinst#L31-39

i just tried installing postgres-17 on an empty bookworm-slim container and I got:

root@34bd069f5bfe:/# id postgres
uid=100(postgres) gid=102(postgres) groups=102(postgres),101(ssl-cert)

i'm not entirely sure the reasoning for specifically choosing 26 instead of 100 for cnpg? it doesn't seem like a good coding practice at all to assume a user will have a specific UID

regardless, if we need to lock the UID in cnpg, should we also be ensuring that the GID is always 102 (or explicitly setting it to some other value)?

Copy link
Member

Choose a reason for hiding this comment

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

Historical reasons, 26 has been in use for a lot of things and it was the id long time ago, changing that now will affect a lot of people and is just a UID, in many system is even replaced.

The UID needs to be known for permissions and fs rules and security context, etc.

If we start changing to something else, then Debian may change the rule (as they already did) and we will have to change it again? in my opinion, changing will bring more problems than keep it them. If debian change the default we will have to change again and system already running with the permissions on the old UID may have issues when triggering an update of the image to the new one with a different UID

Choose a reason for hiding this comment

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

makes sense ~ thx for explaining

no historical GID right?

Copy link
Member

Choose a reason for hiding this comment

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

Historically, it has also been GID=26, but this doesn't matter during the image build. The operator works well even if the GID is not present in the image as long as the GID doesn't change.

apt-get install -y --no-install-recommends locales-all \
"postgresql-${PG_MAJOR}-pgaudit" \
"postgresql-${PG_MAJOR}-pgvector" \
"postgresql-${PG_MAJOR}-pg-failover-slots" && \

Choose a reason for hiding this comment

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

over in #132 (comment) I asked if it makes sense to move pg-failover-slots to the minimal image; I'm not sure where the other discussion will go but thought I'd flag it here just as FYI

Copy link
Member

Choose a reason for hiding this comment

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

The minimal needs to be minimal in my opinion, just postgres without anything else

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @sxd. The goal of minimal is to provide just PostgreSQL and allow us to start working on the extension container images project, which will take years to complete.

I would move the existing extensions to standard. After all, as you pointed out, producing images with the new process will be much easier. I expect many image customisations to happen in the future.

@gbartolini gbartolini marked this pull request as ready for review January 13, 2025 12:58
fcanovai and others added 8 commits January 13, 2025 13:59
Build images without barman-cloud, to be used with backup plugins.

Closes #132

Signed-off-by: Francesco Canovai <[email protected]>
Signed-off-by: Gabriele Bartolini <[email protected]>
Signed-off-by: Gabriele Bartolini <[email protected]>
Signed-off-by: Gabriele Bartolini <[email protected]>
Signed-off-by: Gabriele Bartolini <[email protected]>
Signed-off-by: Gabriele Bartolini <[email protected]>
Signed-off-by: Gabriele Bartolini <[email protected]>
gbartolini and others added 4 commits January 14, 2025 11:53
Signed-off-by: Gabriele Bartolini <[email protected]>
Signed-off-by: Francesco Canovai <[email protected]>
Signed-off-by: Niccolò Fei <[email protected]>
by GFedi

Signed-off-by: Gabriele Bartolini <[email protected]>
@mnencia mnencia merged commit c330729 into main Jan 16, 2025
1 check passed
@mnencia mnencia deleted the dev/bake branch January 16, 2025 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactoring of the container image build process for simplification, local testing and broader contributions
7 participants