Skip to content

fix(mysql): Unifiy mysql connection logic passthrough - #327

Draft
casey-coreweave wants to merge 2 commits into
mainfrom
cabernathy/better_mysql
Draft

fix(mysql): Unifiy mysql connection logic passthrough#327
casey-coreweave wants to merge 2 commits into
mainfrom
cabernathy/better_mysql

Conversation

@casey-coreweave

Copy link
Copy Markdown
Contributor

Changes:

  • External and MOCO providers now produce a shared ConnectionMaterial and canonical per-instance Secret in [bundle.go]
  • URLs use safe escaping, IPv6 handling, explicit per-instance TLS paths, and currently emit tls=custom for compatibility with deployed Gorilla.
  • Applications and migration Jobs mount only their required TLS material and receive bundle rollout digests in [mysql_bundles.go]
  • Secret watches cover every external selector and MOCO credential Secret in [weightsandbiases_controller.go (line 139)]
  • Managed bootstrap now invokes mysql directly and idempotently creates wandb_local in [mysql.go (line 447)]
  • Managed lifecycle now handles cross-namespace MOCO resources, UID/instance scoping, detach/purge, retention metadata, credentials, PVCs, stale providers, and CR recreation safety.
  • Global MySQL CA mounting and MYSQL_CA_CERT_PATH emission were removed. Redis/global CA behavior remains intact.
  • External selector and duplicate managed-resource validation were added without changing the v2 schema.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9680a173-ddd8-43bf-b00e-0faae2d188f4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Match stale MOCO clusters by resource identity instead of the instance
fingerprint, purge instance storage by name so an unlabelled legacy cluster
never selects a sibling instance's PVCs, re-adopt clusters this deployment
detached, and cascade job deletions on bundle checksum changes.
@devin-ai-integration

Copy link
Copy Markdown

Addressed the review findings in 365dac3 (PR left in draft).

Blocking

  1. Stale purge no longer relies on deployment-wide labels: PurgeFinalizer now deletes the cluster's PVCs by Moco's <dataVolumeName>-moco-<cluster>-<n> naming, and reconcileStaleManagedMySQL passes an instance-scoped selector or labels.Nothing() when the legacy cluster carries no instance label — so an unlabelled leftover can't select a live sibling instance's storage.
  2. Stale detection is now keyed by the MOCO resource (namespace/name) each desired instance points at instead of InstanceID(key), so renaming an instance key that still points at the same cluster is a no-op.
  3. Added moco.ClearDetached: a cluster (and its my.cnf ConfigMap) carrying DetachedAnnotation is re-adopted when it is desired again and its WandbUIDLabel matches this CR, so a transient/undone detach is no longer permanent.
  4. Both bundle-checksum job deletions now use DeletePropagationBackground, via a shared deleteJobCascading helper, so the superseded pod can't race the recreated job's pod.

Minor

  • applyMySQLBundlesToWorkload now skips an instance whose status has no connection Secret yet instead of failing the whole reconcile.
  • ToMocoMySQLClusterSpec resolves the MySQL image per instance (falling back to default), matching the init job.
  • hack/scripts/verify-custom-ca-e2e.sh reads the per-instance bundle Secret from status.mysqlStatus.<instance>.connection.url, derives the CA path/volume from the URL, and no longer asserts the removed MYSQL_CA_CERT_PATH.

New tests cover the sibling-PVC survival, the renamed instance key, and re-adoption. make lint and make test pass locally.

Still open (deliberately unchanged, need your call)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant