-
Notifications
You must be signed in to change notification settings - Fork 20
feat: make hub statefulset work by stripping some properties from generated PVCs #347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: make hub statefulset work by stripping some properties from generated PVCs #347
Conversation
Signed-off-by: Wei Weng <Wei.Weng@microsoft.com>
Signed-off-by: Wei Weng <Wei.Weng@microsoft.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| } | ||
| // Use customizedPlacementStatusUpdatedActual with resourceIsTrackable=false | ||
| // because Jobs don't have availability tracking like Deployments/DaemonSets do | ||
| // because Jobs and PVCs don't have availability tracking like Deployments/DaemonSets do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PVC has conditions, I wonder why it's not trackable? However, do we really want to place PVC if they are the by-product of a statefulSet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added logic to track availability of PVC in availability tracker
I don't think we should place PVC if they are the by-product of a statefulset
I originally wanted to not propagate PVC but realized that stateful set controller doesn't set owner reference on the PVC so cannot reliably tell whether a PVC is created because of a statefulset (unless I infer from the name)
The hub PVCs look like this during the hub workload e2e test
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
annotations:
pv.kubernetes.io/bind-completed: "yes"
pv.kubernetes.io/bound-by-controller: "yes"
volume.beta.kubernetes.io/storage-provisioner: rancher.io/local-path
volume.kubernetes.io/selected-node: hub-control-plane
volume.kubernetes.io/storage-provisioner: rancher.io/local-path
creationTimestamp: "2025-11-26T17:02:01Z"
finalizers:
- kubernetes.io/pvc-protection
labels:
app: test-ss
name: test-ss-pvc-test-ss-0
namespace: application-1
resourceVersion: "2328"
uid: 1bab1a0e-d24d-44c6-bbbb-8a1f64a70e5c
spec:
accessModes:
- ReadWriteOnce
resources:
requests:
storage: 100Mi
storageClassName: standard
volumeMode: Filesystem
volumeName: pvc-1bab1a0e-d24d-44c6-bbbb-8a1f64a70e5c
status:
accessModes:
- ReadWriteOnce
capacity:
storage: 100Mi
phase: Bound
---
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
annotations:
pv.kubernetes.io/bind-completed: "yes"
pv.kubernetes.io/bound-by-controller: "yes"
volume.beta.kubernetes.io/storage-provisioner: rancher.io/local-path
volume.kubernetes.io/selected-node: hub-control-plane
volume.kubernetes.io/storage-provisioner: rancher.io/local-path
creationTimestamp: "2025-11-26T17:02:08Z"
finalizers:
- kubernetes.io/pvc-protection
labels:
app: test-ss
name: test-ss-pvc-test-ss-1
namespace: application-1
resourceVersion: "2424"
uid: 6a89a3cd-7a26-47eb-82bf-af929a1d2f8b
spec:
accessModes:
- ReadWriteOnce
resources:
requests:
storage: 100Mi
storageClassName: standard
volumeMode: Filesystem
volumeName: pvc-6a89a3cd-7a26-47eb-82bf-af929a1d2f8b
status:
accessModes:
- ReadWriteOnce
capacity:
storage: 100Mi
phase: BoundSigned-off-by: Wei Weng <Wei.Weng@microsoft.com>
Signed-off-by: Wei Weng <Wei.Weng@microsoft.com>
Signed-off-by: Wei Weng <Wei.Weng@microsoft.com>
| // Check if the PVC is bound. | ||
| // A PVC is considered available when it's in the Bound phase, meaning it has been | ||
| // successfully bound to a PersistentVolume and is ready to be used by pods. | ||
| if pvc.Status.Phase == corev1.ClaimBound { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the StorageClass uses the WaitForFirstConsumer volume binding mode. The PVC will bind only when a Pod attempts to use it.
this seems to be a bit tricky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way is to check the storage class using a client as well to tell the difference between VolumeBindingWaitForFirstConsumer vs others and if VolumeBindingWaitForFirstConsumer, treat pending as available as well
But since we are okay with not even propagating PVCs. That is a simpler option
| Kind: "PersistentVolumeClaim", | ||
| Name: fmt.Sprintf("%s-%s-%d", testStatefulSet.Spec.VolumeClaimTemplates[0].Name, testStatefulSet.Name, 1), | ||
| Namespace: workNamespace.Name, | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to the PVs created by the PVC? Do we ever place PV?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the controller on the member cluster will create PVCs for the statefulSet that we place on the member cluster too. From the look of it, their name would be the same (deterministic) but this can lead to problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the PVs are cluster scoped resources so they are not selected by the CRP
…make-hub-statefulset-work
Signed-off-by: Wei Weng <Wei.Weng@microsoft.com>
46cd732 to
540fe6f
Compare
Signed-off-by: Wei Weng <Wei.Weng@microsoft.com>
Description of your changes
hub stateful set doesn't work when propagated to members because some properties were not stripped from generated PVCs
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer