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

[bitnami/postgresql] Enable override the archive_command value in postgresql containers #51872

Closed

Conversation

Alvaro-Campesino
Copy link

Description of the change

This changes enables overriding the value of archive_command int the postgresql.conf when POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR is defined as an environment variable

Default value: /bin/true
Value when overrided: cp %p ${POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR}/%f

Benefits

Now it is possible to store incremental backups in an additional volume. Increasing restore capabilities.

Possible drawbacks

Applicable issues

Additional information

@github-actions github-actions bot added the triage Triage is needed label Oct 13, 2023
@bitnami-bot bitnami-bot requested a review from javsalgar October 13, 2023 16:29
@javsalgar javsalgar changed the title Enable override the archive_command value in postgresql containers [bitnami/postgresql] Enable override the archive_command value in postgresql containers Oct 16, 2023
@javsalgar
Copy link
Contributor

Hi!

Thank you so much for the PR! Could you fix the DCO issue?

@Alvaro-Campesino Alvaro-Campesino force-pushed the incremental_backup branch 2 times, most recently from 888d8f0 to ee0b54c Compare October 17, 2023 18:20
@Alvaro-Campesino
Copy link
Author

@javsalgar DCO issue was fixed. PR is ready for review

@javsalgar javsalgar added the verify Execute verification workflow for these changes label Oct 27, 2023
@github-actions github-actions bot added in-progress and removed triage Triage is needed labels Oct 27, 2023
@bitnami-bot bitnami-bot removed the request for review from javsalgar October 27, 2023 08:36
@bitnami-bot bitnami-bot requested a review from fmulero October 27, 2023 08:36
Copy link
Collaborator

@fmulero fmulero left a comment

Choose a reason for hiding this comment

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

Thanks a lot Alvaro for your contribution.

As general comment, could you apply your changes in just one asset (postgresql for example)? That will help us to test your changes, don't worry about the other assets, once the PR is approved, we will apply the changes to all of them.

I also left some comments, could you take them a glance?

Copy link
Collaborator

@fmulero fmulero left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Alvaro-Campesino

Please implement the changes in only one container, use postgresql container for example and remove changes in postgresql-repmgr and supabase-postgres . That will help us to review and test the changes. Please also fix the DCO, we can't not merge changes if they are not properly signed.

Also I left a little comment, please take it a look.

@Alvaro-Campesino
Copy link
Author

Thanks @fmulero I will do as requested.

@Alvaro-Campesino Alvaro-Campesino force-pushed the incremental_backup branch 2 times, most recently from 0afdd49 to 32e7536 Compare November 7, 2023 10:38
Copy link
Collaborator

@fmulero fmulero left a comment

Choose a reason for hiding this comment

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

Hi @Alvaro-Campesino

Thanks a lot for applying the changes only to postgresql container.

About my comment here I was wrong, if we use a separate volume to store the files, is not needed to create the folders and permissions. Sorry to point you in the wrong direction. Maybe adding a validation to ensure the directory exists would be a good idea in this case.

