From 807f7906ab7773ffd7154b5e9573d03f3f36978d Mon Sep 17 00:00:00 2001 From: Steve Astels Date: Wed, 28 Feb 2024 16:43:35 -0500 Subject: [PATCH 01/10] add more logging to the remove_sms_email_jobs task (#2125) --- app/celery/nightly_tasks.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py index 0a2ac8d652..c69e32018a 100644 --- a/app/celery/nightly_tasks.py +++ b/app/celery/nightly_tasks.py @@ -50,8 +50,11 @@ def remove_letter_csv_files(): def _remove_csv_files(job_types): jobs = dao_get_jobs_older_than_data_retention(notification_types=job_types) + current_app.logger.info("TEMP LOGGING: trying to remove {} jobs.".format(len(jobs))) for job in jobs: + current_app.logger.info("TEMP LOGGING: trying to remove Job ID {} from s3.".format(job.id)) s3.remove_job_from_s3(job.service_id, job.id) + current_app.logger.info("TEMP LOGGING: trying to archive Job ID {}".format(job.id)) dao_archive_job(job) current_app.logger.info("Job ID {} has been removed from s3.".format(job.id)) From bfadea3bb7ba83cf57d3d206e907b01bfa9239ef Mon Sep 17 00:00:00 2001 From: Jimmy Royer Date: Thu, 29 Feb 2024 12:11:25 -0500 Subject: [PATCH 02/10] Updated the PR template to latest agreed format (#2122) * Updated the PR template to latest agreed format. * Reworked format and removed question marks in reviewer's checklist. * Added French translation for the related issues section. * Reworded the reviewer's checklist. * Removed superfluous description * Added further instructions on reviewer's checklist. --- pull_request_template.md | 61 ++++++++-------------------------------- 1 file changed, 11 insertions(+), 50 deletions(-) diff --git a/pull_request_template.md b/pull_request_template.md index f1ffb136c6..483d2b2639 100644 --- a/pull_request_template.md +++ b/pull_request_template.md @@ -1,65 +1,26 @@ # Summary | Résumé -> 1-3 sentence description of the changed you're proposing, including a link to -> a GitHub Issue # or Trello card if applicable. +_TODO: 1-3 sentence description of the changed you're proposing._ ---- +## Related Issues | Cartes liées -> Description en 1 à 3 phrases de la modification proposée, avec un lien vers le -> problème (« issue ») GitHub ou la fiche Trello, le cas échéant. +* https://app.zenhub.com/workspaces/notify-planning-614b3ad91bc2030015ed22f5/issues/gh/cds-snc/notification-planning/1 +* https://app.zenhub.com/workspaces/notify-planning-core-6411dfb7c95fb80014e0cab0/issues/gh/cds-snc/notification-planning-core/1 # Test instructions | Instructions pour tester la modification -> Sequential steps (1., 2., 3., ...) that describe how to test this change. This -> will help a developer test things out without too much detective work. Also, -> include any environmental setup steps that aren't in the normal README steps -> and/or any time-based elements that this requires. - ---- - -> Étapes consécutives (1., 2., 3., …) qui décrivent la façon de tester la -> modification. Elles aideront les développeurs à faire des tests sans avoir à -> jouer au détective. Veuillez aussi inclure toutes les étapes de configuration -> de l’environnement qui ne font pas partie des étapes normales dans le fichier -> README et tout élément temporel requis. +_TODO: Fill in test instructions for the reviewer._ # Release Instructions | Instructions pour le déploiement None. -> Necessary steps to perform before and after the deployment of these changes. -> For example, emptying the cache on a feature that changes the cache data -> structure in Redis could be mentioned. - ---- - -> Étapes nécessaires à exécuter avant et après le déploiement des changements -> introduits par cette proposition. Par exemple, vider la cache suite à des -> changements modifiant une structure de données de la cache pourrait être -> mentionné. - # Reviewer checklist | Liste de vérification du réviseur -This is a suggested checklist of questions reviewers might ask during their -review | Voici une suggestion de liste de vérification comprenant des questions -que les réviseurs pourraient poser pendant leur examen : - +- [ ] This PR does not break existing functionality. +- [ ] This PR does not violate GCNotify's privacy policies. +- [ ] This PR does not raise new security concerns. Refer to our GC Notify Risk Register document on our Google drive. +- [ ] This PR does not significantly alter performance. +- [ ] Additional required documentation resulting of these changes is covered (such as the README, setup instructions, a related ADR or the technical documentation). -- [ ] Is the code maintainable? | Est-ce que le code peut être maintenu? -- [ ] Have you tested it? | L’avez-vous testé? -- [ ] Are there automated tests? | Y a-t-il des tests automatisés? -- [ ] Does this cause automated test coverage to drop? | Est-ce que ça entraîne - une baisse de la quantité de code couvert par les tests automatisés? -- [ ] Does this break existing functionality? | Est-ce que ça brise une - fonctionnalité existante? -- [ ] Does this change the privacy policy? | Est-ce que ça entraîne une - modification de la politique de confidentialité? -- [ ] Does this introduce any security concerns? | Est-ce que ça introduit des - préoccupations liées à la sécurité? -- [ ] Does this significantly alter performance? | Est-ce que ça modifie de - façon importante la performance? -- [ ] What is the risk level of using added dependencies? | Quel est le degré de - risque d’utiliser des dépendances ajoutées? -- [ ] Should any documentation be updated as a result of this? (i.e. README - setup, etc.) | Faudra-t-il mettre à jour la documentation à la suite de ce - changement (fichier README, etc.)? +> ⚠ If boxes cannot be checked off before merging the PR, they should be moved to the "Release Instructions" section with appropriate steps required to verify before release. For example, changes to celery code may require tests on staging to verify that performance has not been affected. \ No newline at end of file From 6f4903dd9f03aabca626df05ce79c4771b8b7f5f Mon Sep 17 00:00:00 2001 From: Steve Astels Date: Tue, 5 Mar 2024 16:43:07 -0500 Subject: [PATCH 03/10] add diagnostic logging to create_job (#2130) --- app/job/rest.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/job/rest.py b/app/job/rest.py index 950f1554c9..a5e6c0e2ae 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -136,6 +136,7 @@ def get_jobs_by_service(service_id): @job_blueprint.route("", methods=["POST"]) def create_job(service_id): service = dao_fetch_service_by_id(service_id) + current_app.logger.info(" TEMP LOGGING 1: done dao_fetch_service_by_id") if not service.active: raise InvalidRequest("Create job is not allowed: service is inactive ", 403) @@ -146,6 +147,7 @@ def create_job(service_id): data.update(**get_job_metadata_from_s3(service_id, data["id"])) except KeyError: raise InvalidRequest({"id": ["Missing data for required field."]}, status_code=400) + current_app.logger.info(" TEMP LOGGING 2: done data.update") if data.get("valid") != "True": raise InvalidRequest("File is not valid, can't create job", 400) @@ -153,18 +155,21 @@ def create_job(service_id): data["template"] = data.pop("template_id") template = dao_get_template_by_id(data["template"]) + current_app.logger.info(" TEMP LOGGING 3: done dao_get_template_by_id") template_errors = unarchived_template_schema.validate({"archived": template.archived}) if template_errors: raise InvalidRequest(template_errors, status_code=400) job = get_job_from_s3(service_id, data["id"]) + current_app.logger.info(" TEMP LOGGING 4: done get_job_from_s3") recipient_csv = RecipientCSV( job, template_type=template.template_type, placeholders=template._as_utils_template().placeholders, template=Template(template.__dict__), ) + current_app.logger.info(" TEMP LOGGING 5: done RecipientCSV()") if template.template_type == SMS_TYPE: # calculate the number of simulated recipients @@ -189,6 +194,7 @@ def create_job(service_id): if scheduled_for is None or not scheduled_for.date() > datetime.today().date(): increment_email_daily_count_send_warnings_if_needed(service, len(list(recipient_csv.get_rows()))) + current_app.logger.info(" TEMP LOGGING 6: done checking limits") data.update({"template_version": template.version}) @@ -198,9 +204,11 @@ def create_job(service_id): job.job_status = JOB_STATUS_SCHEDULED dao_create_job(job) + current_app.logger.info(" TEMP LOGGING 7: done dao_create_job") if job.job_status == JOB_STATUS_PENDING: process_job.apply_async([str(job.id)], queue=QueueNames.JOBS) + current_app.logger.info(" TEMP LOGGING 8: done process_job.apply_async") job_json = job_schema.dump(job) job_json["statistics"] = [] From b33730afa68bb62fbfbf28067d6f603516a5fec5 Mon Sep 17 00:00:00 2001 From: Steve Astels Date: Wed, 6 Mar 2024 14:57:03 -0500 Subject: [PATCH 04/10] Add logging and tweak recipient_csv use (#2132) --- app/job/rest.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/job/rest.py b/app/job/rest.py index a5e6c0e2ae..00b55e4cde 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -173,27 +173,27 @@ def create_job(service_id): if template.template_type == SMS_TYPE: # calculate the number of simulated recipients - numberOfSimulated = sum( - simulated_recipient(i["phone_number"].data, template.template_type) for i in list(recipient_csv.get_rows()) - ) - mixedRecipients = numberOfSimulated > 0 and numberOfSimulated != len(list(recipient_csv.get_rows())) + numberOfSimulated = sum(simulated_recipient(i["phone_number"].data, template.template_type) for i in recipient_csv.rows) + mixedRecipients = numberOfSimulated > 0 and numberOfSimulated != len(recipient_csv) # if they have specified testing and NON-testing recipients, raise an error if mixedRecipients: raise InvalidRequest(message="Bulk sending to testing and non-testing numbers is not supported", status_code=400) - is_test_notification = len(list(recipient_csv.get_rows())) == numberOfSimulated + is_test_notification = len(recipient_csv) == numberOfSimulated if not is_test_notification: check_sms_daily_limit(service, len(recipient_csv)) increment_sms_daily_count_send_warnings_if_needed(service, len(recipient_csv)) elif template.template_type == EMAIL_TYPE: - check_email_daily_limit(service, len(list(recipient_csv.get_rows()))) + check_email_daily_limit(service, len(recipient_csv)) + current_app.logger.info(" TEMP LOGGING 6a: done check_email_daily_limit") + scheduled_for = datetime.fromisoformat(data.get("scheduled_for")) if data.get("scheduled_for") else None if scheduled_for is None or not scheduled_for.date() > datetime.today().date(): - increment_email_daily_count_send_warnings_if_needed(service, len(list(recipient_csv.get_rows()))) + increment_email_daily_count_send_warnings_if_needed(service, len(recipient_csv)) current_app.logger.info(" TEMP LOGGING 6: done checking limits") data.update({"template_version": template.version}) From 25645042248c4996fcf079a28a64a7823a671aa0 Mon Sep 17 00:00:00 2001 From: Jumana B Date: Thu, 7 Mar 2024 10:02:44 -0500 Subject: [PATCH 05/10] Add org_id to Email Branding Table (#2128) * Add organisation_id to email_branding * fix format * fix * test updates * fixes --- app/dao/organisation_dao.py | 13 +++++- app/models.py | 8 +++- .../versions/0445_add_org_id_branding.py | 46 +++++++++++++++++++ tests/app/email_branding/test_rest.py | 7 ++- 4 files changed, 69 insertions(+), 5 deletions(-) create mode 100644 migrations/versions/0445_add_org_id_branding.py diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py index 8c2ef63ddd..06ed25958d 100644 --- a/app/dao/organisation_dao.py +++ b/app/dao/organisation_dao.py @@ -2,7 +2,14 @@ from app import db from app.dao.dao_utils import transactional, version_class -from app.models import Domain, InvitedOrganisationUser, Organisation, Service, User +from app.models import ( + Domain, + EmailBranding, + InvitedOrganisationUser, + Organisation, + Service, + User, +) def dao_get_organisations(): @@ -55,6 +62,10 @@ def dao_update_organisation(organisation_id, **kwargs): domains = kwargs.pop("domains", None) num_updated = Organisation.query.filter_by(id=organisation_id).update(kwargs) + if "email_branding_id" in kwargs: + email_brand = EmailBranding.query.filter_by(id=kwargs["email_branding_id"]).one() + org = Organisation.query.get(organisation_id) + org.email_branding = email_brand if isinstance(domains, list): Domain.query.filter_by(organisation_id=organisation_id).delete() diff --git a/app/models.py b/app/models.py index 704ccf798a..81767406a1 100644 --- a/app/models.py +++ b/app/models.py @@ -276,6 +276,10 @@ class EmailBranding(BaseModel): nullable=False, default=BRANDING_ORG_NEW, ) + organisation_id = db.Column( + UUID(as_uuid=True), db.ForeignKey("organisation.id", ondelete="SET NULL"), index=True, nullable=True + ) + organisation = db.relationship("Organisation", back_populates="email_branding", foreign_keys=[organisation_id]) def serialize(self) -> dict: serialized = { @@ -285,6 +289,7 @@ def serialize(self) -> dict: "name": self.name, "text": self.text, "brand_type": self.brand_type, + "organisation_id": str(self.organisation_id) if self.organisation_id else "", } return serialized @@ -449,10 +454,9 @@ class Organisation(BaseModel): "Domain", ) - email_branding = db.relationship("EmailBranding") + email_branding = db.relationship("EmailBranding", uselist=False) email_branding_id = db.Column( UUID(as_uuid=True), - db.ForeignKey("email_branding.id"), nullable=True, ) diff --git a/migrations/versions/0445_add_org_id_branding.py b/migrations/versions/0445_add_org_id_branding.py new file mode 100644 index 0000000000..0504d5f492 --- /dev/null +++ b/migrations/versions/0445_add_org_id_branding.py @@ -0,0 +1,46 @@ +""" +Revision ID: 0445_add_org_id_branding +Revises: 0444_add_index_n_history2.py +Create Date: 2024-02-27 +""" +import sqlalchemy as sa +from alembic import op +from sqlalchemy.dialects import postgresql + +revision = "0445_add_org_id_branding" +down_revision = "0444_add_index_n_history2" + + +def upgrade(): + op.add_column( + "email_branding", + sa.Column("organisation_id", postgresql.UUID(as_uuid=True), nullable=True), + ) + op.create_index( + op.f("ix_email_branding_organisation_id"), + "email_branding", + ["organisation_id"], + unique=False, + ) + op.create_foreign_key( + "fk_email_branding_organisation", + "email_branding", + "organisation", + ["organisation_id"], + ["id"], + ondelete="SET NULL", + ) + op.drop_constraint("fk_organisation_email_branding_id", "organisation", type_="foreignkey") + + +def downgrade(): + op.drop_index(op.f("ix_email_branding_organisation_id"), table_name="email_branding") + op.drop_constraint("fk_email_branding_organisation", "email_branding", type_="foreignkey") + op.drop_column("email_branding", "organisation_id") + op.create_foreign_key( + "fk_organisation_email_branding_id", + "organisation", + "email_branding", + ["email_branding_id"], + ["id"], + ) diff --git a/tests/app/email_branding/test_rest.py b/tests/app/email_branding/test_rest.py index c09218d62d..9d7bd1f6f7 100644 --- a/tests/app/email_branding/test_rest.py +++ b/tests/app/email_branding/test_rest.py @@ -4,8 +4,8 @@ from tests.app.db import create_email_branding -def test_get_email_branding_options(admin_request, notify_db, notify_db_session): - email_branding1 = EmailBranding(colour="#FFFFFF", logo="/path/image.png", name="Org1") +def test_get_email_branding_options(admin_request, notify_db, notify_db_session, sample_organisation): + email_branding1 = EmailBranding(colour="#FFFFFF", logo="/path/image.png", name="Org1", organisation_id=sample_organisation.id) email_branding2 = EmailBranding(colour="#000000", logo="/path/other.png", name="Org2") notify_db.session.add_all([email_branding1, email_branding2]) notify_db.session.commit() @@ -17,6 +17,8 @@ def test_get_email_branding_options(admin_request, notify_db, notify_db_session) str(email_branding1.id), str(email_branding2.id), } + assert email_branding[0]["organisation_id"] == str(sample_organisation.id) + assert email_branding[1]["organisation_id"] == "" def test_get_email_branding_by_id(admin_request, notify_db, notify_db_session): @@ -37,6 +39,7 @@ def test_get_email_branding_by_id(admin_request, notify_db, notify_db_session): "id", "text", "brand_type", + "organisation_id", } assert response["email_branding"]["colour"] == "#FFFFFF" assert response["email_branding"]["logo"] == "/path/image.png" From 4c0ce9a94ca62e39f0bd62c28357637306b34d7c Mon Sep 17 00:00:00 2001 From: Steve Astels Date: Thu, 7 Mar 2024 11:49:31 -0500 Subject: [PATCH 06/10] get notification count faster (#2133) --- app/job/rest.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/app/job/rest.py b/app/job/rest.py index 00b55e4cde..214eaea866 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -136,7 +136,6 @@ def get_jobs_by_service(service_id): @job_blueprint.route("", methods=["POST"]) def create_job(service_id): service = dao_fetch_service_by_id(service_id) - current_app.logger.info(" TEMP LOGGING 1: done dao_fetch_service_by_id") if not service.active: raise InvalidRequest("Create job is not allowed: service is inactive ", 403) @@ -147,7 +146,6 @@ def create_job(service_id): data.update(**get_job_metadata_from_s3(service_id, data["id"])) except KeyError: raise InvalidRequest({"id": ["Missing data for required field."]}, status_code=400) - current_app.logger.info(" TEMP LOGGING 2: done data.update") if data.get("valid") != "True": raise InvalidRequest("File is not valid, can't create job", 400) @@ -155,21 +153,18 @@ def create_job(service_id): data["template"] = data.pop("template_id") template = dao_get_template_by_id(data["template"]) - current_app.logger.info(" TEMP LOGGING 3: done dao_get_template_by_id") template_errors = unarchived_template_schema.validate({"archived": template.archived}) if template_errors: raise InvalidRequest(template_errors, status_code=400) job = get_job_from_s3(service_id, data["id"]) - current_app.logger.info(" TEMP LOGGING 4: done get_job_from_s3") recipient_csv = RecipientCSV( job, template_type=template.template_type, placeholders=template._as_utils_template().placeholders, template=Template(template.__dict__), ) - current_app.logger.info(" TEMP LOGGING 5: done RecipientCSV()") if template.template_type == SMS_TYPE: # calculate the number of simulated recipients @@ -187,14 +182,13 @@ def create_job(service_id): increment_sms_daily_count_send_warnings_if_needed(service, len(recipient_csv)) elif template.template_type == EMAIL_TYPE: - check_email_daily_limit(service, len(recipient_csv)) - current_app.logger.info(" TEMP LOGGING 6a: done check_email_daily_limit") + notification_count = int(data.get("notification_count", len(recipient_csv))) + check_email_daily_limit(service, notification_count) scheduled_for = datetime.fromisoformat(data.get("scheduled_for")) if data.get("scheduled_for") else None if scheduled_for is None or not scheduled_for.date() > datetime.today().date(): - increment_email_daily_count_send_warnings_if_needed(service, len(recipient_csv)) - current_app.logger.info(" TEMP LOGGING 6: done checking limits") + increment_email_daily_count_send_warnings_if_needed(service, notification_count) data.update({"template_version": template.version}) @@ -204,11 +198,9 @@ def create_job(service_id): job.job_status = JOB_STATUS_SCHEDULED dao_create_job(job) - current_app.logger.info(" TEMP LOGGING 7: done dao_create_job") if job.job_status == JOB_STATUS_PENDING: process_job.apply_async([str(job.id)], queue=QueueNames.JOBS) - current_app.logger.info(" TEMP LOGGING 8: done process_job.apply_async") job_json = job_schema.dump(job) job_json["statistics"] = [] From 9117ec7dd81fe99def8a40f9ebbb35f1b29e5ec9 Mon Sep 17 00:00:00 2001 From: Steve Astels Date: Thu, 7 Mar 2024 14:04:03 -0500 Subject: [PATCH 07/10] Fix slow create_job for real this time (#2135) --- app/job/rest.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/job/rest.py b/app/job/rest.py index 214eaea866..8b29b73ccf 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -182,7 +182,14 @@ def create_job(service_id): increment_sms_daily_count_send_warnings_if_needed(service, len(recipient_csv)) elif template.template_type == EMAIL_TYPE: - notification_count = int(data.get("notification_count", len(recipient_csv))) + if "notification_count" in data: + notification_count = int(data["notification_count"]) + else: + current_app.logger.warning( + f"notification_count not in metadata for job {data['id']}, using len(recipient_csv) instead." + ) + notification_count = len(recipient_csv) + check_email_daily_limit(service, notification_count) scheduled_for = datetime.fromisoformat(data.get("scheduled_for")) if data.get("scheduled_for") else None From da146e2d211d2037cb8ec9e08d78f78eab7fbf0e Mon Sep 17 00:00:00 2001 From: Ben Larabie Date: Thu, 7 Mar 2024 14:54:16 -0500 Subject: [PATCH 08/10] K8s rollout workflow (#2134) Remotely launch k8s workflow --- .github/workflows/docker.yaml | 51 ++++++++--------------------------- 1 file changed, 11 insertions(+), 40 deletions(-) diff --git a/.github/workflows/docker.yaml b/.github/workflows/docker.yaml index e4c5312d51..5fa4e2f343 100644 --- a/.github/workflows/docker.yaml +++ b/.github/workflows/docker.yaml @@ -9,6 +9,7 @@ env: DOCKER_ORG: public.ecr.aws/v6b8u5o6 DOCKER_SLUG: public.ecr.aws/v6b8u5o6/notify-api KUBECTL_VERSION: '1.23.6' + WORKFLOW_PAT: ${{ secrets.WORKFLOW_GITHUB_PAT }} permissions: id-token: write # This is required for requesting the OIDC JWT @@ -26,13 +27,6 @@ jobs: unzip -q awscliv2.zip sudo ./aws/install --update aws --version - - name: Install kubectl - run: | - curl -LO https://storage.googleapis.com/kubernetes-release/release/v$KUBECTL_VERSION/bin/linux/amd64/kubectl - chmod +x ./kubectl - sudo mv ./kubectl /usr/local/bin/kubectl - kubectl version --client - mkdir -p $HOME/.kube - name: Configure credentials to CDS public ECR using OIDC uses: aws-actions/configure-aws-credentials@master @@ -40,7 +34,7 @@ jobs: role-to-assume: arn:aws:iam::283582579564:role/notification-api-apply role-session-name: NotifyApiGitHubActions aws-region: "us-east-1" - + - name: Login to ECR id: login-ecr uses: aws-actions/amazon-ecr-login@5a88a04c91d5c6f97aae0d9be790e64d9b1d47b7 # v1.7.1 @@ -56,43 +50,19 @@ jobs: -t $DOCKER_SLUG:${GITHUB_SHA::7} \ -t $DOCKER_SLUG:latest \ -f ci/Dockerfile . + - name: Publish run: | docker push $DOCKER_SLUG:latest && docker push $DOCKER_SLUG:${GITHUB_SHA::7} - - name: Configure credentials to Notify account using OIDC - uses: aws-actions/configure-aws-credentials@master - with: - role-to-assume: arn:aws:iam::239043911459:role/notification-api-apply - role-session-name: NotifyApiGitHubActions - aws-region: "ca-central-1" - - - name: Get Kubernetes configuration - run: | - aws eks --region $AWS_REGION update-kubeconfig --name notification-canada-ca-staging-eks-cluster --kubeconfig $HOME/.kube/config - - name: Update images in staging + - name: Rollout in Kubernetes run: | - kubectl set image deployment.apps/api api=$DOCKER_SLUG:${GITHUB_SHA::7} -n=notification-canada-ca --kubeconfig=$HOME/.kube/config - kubectl set image deployment.apps/celery-beat celery-beat=$DOCKER_SLUG:${GITHUB_SHA::7} -n=notification-canada-ca --kubeconfig=$HOME/.kube/config - kubectl set image deployment.apps/celery-sms celery-sms=$DOCKER_SLUG:${GITHUB_SHA::7} -n=notification-canada-ca --kubeconfig=$HOME/.kube/config - kubectl set image deployment.apps/celery-primary celery-primary=$DOCKER_SLUG:${GITHUB_SHA::7} -n=notification-canada-ca --kubeconfig=$HOME/.kube/config - kubectl set image deployment.apps/celery-scalable celery-scalable=$DOCKER_SLUG:${GITHUB_SHA::7} -n=notification-canada-ca --kubeconfig=$HOME/.kube/config - kubectl set image deployment.apps/celery-sms-send-primary celery-sms-send-primary=$DOCKER_SLUG:${GITHUB_SHA::7} -n=notification-canada-ca --kubeconfig=$HOME/.kube/config - kubectl set image deployment.apps/celery-sms-send-scalable celery-sms-send-scalable=$DOCKER_SLUG:${GITHUB_SHA::7} -n=notification-canada-ca --kubeconfig=$HOME/.kube/config - kubectl set image deployment.apps/celery-email-send-primary celery-email-send-primary=$DOCKER_SLUG:${GITHUB_SHA::7} -n=notification-canada-ca --kubeconfig=$HOME/.kube/config - kubectl set image deployment.apps/celery-email-send-scalable celery-email-send-scalable=$DOCKER_SLUG:${GITHUB_SHA::7} -n=notification-canada-ca --kubeconfig=$HOME/.kube/config - - - name: Restart deployments in staging - run: | - kubectl rollout restart deployment/api -n notification-canada-ca - kubectl rollout restart deployment/celery-beat -n notification-canada-ca - kubectl rollout restart deployment/celery-sms -n notification-canada-ca - kubectl rollout restart deployment/celery-primary -n notification-canada-ca - kubectl rollout restart deployment/celery-scalable -n notification-canada-ca - kubectl rollout restart deployment/celery-sms-send-primary -n notification-canada-ca - kubectl rollout restart deployment/celery-sms-send-scalable -n notification-canada-ca - kubectl rollout restart deployment/celery-email-send-primary -n notification-canada-ca - kubectl rollout restart deployment/celery-email-send-scalable -n notification-canada-ca + PAYLOAD={\"ref\":\"main\",\"inputs\":{\"docker_sha\":\"${GITHUB_SHA::7}\"}} + curl -L -X POST -H "Accept: application/vnd.github+json" \ + -H "Authorization: Bearer $WORKFLOW_PAT" \ + -H "X-GitHub-Api-Version: 2022-11-28" \ + https://api.github.com/repos/cds-snc/notification-manifests/actions/workflows/api-rollout-k8s-staging.yaml/dispatches \ + -d $PAYLOAD - name: my-app-install token id: notify-pr-bot @@ -118,3 +88,4 @@ jobs: run: | json="{'text':' CI is failing in !'}" curl -X POST -H 'Content-type: application/json' --data "$json" ${{ secrets.SLACK_WEBHOOK }} + From ad0546694ead9e477397fc1fb3a6e4934aeeac86 Mon Sep 17 00:00:00 2001 From: Steve Astels Date: Thu, 7 Mar 2024 15:33:31 -0500 Subject: [PATCH 09/10] Fix/restrict job query size (#2127) --- app/celery/nightly_tasks.py | 17 +++++++++-------- app/dao/jobs_dao.py | 15 +++++++++------ tests/app/dao/test_jobs_dao.py | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 14 deletions(-) diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py index c69e32018a..61a358fecc 100644 --- a/app/celery/nightly_tasks.py +++ b/app/celery/nightly_tasks.py @@ -49,14 +49,15 @@ def remove_letter_csv_files(): def _remove_csv_files(job_types): - jobs = dao_get_jobs_older_than_data_retention(notification_types=job_types) - current_app.logger.info("TEMP LOGGING: trying to remove {} jobs.".format(len(jobs))) - for job in jobs: - current_app.logger.info("TEMP LOGGING: trying to remove Job ID {} from s3.".format(job.id)) - s3.remove_job_from_s3(job.service_id, job.id) - current_app.logger.info("TEMP LOGGING: trying to archive Job ID {}".format(job.id)) - dao_archive_job(job) - current_app.logger.info("Job ID {} has been removed from s3.".format(job.id)) + while True: + jobs = dao_get_jobs_older_than_data_retention(notification_types=job_types, limit=20000) + if len(jobs) == 0: + break + current_app.logger.info("Archiving {} jobs.".format(len(jobs))) + for job in jobs: + s3.remove_job_from_s3(job.service_id, job.id) + dao_archive_job(job) + current_app.logger.info("Job ID {} has been removed from s3.".format(job.id)) @notify_celery.task(name="delete-sms-notifications") diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 3b945b15fc..ec3b80f1ae 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -129,7 +129,7 @@ def dao_update_job(job): db.session.commit() -def dao_get_jobs_older_than_data_retention(notification_types): +def dao_get_jobs_older_than_data_retention(notification_types, limit=None): flexible_data_retention = ServiceDataRetention.query.filter( ServiceDataRetention.notification_type.in_(notification_types) ).all() @@ -137,8 +137,7 @@ def dao_get_jobs_older_than_data_retention(notification_types): today = datetime.utcnow().date() for f in flexible_data_retention: end_date = today - timedelta(days=f.days_of_retention) - - jobs.extend( + query = ( Job.query.join(Template) .filter( func.coalesce(Job.scheduled_for, Job.created_at) < end_date, @@ -147,13 +146,15 @@ def dao_get_jobs_older_than_data_retention(notification_types): Job.service_id == f.service_id, ) .order_by(desc(Job.created_at)) - .all() ) + if limit: + query = query.limit(limit) + jobs.extend(query.all()) end_date = today - timedelta(days=7) for notification_type in notification_types: services_with_data_retention = [x.service_id for x in flexible_data_retention if x.notification_type == notification_type] - jobs.extend( + query = ( Job.query.join(Template) .filter( func.coalesce(Job.scheduled_for, Job.created_at) < end_date, @@ -162,8 +163,10 @@ def dao_get_jobs_older_than_data_retention(notification_types): Job.service_id.notin_(services_with_data_retention), ) .order_by(desc(Job.created_at)) - .all() ) + if limit: + query = query.limit(limit - len(jobs)) + jobs.extend(query.all()) return jobs diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 29e1c001f0..ebb8f50f01 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -17,6 +17,7 @@ dao_set_scheduled_jobs_to_pending, dao_update_job, ) +from app.dao.service_data_retention_dao import insert_service_data_retention from app.models import EMAIL_TYPE, LETTER_TYPE, SMS_TYPE, Job from tests.app.db import ( create_job, @@ -348,6 +349,38 @@ def test_should_get_jobs_seven_days_old_by_scheduled_for_date(sample_service): assert job_to_remain.id not in [job.id for job in jobs] +@freeze_time("2016-10-31 10:00:00") +def test_should_get_limited_number_of_jobs(sample_template): + flexable_retention_service = create_service(service_name="Another service") + insert_service_data_retention(flexable_retention_service.id, sample_template.template_type, 3) + flexable_template = create_template(flexable_retention_service, template_type=sample_template.template_type) + + eight_days_ago = datetime.utcnow() - timedelta(days=8) + four_days_ago = datetime.utcnow() - timedelta(days=4) + + create_job(flexable_template, created_at=four_days_ago) + create_job(flexable_template, created_at=four_days_ago) + create_job(sample_template, created_at=eight_days_ago) + create_job(sample_template, created_at=eight_days_ago) + + jobs = dao_get_jobs_older_than_data_retention(notification_types=[sample_template.template_type], limit=3) + + assert len(jobs) == 3 + + +@freeze_time("2016-10-31 10:00:00") +def test_should_get_not_get_limited_number_of_jobs_by_default(sample_template): + eight_days_ago = datetime.utcnow() - timedelta(days=8) + + create_job(sample_template, created_at=eight_days_ago) + create_job(sample_template, created_at=eight_days_ago) + create_job(sample_template, created_at=eight_days_ago) + + jobs = dao_get_jobs_older_than_data_retention(notification_types=[sample_template.template_type]) + + assert len(jobs) == 3 + + def assert_job_stat(job, result, sent, delivered, failed): assert result.job_id == job.id assert result.original_file_name == job.original_file_name From 00c44e33e565d5d6e54b76e264e861d78f16c9a0 Mon Sep 17 00:00:00 2001 From: Steve Astels Date: Mon, 11 Mar 2024 11:54:47 -0400 Subject: [PATCH 10/10] upgrade utils to 52.1.5 (#2131) --- poetry.lock | 22 +++++++++++----------- pyproject.toml | 2 +- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/poetry.lock b/poetry.lock index cd2b5ca1ba..1ab0346e40 100644 --- a/poetry.lock +++ b/poetry.lock @@ -331,13 +331,13 @@ uvloop = ["uvloop (>=0.15.2)"] [[package]] name = "bleach" -version = "6.0.0" +version = "6.1.0" description = "An easy safelist-based HTML-sanitizing tool." optional = false -python-versions = ">=3.7" +python-versions = ">=3.8" files = [ - {file = "bleach-6.0.0-py3-none-any.whl", hash = "sha256:33c16e3353dbd13028ab4799a0f89a83f113405c766e9c122df8a06f5b85b3f4"}, - {file = "bleach-6.0.0.tar.gz", hash = "sha256:1a1a85c1595e07d8db14c5f09f09e6433502c51c595970edc090551f0db99414"}, + {file = "bleach-6.1.0-py3-none-any.whl", hash = "sha256:3225f354cfc436b9789c66c4ee030194bee0568fbf9cbdad3bc8b5c26c5f12b6"}, + {file = "bleach-6.1.0.tar.gz", hash = "sha256:0a31f1837963c41d46bbf1331b8778e1308ea0791db03cc4e7357b97cf42a8fe"}, ] [package.dependencies] @@ -345,7 +345,7 @@ six = ">=1.9.0" webencodings = "*" [package.extras] -css = ["tinycss2 (>=1.1.0,<1.2)"] +css = ["tinycss2 (>=1.1.0,<1.3)"] [[package]] name = "blinker" @@ -2444,16 +2444,16 @@ requests = ">=2.0.0" [[package]] name = "notifications-utils" -version = "52.1.3" +version = "52.1.5" description = "Shared python code for Notification - Provides logging utils etc." optional = false -python-versions = "~3.10" +python-versions = "~3.10.9" files = [] develop = false [package.dependencies] awscli = "1.32.25" -bleach = "6.0.0" +bleach = "6.1.0" boto3 = "1.34.25" cachetools = "4.2.4" certifi = "^2023.7.22" @@ -2479,8 +2479,8 @@ werkzeug = "2.3.7" [package.source] type = "git" url = "https://github.com/cds-snc/notifier-utils.git" -reference = "52.1.3" -resolved_reference = "06a40db6286f525fe3551e029418458d33342592" +reference = "52.1.5" +resolved_reference = "9d9e8c7c32e3608f4dd8f320eaba4bb67edfcbf5" [[package]] name = "ordered-set" @@ -4255,4 +4255,4 @@ testing = ["coverage (>=5.0.3)", "zope.event", "zope.testing"] [metadata] lock-version = "2.0" python-versions = "~3.10.9" -content-hash = "f2bf5c58fe6d2689072e7b9d4cf91976e07e76ade98dc3153977c4377b98c86e" +content-hash = "f00992b7f47d8434a76d0be08135eace31315c696e20a222a10e2bf926e8a561" diff --git a/pyproject.toml b/pyproject.toml index 609ff16d41..db3ae6fff3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -64,7 +64,7 @@ Werkzeug = "2.3.7" MarkupSafe = "2.1.4" # REVIEW: v2 is using sha512 instead of sha1 by default (in v1) itsdangerous = "2.1.2" -notifications-utils = { git = "https://github.com/cds-snc/notifier-utils.git", tag = "52.1.3" } +notifications-utils = { git = "https://github.com/cds-snc/notifier-utils.git", tag = "52.1.5" } # rsa = "4.9 # awscli 1.22.38 depends on rsa<4.8 typing-extensions = "4.7.1" greenlet = "2.0.2"