Skip to content

fix: Rework object store configs - #253

Merged
danielpanzella merged 6 commits into
v2from
danielpanzella/clickhouse-objectstore-fixes
Jul 15, 2026
Merged

fix: Rework object store configs#253
danielpanzella merged 6 commits into
v2from
danielpanzella/clickhouse-objectstore-fixes

Conversation

@danielpanzella

Copy link
Copy Markdown
Contributor

No description provided.

@danielpanzella
danielpanzella requested a review from a team as a code owner July 15, 2026 01:06
@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

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

Run ID: 8c6971f9-c107-40ef-a90e-6e5be021a29d

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
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch danielpanzella/clickhouse-objectstore-fixes

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.

danielpanzella and others added 4 commits July 14, 2026 22:58
Upgraded v1 deployments never carried force-path-style into the v2
object-store connection, so Bufstream rendered force_path_style: false
against S3-compatible endpoints (MinIO/SeaweedFS) and failed on
virtual-hosted DNS lookups.

- Derive it at three layers sharing one rule (any custom endpoint
  except CoreWeave): the v1 bucket-annotation drain, the canonical
  connection-secret writer, and Bufstream's reader fallback
- Honor explicit ?forcePathStyle=/?tls=/?region= overrides that v1
  configs carried in bucket.name/bucket.path
- Add ObjectStoreConnection.Path and wire v1 bucket.path through the
  drain, the canonical url, and Bufstream's storage URI
- Fix Bufstream GCS/Azure storage URIs (gs:// scheme, blob host derived
  from the storage account)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Upgraded v1 installs never had their legacy -bc deployments removed:
cleanupLegacyV1Deployments gates on every desired application reporting
ready, and the five Service-bearing apps never converged.

Two write loops kept them churning:
- The Application CRD schema defaults serviceTemplate.ports[].protocol,
  so the un-normalized template reconcileApplications rebuilt each pass
  never compared equal to the stored spec and the Application was
  Updated (generation bumped) on every reconcile.
- reconcileService copied raw template values over the live Service,
  clobbering API-server-defaulted fields (protocol, targetPort,
  sessionAffinity, type, internalTrafficPolicy) and empty annotations,
  so the update gate from #247 never engaged and each write re-queued
  the Application via the Owns(Service) watch.

Normalize ServiceTemplate ports at construction, preserve
server-defaulted/allocated Service fields the template leaves unset,
and log which apps block legacy cleanup instead of skipping silently.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The v1->v2 upgrade never deleted the legacy -bc deployments because the
cleanup gate read status.wandb.applications, a snapshot that froze at
the last convergence-driven reconcile: the parent's Owns(Application)
watch never fired (Applications carry plain owner refs, and Owns only
enqueues for a controller owner), and the terminal reconcile result
carries no requeue, so nothing refreshed the map after cutover. Apps
that were still mid-rollout at that moment stayed ready:false forever.

- Gate cleanupLegacyV1Deployments on live Deployment readiness
  (observedGeneration current, ReadyReplicas==Replicas>0) instead of
  the copied status map, and log which Deployments block cleanup
- Watch Applications with MatchEveryOwner so status changes enqueue the
  parent through the existing plain owner refs (controller refs would
  break namespaces hosting multiple CRs) and the status map stays fresh

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@danielpanzella
danielpanzella merged commit 481a3f9 into v2 Jul 15, 2026
7 of 8 checks passed
@danielpanzella
danielpanzella deleted the danielpanzella/clickhouse-objectstore-fixes branch July 15, 2026 16:10
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