Centralize VolumeGroupSnapshot members resource creation in the snapshot-controller #1171

leonardoce · 2024-10-24T06:42:04Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Previously, the csi-snapshotter was responsible for creating individual
VolumeSnapshots and VolumeSnapshotContents for each member when
dynamically provisioning a VolumeGroupSnapshot.

This commit relocates this logic to the snapshot-controller, bringing
all resource creation processes under a single authority.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Move the logic of creating individual VolumeSnapshot and VolumeSnapshotContent resources for dynamically created VolumeGroupSnapshot from csi-snapshotter sidecar to snapshot-controller.

k8s-ci-robot · 2024-10-24T06:42:07Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

xing-yang · 2024-10-24T15:04:41Z

Hi @leonardoce, can you please split this into 2 separate PRs, one depending on the other? Thanks!

leonardoce · 2024-10-24T15:06:52Z

Hi! Yes. This PR is composed by 2 commits, with the first being the dependent PR.

Do you want me to remove it? Without that it won't compile.

xing-yang · 2024-10-24T15:10:09Z

@leonardoce Never mind! You are right. They are already separate PRs.

pkg/common-controller/groupsnapshot_controller_helper.go

xing-yang · 2024-10-25T03:05:38Z

Can you add a release note?

xing-yang · 2024-10-25T03:05:46Z

/assign @jsafrane

deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml

leonardoce · 2024-10-28T07:45:53Z

Since #1169 is merged, I rebased over the latest master and marked it as ready for review.

xing-yang · 2024-10-29T03:03:59Z

Can you reword the release notes as follows?
Move the logic of creating individual VolumeSnapshot and VolumeSnapshotContent resources for dynamically created VolumeGroupSnapshot from csi-snapshotter sidecar to snapshot-controller.

leonardoce · 2024-10-29T06:39:02Z

I did it @xing-yang. Thank you!

jsafrane · 2024-11-01T15:58:03Z

pkg/common-controller/groupsnapshot_controller_helper.go

+			groupSnapshotContent.Name, err)
+	}
+
+	pvs, err := ctrl.client.CoreV1().PersistentVolumes().List(context.TODO(), metav1.ListOptions{})


All API server calls are expensive. List is a very expensive one. The controller should have a PV lister + indexer on spec.csi.driver + "^" + spec.csi.volumeHandle for fast lookups.
And use the lister to get PVs in the other parts of the controller.

Agree with you. In this PR, I'm just moving the existing logic. Should we address it here, in your opinion?

@jsafrane Can this be addressed in a followup PR? This way it will be easier to review.
I added this issue in the doc: https://docs.google.com/document/d/1jjliw0tFhCPnT9ixlYpj61k5u8_-awf1qSCWrkunshQ/edit?tab=t.0

I added the informer and the indexer in a separate commit, so we can decide whether to include it in this PR or in a separate one.

Xing asked me to put that on a separate PR. I'm on it.

pkg/common-controller/groupsnapshot_controller_helper.go

jsafrane · 2024-11-01T16:16:33Z

pkg/common-controller/groupsnapshot_controller_helper.go

+
+		pv := utils.GetPersistentVolumeFromHandle(pvs, groupSnapshotContent.Spec.Driver, volumeHandle)
+		if pv == nil {
+			klog.Errorf(


This is an interesting case. A snapshot exists, but its PV has been deleted. How useful is to have a VolumeSnapshot / VolumeSnapshotContent for it, if the user cannot possibly guess from which PVC the snapshot is?

My gut feeling would be to record also the source PVC + PV name in VolumeSnapshotHandlePairList to be able to reconstruct the VolumeSnapshotContent / VolumeSnapshot with a link to the PV / PVC

This should be discussed in the KEP to keep track of the decision.

We retrieve PV here to populate groupSnapshotContent.Status.PVVolumeSnapshotContentList and set volumeSnapshotContent.Spec.SourceVolumeMode.

The SourceVolumeMode will have an impact on applications that rely on changing volume mode when creating PVC from VolumeSnapshot. Without it being set, it falls back to the default behavior (for backward compatibility) which is not to check it.

Regarding PVVolumeSnapshotContentList, we have an action item to see if we can get rid of it after making all other changes.

I'm not sure if it is worth it to add source PVC and PV name in VolumeSnapshotHandlePairList. That sounds like the wrong place to store this info. We can discuss more about it.

Let me open an issue and we'll get back to this one later.

Issue opened: #1183

pkg/common-controller/groupsnapshot_controller_helper.go

…hot-controller Previously, the csi-snapshotter was responsible for creating individual VolumeSnapshots and VolumeSnapshotContents for each member when dynamically provisioning a VolumeGroupSnapshot. This commit relocates this logic to the snapshot-controller, bringing all resource creation processes under a single authority.

leonardoce · 2024-11-04T16:51:04Z

I rebased it on the latest master and now I'm filing a separate PR for the added informer and lister.

jsafrane · 2024-11-05T17:50:19Z

/lgtm

jsafrane · 2024-11-05T17:51:01Z

The amount of issues and subsequent PRs worries me a bit, but I can see it's moving towards a better controller.

jsafrane · 2024-11-05T17:51:19Z

/approve

k8s-ci-robot · 2024-11-05T17:51:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, leonardoce

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:

~~OWNERS~~ [jsafrane]

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

k8s-ci-robot requested review from gnufied and RaunakShah October 24, 2024 06:42

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 24, 2024

xing-yang reviewed Oct 25, 2024

View reviewed changes

pkg/common-controller/groupsnapshot_controller_helper.go Outdated Show resolved Hide resolved

pkg/common-controller/groupsnapshot_controller_helper.go Outdated Show resolved Hide resolved

pkg/common-controller/groupsnapshot_controller_helper.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned jsafrane Oct 25, 2024

leonardoce force-pushed the move-logic branch from 6cff7f8 to 09d37ae Compare October 25, 2024 08:19

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 25, 2024

xing-yang reviewed Oct 25, 2024

View reviewed changes

deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml Show resolved Hide resolved

leonardoce force-pushed the move-logic branch 2 times, most recently from aaba439 to e0b5871 Compare October 28, 2024 07:44

leonardoce marked this pull request as ready for review October 28, 2024 07:44

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 28, 2024

k8s-ci-robot requested a review from chrishenzie October 28, 2024 07:44

leonardoce mentioned this pull request Oct 29, 2024

Set PVC name on VolumeGroupSnapshot members #1177

Merged

jsafrane reviewed Nov 1, 2024

View reviewed changes

leonardoce force-pushed the move-logic branch from e0b5871 to 414ce46 Compare November 2, 2024 10:09

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 2, 2024

leonardoce force-pushed the move-logic branch 5 times, most recently from 91cff49 to 6208785 Compare November 4, 2024 16:46

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 4, 2024

leonardoce force-pushed the move-logic branch from 6208785 to 86b7ce1 Compare November 4, 2024 16:50

leonardoce mentioned this pull request Nov 4, 2024

Add PV informer and indexer #1182

Merged

xing-yang mentioned this pull request Nov 4, 2024

Need to discuss how to handle the case when a PV is deleted while creating a VolumeGroupSnapshot #1183

Open

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 5, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2024

k8s-ci-robot merged commit be73f8a into kubernetes-csi:master Nov 5, 2024
8 checks passed

nixpanic mentioned this pull request Nov 11, 2024

Pass context on in GroupSnapshot sync functions #1193

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralize VolumeGroupSnapshot members resource creation in the snapshot-controller #1171

Centralize VolumeGroupSnapshot members resource creation in the snapshot-controller #1171

leonardoce commented Oct 24, 2024 •

edited

Loading

k8s-ci-robot commented Oct 24, 2024

xing-yang commented Oct 24, 2024

leonardoce commented Oct 24, 2024

xing-yang commented Oct 24, 2024

xing-yang commented Oct 25, 2024

xing-yang commented Oct 25, 2024

leonardoce commented Oct 28, 2024

xing-yang commented Oct 29, 2024

leonardoce commented Oct 29, 2024

jsafrane Nov 1, 2024

leonardoce Nov 1, 2024

xing-yang Nov 1, 2024

leonardoce Nov 2, 2024

leonardoce Nov 4, 2024

jsafrane Nov 1, 2024

xing-yang Nov 1, 2024 •

edited

Loading

xing-yang Nov 4, 2024

xing-yang Nov 4, 2024

leonardoce commented Nov 4, 2024

jsafrane commented Nov 5, 2024

jsafrane commented Nov 5, 2024

jsafrane commented Nov 5, 2024

k8s-ci-robot commented Nov 5, 2024

Centralize VolumeGroupSnapshot members resource creation in the snapshot-controller #1171

Centralize VolumeGroupSnapshot members resource creation in the snapshot-controller #1171

Conversation

leonardoce commented Oct 24, 2024 • edited Loading

k8s-ci-robot commented Oct 24, 2024

xing-yang commented Oct 24, 2024

leonardoce commented Oct 24, 2024

xing-yang commented Oct 24, 2024

xing-yang commented Oct 25, 2024

xing-yang commented Oct 25, 2024

leonardoce commented Oct 28, 2024

xing-yang commented Oct 29, 2024

leonardoce commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xing-yang Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardoce commented Nov 4, 2024

jsafrane commented Nov 5, 2024

jsafrane commented Nov 5, 2024

jsafrane commented Nov 5, 2024

k8s-ci-robot commented Nov 5, 2024

leonardoce commented Oct 24, 2024 •

edited

Loading

xing-yang Nov 1, 2024 •

edited

Loading