add support for migrating postgres pvcs to hyperdisk #3653
Conversation
[static] Signed-off-by: Nicu Reut <nicu.reut@digitalasset.com>
|
Haven't looked at the code yet (will do a bit later when I'm back at the pc), but from your description it sounds like "migrating" has three value: true, false, and undefined? That sounds quite confusing, wouldn't it be better to give it three explicit values rather than using "undefined" as the third? Or at least not use boolean for the other two. |
No, it just defaults to false. Same for all the other flags, all default to false. |
| volumeMode: Filesystem | ||
| {{- with .Values.db.dataSource }} | ||
| dataSource: | ||
| {{- toYaml . | nindent 8 }} |
There was a problem hiding this comment.
you are a brave man... I would have done a helm test here just to be sure I counted the whitespaces right (I think I'd still soft recommend it)
There was a problem hiding this comment.
I did a full manual test to migrate a scratch
| const smallDiskSize = clusterSmallDisk ? '240Gi' : undefined; | ||
| const supportsHyperdisk = | ||
| hyperdiskSupportConfig.hyperdiskSupport.enabled && !useInfraAffinityAndTolerations; | ||
| let hyperdiskMigrationValues = {}; |
There was a problem hiding this comment.
any reason not do const hyperdiskMigrationValues = xyz ? { ... } : { } like we usually do?
There was a problem hiding this comment.
have to create the snapshot in the if
There was a problem hiding this comment.
ah I see; this sounds like a reason then...
want to add a brief comment? (nit)
There was a problem hiding this comment.
I don't see the value of the comment tbh, it would comment on the syntax of typescript not add any extra details about the why
There was a problem hiding this comment.
Actually, why can't you write this in functional style again?
Would something like this work?
const hyperdiskMigrationValues = (xyz ? (() => { new xyz; return {abc}) : (() => {}))()
(all of this is nit; I just stumbled about the inconsistency so figured other readers might too; we very rarely use mutable variables)
There was a problem hiding this comment.
We do use mutable variables in tyepscript when needed.
I find that the mutable state with the if is much cleaner to read compared to an anonymous function that gets called in the same line. It took me a few reads to figure out the nested complexity there.
| namespace: xns.logicalName, | ||
| }, | ||
| spec: { | ||
| volumeSnapshotClassName: 'dev-vsc', |
There was a problem hiding this comment.
offtopic, just ranting: that name...
There was a problem hiding this comment.
have no idea who created that, it's in all the clusters 🤷 not managed by us
martinflorian-da
left a comment
martinflorian-da
left a comment
There was a problem hiding this comment.
It looks scary but also somehow reasonable...
I did a full manual test to migrate a scratch
Well if you say so 👍
Thanks!
[static] Co-authored-by: Martin Florian <martin.florian@digitalasset.com> Signed-off-by: Nicu Reut <nicu.reut@digitalasset.com>
nicu-da
deleted the
nicuda/techdebt/support_postgres_migration_hyperdisk
branch
[static]
3 step migration process:
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines