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

#1882 - Recovery and backup startegy patroni #2057

Merged
merged 14 commits into from
Jun 30, 2023

Conversation

guru-aot
Copy link
Collaborator

@guru-aot guru-aot commented Jun 28, 2023

image

export BACKUP_MONGODB_APP_NAME := $(or $(BACKUP_MONGODB_APP_NAME), mongodb-backup)
export BACKUP_CONFIGMAP_NAME := $(or $(BACKUP_CONFIGMAP_NAME), backup-conf)
export BACKUP_POSTGRESQL_APP_NAME := $(or $(BACKUP_POSTGRESQL_APP_NAME), patroni-simsdb-backup)
export PATRONI_SIMSDB_BACKUP_CONFIGMAP_NAME := $(or $(PATRONI_SIMSDB_BACKUP_CONFIGMAP_NAME), patroni-simsdb-backup-conf)
Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot Jun 28, 2023

Choose a reason for hiding this comment

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

When this was mentioned "The backup.conf should be created for databases separately" does it means that they need to be created per database or per database type? For instance, Mongo/Postgres? In case we have a second DB on Postgres, do we need to create a second backup structure?
IMO, the answer to this question can help determine the variable's names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah for seperate database not the database type, because individual databases can have different schedule of backups

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, "The backup.conf should be created for databases separately" actually means that the backups can be configured for multiple databases but we are actually structuring it in a way that it will be per database assuming that a new database will have a different backup schedule, right?

Does it mean that in the future, if we have more than one DB, instead of a new line in a config file we will create a separate structure to handle the new backup?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes you got it right, that was the plan, as individual backup stratergy for different databases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I went through the conversation @guru-aot @andrewsignori-aot .

@guru-aot what could be the benefit of having individual backup strategy per db?

@@ -73,4 +71,4 @@ parameters:
displayName: FROM Image Tag
description: Base image to build from. Docker creds or Artificatory setup may be needed to alleviate docker rate-limiting
required: true
value: artifacts.developer.gov.bc.ca/docker-remote/centos/postgresql-12-centos7:20200917-804ef01
value: docker.io/centos/postgresql-12-centos7:20200917-804ef01
Copy link
Collaborator

Choose a reason for hiding this comment

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

artifacts.developer.gov.bc.ca/docker-remote is not the same as docker.io?
Are we not supposed to be using the artifacts URL as much as possible (if not for everything)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes you are right, but in this case, the build.yaml has been copied from the https://github.com/BCDevOps/backup-container/blob/master/openshift/templates/backup/backup-build.yaml. And the base image is actually not uses this too instead it uses quay.io/fedora/postgresql-14:14, as per the suggestion from the bcgov developer who did the changes to the dockerfile, which supports postgres 14.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@guru-aot does artifacts.developer.gov.bc.ca/docker-remote/centos/postgresql-12-centos7:20200917-804ef01 work? as artifacts.developer.gov.bc.ca/docker-remote is just a proxy.
Correct me if wrong.

IMO only if it does not work we should start thinking about docker.io IMO.

@@ -170,16 +169,16 @@ objects:
value: ${MONGODB_AUTHENTICATION_DATABASE}
- name: TABLE_SCHEMA
value: ${TABLE_SCHEMA}
- name: ${DATABASE_SERVER_NAME}_USER
- name: DATABASE_USER
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we removing the "_USER" because we will no longer support the backup for multiple DBs?
"_USER" reference: https://github.com/BCDevOps/backup-container/tree/master#backupconf

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we decide to move away from back-up of multiple db in same backup container?

If there is a requirement in future to have a another db, or we if decide to back up redis(by any chance) current way will be easily adaptable right?

# Create the POSTGRES structure
db-backup-build-postgresql:
db-backup-build-patroni:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this also include SIMS to be aligned with the other renamed variables?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i specifically did not add SIMS, becuase build is going to be same for all, but while deploying we can have multiple databases with different names.

Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot left a comment

Choose a reason for hiding this comment

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

Thanks for the research on this. Few comments to get moe clarity 😉

