Avoid treating PVC managed by VolumeClaimTemplate as dependencies

[jklaw90]

What type of PR is this?
/kind bug

What this PR does / why we need it:
It was incorrectly handling PVCs when it belongs to the StatefulSet VolumeClaimTemplates.
If you set the claim name in the to match the StatefulSet VolumeClaimTemplate the StatefulSet controller would replace it for the pod, so the name isn't a backed by a real pvc.

create the following statefulset, and note the pod created has a claim with name www-web-0

apiVersion: apps/v1
kind: StatefulSet
metadata:
  name: web
spec:
  selector:
    matchLabels:
      app: nginx # has to match .spec.template.metadata.labels
  serviceName: "nginx"
  replicas: 3 # by default is 1
  minReadySeconds: 10 # by default is 0
  template:
    metadata:
      labels:
        app: nginx # has to match .spec.selector.matchLabels
    spec:
      volumes:
      - name: www
        persistentVolumeClaim:
          claimName: www
      terminationGracePeriodSeconds: 10
      containers:
      - name: nginx
        image: registry.k8s.io/nginx-slim:0.24
        ports:
        - containerPort: 80
          name: web
        volumeMounts:
        - name: www
          mountPath: /usr/share/nginx/html
  volumeClaimTemplates:
  - metadata:
      name: www
    spec:
      accessModes: [ "ReadWriteOnce" ]
      storageClassName: "my-storage-class"
      resources:
        requests:
          storage: 1Gi

Which issue(s) this PR fixes:
Fixes # n/a

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-controller-manager`: Ignored StatefulSet Dependencies with PVCs created via the VolumeClaimTemplates.

[karmada-bot]

Welcome @jklaw90! It looks like this is your first PR to karmada-io/karmada 🎉

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.

Project coverage is 35.21%. Comparing base (8cc712f) to head (902bb1b).

Files with missing lines	Patch %	Lines
...resourceinterpreter/default/native/dependencies.go	82.35%	2 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5568      +/-   ##
==========================================
+ Coverage   35.19%   35.21%   +0.02%     
==========================================
  Files         645      645              
  Lines       44869    44885      +16     
==========================================
+ Hits        15791    15808      +17     
  Misses      27846    27846              
+ Partials     1232     1231       -1     

Flag	Coverage Δ
unittests	35.21% <82.35%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[RainbowMango]

cc @XiShanYongYe-Chang for help

[XiShanYongYe-Chang]

Thanks @jklaw90
/assign

[XiShanYongYe-Chang]

Hi @jklaw90 Do you know how to get the correct pvc name from podTemplate?

[jklaw90]

Hi @jklaw90 Do you know how to get the correct pvc name from podTemplate?

Hi @XiShanYongYe-Chang
I don't think we can derive it from the podTemplate alone, we need the volumeClaimTemplates to determine the real name. The pvc name would be <template-claim-name>-<sts-name>-<index>.
That being said should the StatefulSet have dependencies or the pod in this case?

[XiShanYongYe-Chang]

You're right, we cannot obtain the PVC name directly from the StatefulSet, as it also needs to be combined with the pod's index. Therefore, the current method of obtaining the dependent PVC name is incorrect.

Provide the Pod's YAML information here:

apiVersion: v1
kind: Pod
metadata:
  generateName: web-
  labels:
    app: nginx
    apps.kubernetes.io/pod-index: "0"
    controller-revision-hash: web-59cfccf6c
    statefulset.kubernetes.io/pod-name: web-0
  name: web-0
  namespace: default
  ownerReferences:
  - apiVersion: apps/v1
    blockOwnerDeletion: true
    controller: true
    kind: StatefulSet
    name: web
    uid: 13e01c3d-4c7e-4d3a-8611-a0b41dd30be9
spec:
  containers:
  - image: registry.k8s.io/nginx-slim:0.24
    imagePullPolicy: IfNotPresent
    name: nginx
    ports:
    - containerPort: 80
      name: web
      protocol: TCP
    resources: {}
    terminationMessagePath: /dev/termination-log
    terminationMessagePolicy: File
    volumeMounts:
    - mountPath: /usr/share/nginx/html
      name: www
  dnsPolicy: ClusterFirst
  enableServiceLinks: true
  hostname: web-0
  preemptionPolicy: PreemptLowerPriority
  priority: 0
  restartPolicy: Always
  schedulerName: default-scheduler
  securityContext: {}
  serviceAccount: default
  serviceAccountName: default
  subdomain: nginx
  terminationGracePeriodSeconds: 10
  volumes:
  - name: www
    persistentVolumeClaim:
      claimName: www-web-0

cc @RainbowMango

[XiShanYongYe-Chang]

Hi @jklaw90 how are you currently obtaining the dependent PVC, or is it also not being obtained?

[jklaw90]

@XiShanYongYe-Chang

It's not being obtained currently. I assumed getPodDependencies would handle this for pods?

[XiShanYongYe-Chang]

The getPodDependencies function can serve to obtain the pod's dependent resources, but we will not be able to retrieve the associated PVC resources when distributing StatefulSets from the Karmada control plane.

[jklaw90]

We could use replicas and StatefulSet name with the claim template to derive pvc names.
But we have the possibility of missing some of the replicas were scaled down at all.

I think we'd have to query all the pvcs in the namespace to know with 100% certainty. (which is how i'm using it, with this function)

[XiShanYongYe-Chang]

For the sts resource, according to the template provided by you, when it is distributed to the member cluster, the pvc resource is automatically created. Therefore, there is no need to determine the pvc resource that the sts resource follows and distribute it.

Hi @jklaw90, this seems to depend on whether the .spec.volumeClaimTemplates field is defined in the template with sts. If I remove this filed in the sts, the generated pod will looks like this:

[jklaw90]

if you remove the template then that PVC should be a dependency.
so i think the current pr handles the cases you want.

[XiShanYongYe-Chang]

Should we execute different logic depending on whether .spec.volumeClaimTemplates exists or not?

[jklaw90]

Should we execute different logic depending on whether .spec.volumeClaimTemplates exists or not?

I might not understand the question. but i think the current code handles this, the first and new test should cover both cases.

[XiShanYongYe-Chang]

Oh @jklaw90 thanks, I get you, you are right.

[XiShanYongYe-Chang]

Thanks~
/lgtm
/cc @RainbowMango

[RainbowMango]

/retitle Avoid treating PVC managed by VolumeClaimTemplate as dependencies

[RainbowMango]

@jklaw90 Could you please squash your commits? Then things will be all set.

[jklaw90]

@RainbowMango i've been having some issues with chart-lint-test
It's not reporting the status, but i've also had it fail on some of the installation steps. seems unrelated to this pr.

[RainbowMango]

@RainbowMango i've been having some issues with chart-lint-test It's not reporting the status, but i've also had it fail on some of the installation steps. seems unrelated to this pr.

No worries. The test chart-lint-test will only run when there are changes in the content of the chart directory. See the configuration here.

[RainbowMango]

/lgtm
/approve

[karmada-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/resourceinterpreter/OWNERS [RainbowMango]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[XiShanYongYe-Chang]

Hi @jklaw90 @RainbowMango do you think we need to rebase this commit to the previous branch?

[RainbowMango]

I guess you mean if we need to backport this PR to release branches. I think we can do it although it's not a critical bug fix, as I don't see any risk with it.

[jklaw90]

yeah, next release is fine for us.