@@ -614,15 +627,19 @@ postgresql_initialize() {
ensure_dir_exists "$dir"
am_i_root && chown "$POSTGRESQL_DAEMON_USER:$POSTGRESQL_DAEMON_GROUP" "$dir"
done
is_empty_value "$POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR" || ensure_dir_exists "$POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR" && am_i_root && chown "$POSTGRESQL_DAEMON_USER:$POSTGRESQL_DAEMON_GROUP" "$POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR"
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we use a separate volume to store the backups, this piece of code can be removed. Adding a validation in postgresql_validate could be a good idea

Copy link
Author

@Alvaro-Campesino Alvaro-Campesino Nov 15, 2023

Choose a reason for hiding this comment

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

I have been thinking about this and even if it is a separate volume , I think it is a good idea to use ensure_dir_exists as for example when using variables you can set the final path in a variable (which is my current setup), in order to have each pods WAL files in a separate folder.

    - name: POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR
      value: /backup/$(MY_POD_NAME)

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case everything will work smoothly and I agree that's a good idea, you can have a shared backup volume and each container will write in its own folder.

Now if you have this set up (POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR directly points to the volume):

version: '2'

services:
  postgresql:
    image: bitnami/postgresql:debug
    build: .
    ports:
      - '5432:5432'
    volumes:
      - 'postgresql_data:/bitnami/postgresql'
      - 'postgresql_backup:/backups/postgresql'
    environment:
      - 'ALLOW_EMPTY_PASSWORD=yes'
      - 'POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR=/backups/postgresql'

volumes:
  postgresql_data:
    driver: local
  postgresql_backup:
    driver: local

The container will fail because the code below will try to change volume permissions:

debian-11-postgresql-1  | postgresql 08:17:40.12 INFO  ==> Loading custom pre-init scripts...
debian-11-postgresql-1  | postgresql 08:17:40.13 INFO  ==> Initializing PostgreSQL database...
debian-11-postgresql-1  | chmod: changing permissions of '/backups/postgresql': Operation not permitted
debian-11-postgresql-1 exited with code 1

bitnami/postgresql/README.md Outdated Show resolved Hide resolved
@@ -308,6 +308,10 @@ A [Streaming replication](http://www.postgresql.org/docs/9.4/static/warm-standby

In a replication cluster you can have one master and zero or more slaves. When replication is enabled the master node is in read-write mode, while the slaves are in read-only mode. For best performance its advisable to limit the reads to the slaves.

### Setting up database incremental backup storage

* `POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR`: The Path were the archive command will store wal increments using `cp %p ${POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR}/%f` by default `archive_command` value is `/bin/true` stored. No defaults.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please include here some information about the use of a separate volume to store these backup files.

Co-authored-by: Fran Mulero <[email protected]>
Signed-off-by: Alvaro Campesino  <[email protected]>
Signed-off-by: Alvaro Campesino  <[email protected]>
Copy link
Collaborator

@fmulero fmulero left a comment

Choose a reason for hiding this comment

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

Hi @Alvaro-Campesino, I left some comments

Please take them a glance and fix DCO.

Regards

@@ -614,15 +627,19 @@ postgresql_initialize() {
ensure_dir_exists "$dir"
am_i_root && chown "$POSTGRESQL_DAEMON_USER:$POSTGRESQL_DAEMON_GROUP" "$dir"
done
is_empty_value "$POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR" || ensure_dir_exists "$POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR" && am_i_root && chown "$POSTGRESQL_DAEMON_USER:$POSTGRESQL_DAEMON_GROUP" "$POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR"
Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case everything will work smoothly and I agree that's a good idea, you can have a shared backup volume and each container will write in its own folder.

Now if you have this set up (POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR directly points to the volume):

version: '2'

services:
  postgresql:
    image: bitnami/postgresql:debug
    build: .
    ports:
      - '5432:5432'
    volumes:
      - 'postgresql_data:/bitnami/postgresql'
      - 'postgresql_backup:/backups/postgresql'
    environment:
      - 'ALLOW_EMPTY_PASSWORD=yes'
      - 'POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR=/backups/postgresql'

volumes:
  postgresql_data:
    driver: local
  postgresql_backup:
    driver: local

The container will fail because the code below will try to change volume permissions:

debian-11-postgresql-1  | postgresql 08:17:40.12 INFO  ==> Loading custom pre-init scripts...
debian-11-postgresql-1  | postgresql 08:17:40.13 INFO  ==> Initializing PostgreSQL database...
debian-11-postgresql-1  | chmod: changing permissions of '/backups/postgresql': Operation not permitted
debian-11-postgresql-1 exited with code 1

am_i_root && find "$POSTGRESQL_DATA_DIR" -mindepth 1 -maxdepth 1 -not -name ".snapshot" -not -name "lost+found" -exec chown -R "$POSTGRESQL_DAEMON_USER:$POSTGRESQL_DAEMON_GROUP" {} \;
chmod u+rwx "$POSTGRESQL_DATA_DIR" || warn "Lack of permissions on data directory!"
chmod go-rwx "$POSTGRESQL_DATA_DIR" || warn "Lack of permissions on data directory!"
is_empty_value "$POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR" || chmod u+rw "$POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Due to the issue mentioned above, something like this could be worth.

Suggested change
is_empty_value "$POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR" || chmod u+rw "$POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR"
is_empty_value "$POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR" || ! is_file_writable "$POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR" || chmod u+rw "$POSTGRESQL_WAL_ARCHIVE_COMMAND_DIR"

Copy link

github-actions bot commented Dec 6, 2023

This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution.

@github-actions github-actions bot added the stale 15 days without activity label Dec 6, 2023
Copy link

Due to the lack of activity in the last 5 days since it was marked as "stale", we proceed to close this Pull Request. Do not hesitate to reopen it later if necessary.

@bitnami-bot bitnami-bot added stale 15 days without activity and removed stale 15 days without activity labels Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
postgresql solved stale 15 days without activity verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bitnami/postgresql*] Not posible to define archive_command for incremental backup
4 participants