Skip to content
This repository has been archived by the owner on Dec 21, 2021. It is now read-only.

[DDO-1292] Rawls Liquibase Migration Pre-sync Job #387

Merged
merged 37 commits into from
Jun 15, 2021

Conversation

jack-r-warren
Copy link
Contributor

@jack-r-warren jack-r-warren commented Jun 7, 2021

Summary of DDO-1292:

  • New liquibase.properties file produced by firecloud-develop (DDO-1297)
  • K8s job (and related resources) are applied before rest of Rawls in ArgoCD sync, running Liquibase via the app's image before the app ever starts
    • Rationale: Now the app doesn't need to run Liquibase migrations on startup--the job runs to completion and kills its own CloudSQL proxy sidecar to let the ArgoCD sync continue (DDO-1301)
  • Values file enabling this lives over in terra-helmfile, that's where we'd make it do more than a dry-run

Copy link
Contributor

@mikeflinn mikeflinn left a comment

Choose a reason for hiding this comment

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

Awesome work @jack-r-warren! looks great overall. Just a few small questions and nits.

charts/rawls/templates/migration/job.yaml Outdated Show resolved Hide resolved
charts/rawls/templates/migration/job.yaml Show resolved Hide resolved
charts/rawls/templates/migration/role.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@choover-broad choover-broad left a comment

Choose a reason for hiding this comment

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

This is a seriously fantastic first Helm PR @jack-r-warren! 💯🥇

(Left a few comments inline, it may be that I'm just missing context)

charts/rawls/templates/migration/job.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,18 @@
{{- if .Values.migration.enabled }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need separate RBAC resources for this job? I think the permissions will be very similar to whatever the Rawls application uses. (Apologies if there's already been some background discussion here that I missed!)

Seems like we could add PreSync to the existing resources to make sure they exist when the job is run, but not delete them before every sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't strictly need separate RBAC, but we do need them to exist, and unfortunately: argoproj/argo-cd#3502 (comment)

Argo CD hooks are not considered part of Argo CD application resources, and features like drift detection and health assessments do not work. So the solution should involve not having to resort to adding "helm.sh/hook": pre-install to a Namespace resource.

Despite the clutter that this involves (#387 (comment)) I don't know of a better solution

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this comment offer a viable workaround? argoproj/argo-cd#3502 (comment)

I.e.

  • RBAC resources sync in wave -2
  • Job syncs in wave -1 with a Sync hook
  • Everything else syncs in default wave 0

If that won't work, I'm fine with duplicating the RBAC resources!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't seen that, that's a good idea. I'd talked to Mike and just now put the RBAC stuff behind a flag so the job can reuse the existing one. I think the only downside to leaning on sync waves is that the normal rbac resources would need to have that sort of migration-specific attribute, but that's a small downside imo. Let's chat at standup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This worked!

Copy link
Contributor

@choover-broad choover-broad left a comment

Choose a reason for hiding this comment

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

Nice, looks great to me!

The values allow us to enable this in dev only for an arbitrary length of time, right?

@jack-r-warren
Copy link
Contributor Author

Nice, looks great to me!

The values allow us to enable this in dev only for an arbitrary length of time, right?

Correct! Within each environment the values can control whether it is enabled (default false), whether it is dry run (default true), and whether failures will fail the sync (default true)

@jack-r-warren jack-r-warren merged commit 1bee360 into master Jun 15, 2021
@jack-r-warren jack-r-warren deleted the DDO-1292-liquibase-presync-job branch June 15, 2021 16:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants