Skip to content

Fix missing ownerReferences in pvc metadata#1837

Merged
sunsingerus merged 1 commit intoAltinity:0.25.5from
fkywong:fix-pvc-ownerReferences
Oct 13, 2025
Merged

Fix missing ownerReferences in pvc metadata#1837
sunsingerus merged 1 commit intoAltinity:0.25.5from
fkywong:fix-pvc-ownerReferences

Conversation

@fkywong
Copy link
Copy Markdown
Contributor

@fkywong fkywong commented Oct 2, 2025 •
edited by Slach
Loading

Problem

See issue #1799, #1843

Root Cause

By default, the Clickhouse Helm chart deploys the Keeper StatefulSet with PVCs provisioned by the Operator (chk.yaml:33), but the Operator does not set metadata.OwnerReferences on the PVCs.

Without metadata.OwnerReferences correctly set, ArgoCD can't attribute the PVCs to the application installation, and will incorrectly mark the PVCs as OutOfSync, which leads to the following undesirable behaviors/workarounds documented in the aforementioned issue.

The cluster instance's PVCs don't encounter the same issue because the Helm chart chi.yaml doesn't specify spec.defaults.storageManagement.provisioner, so the default StatefulSet provisioner is used:

clickhouse-operator/pkg/model/common/volume/volumer.go

Lines 60 to 61 in 732a1be

// Default value
return api.PVCProvisionerStatefulSet

Thus, ArgoCD is able to correctly map the StatefulSet-created PVCs to the CHI StatefulSet resources.

Solution: set metadata.OwnerReferences on PVCs

There's already precedence with doing this on Operator-provisioned StatefulSet and Service k8s resources:

clickhouse-operator/pkg/model/common/creator/stateful-set.go

Lines 35 to 41 in 732a1be

ObjectMeta: meta.ObjectMeta{
Name: c.nm.Name(interfaces.NameStatefulSet, host),
Namespace: host.GetRuntime().GetAddress().GetNamespace(),
Labels: c.macro.Scope(host).Map(c.tagger.Label(interfaces.LabelSTS, host)),
Annotations: c.macro.Scope(host).Map(c.tagger.Annotate(interfaces.AnnotateSTS, host)),
OwnerReferences: c.or.CreateOwnerReferences(c.cr),
},

clickhouse-operator/pkg/model/common/creator/service.go

Lines 81 to 85 in 732a1be

service.OwnerReferences = ownerReferences
// Combine labels and annotations
service.Labels = macro.Map(util.MergeStringMapsOverwrite(service.Labels, labels))
service.Annotations = macro.Map(util.MergeStringMapsOverwrite(service.Annotations, annotations))

This PR makes code changes in a similar manner to the above code snippets.

Pull Request checklist

  • All commits in the PR are squashed. More info
  • The PR is made into dedicated next-release branch, not into master branch1. More info
  • The PR is signed. More info

@sunsingerus sunsingerus added the planned for review This feature is planned for review label Oct 2, 2025
@dudekk dudekk mentioned this pull request Oct 7, 2025
Open
@sunsingerus sunsingerus merged commit 86bcb4b into Altinity:0.25.5 Oct 13, 2025
2 checks passed
@sunsingerus
Copy link
Copy Markdown
Collaborator

sunsingerus commented Oct 16, 2025

Crushes tests on PVC retain policy. Reverted ATM, will think what to do about it.

@sunsingerus sunsingerus added the work in progress This feautre is not completed yet label Oct 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sunsingerus sunsingerus sunsingerus approved these changes

Assignees

No one assigned

Labels

planned for review This feature is planned for review work in progress This feautre is not completed yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fkywong @sunsingerus