devops/Makefile Outdated
@@ -143,16 +143,15 @@ oc-build-forms: | print-status build-forms
oc-deploy-ha-mongo: | print-status deploy-ha-mongo
oc-deploy-forms: | print-status deploy-forms

# Build backup structure
Copy link
Contributor

Choose a reason for hiding this comment

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

minor - preriod

Copy link
Contributor

@ann-aot ann-aot left a comment

Choose a reason for hiding this comment

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

Whenever available, I would like to have a walk-through for my information

@@ -73,4 +71,4 @@ parameters:
displayName: FROM Image Tag
description: Base image to build from. Docker creds or Artificatory setup may be needed to alleviate docker rate-limiting
required: true
value: artifacts.developer.gov.bc.ca/docker-remote/centos/postgresql-12-centos7:20200917-804ef01
value: docker.io/centos/postgresql-12-centos7:20200917-804ef01
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we claim that current backup supports postgres 14 with this image?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it supports Postgres 14, as per the fedora postgres 14 base image

@@ -1,7 +1,4 @@
postgres=patroni-master:5432/SIMSDB
postgres=patroni-master:5432/FFA_API_DB
postgres=patroni-master:5432/FFA_BPM_DB
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@dheepak-aot
Copy link
Collaborator

dheepak-aot commented Jun 28, 2023

Great work. Added some comments for my understanding and clarity.

Also, as we are in production curious to know about enabling FTP. I know it is outside the PR context but when I looked at the readme of back up container it rang a bell to ask about it.

Copy link
Collaborator

@dheepak-aot dheepak-aot left a comment

Choose a reason for hiding this comment

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

Thanks for the walk through 👍

Copy link
Contributor

@ann-aot ann-aot left a comment

Choose a reason for hiding this comment

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

Thanks for the walk through @guru-aot 👍

Copy link
Collaborator

@andrepestana-aot andrepestana-aot left a comment

Choose a reason for hiding this comment

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

Thanks for the explanations!

Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot left a comment

Choose a reason for hiding this comment

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

Thanks for making this happen. Looks good 👍

@andrewsignori-aot andrewsignori-aot changed the title #1882-Recovery and backup startegy patroni #1882 - Recovery and backup startegy patroni Jun 30, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 17.91% ( 2127 / 11875 )
Methods: 8.33% ( 126 / 1513 )
Lines: 20.71% ( 1863 / 8997 )
Branches: 10.11% ( 138 / 1365 )

@github-actions
Copy link

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 71% ( 399 / 562 )
Methods: 61.97% ( 44 / 71 )
Lines: 72.97% ( 351 / 481 )
Branches: 40% ( 4 / 10 )

@github-actions
Copy link

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 49.81% ( 267 / 536 )
Methods: 41.56% ( 32 / 77 )
Lines: 55.33% ( 218 / 394 )
Branches: 26.15% ( 17 / 65 )

@github-actions
Copy link

E2E SIMS API Coverage Report

Totals Coverage
Statements: 50.18% ( 3543 / 7061 )
Methods: 45.63% ( 418 / 916 )
Lines: 55.38% ( 2915 / 5264 )
Branches: 23.84% ( 210 / 881 )

@guru-aot guru-aot merged commit 0f24257 into main Jun 30, 2023
@guru-aot guru-aot deleted the feature/#1882-Recovery-and-backup-strategy-of-Patroni branch June 30, 2023 18:12
@guru-aot guru-aot temporarily deployed to DEV June 30, 2023 18:45 — with GitHub Actions Inactive
@guru-aot guru-aot temporarily deployed to DEV June 30, 2023 18:55 — with GitHub Actions Inactive
@guru-aot guru-aot temporarily deployed to DEV June 30, 2023 18:55 — with GitHub Actions Inactive
@guru-aot guru-aot temporarily deployed to DEV June 30, 2023 18:55 — with GitHub Actions Inactive
@guru-aot guru-aot temporarily deployed to DEV June 30, 2023 18:55 — with GitHub Actions Inactive
@guru-aot guru-aot temporarily deployed to DEV June 30, 2023 19:04 — with GitHub Actions Inactive
@guru-aot guru-aot temporarily deployed to DEV June 30, 2023 19:04 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Devops Devops
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants