search

kubernetes-csi / external-snapshotter

Sidecar container that watches Kubernetes Snapshot CRD objects and triggers CreateSnapshot/DeleteSnapshot against a CSI endpoint.
Apache License 2.0
501 stars 374 forks source link

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

Closed leonardoce closed 2 weeks ago

leonardoce commented 1 month ago

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 commented 1 month ago

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 commented 4 weeks ago

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

leonardoce commented 4 weeks ago

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 commented 4 weeks ago

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

xing-yang commented 4 weeks ago

Can you add a release note?

xing-yang commented 4 weeks ago

/assign @jsafrane

leonardoce commented 3 weeks ago

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

xing-yang commented 3 weeks ago

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 commented 3 weeks ago

I did it @xing-yang. Thank you!

leonardoce commented 2 weeks ago

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

jsafrane commented 2 weeks ago

/lgtm

jsafrane commented 2 weeks ago

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

jsafrane commented 2 weeks ago

/approve

k8s-ci-robot commented 2 weeks ago

[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](https://github.com/kubernetes-csi/external-snapshotter/blob/master/OWNERS)~~ [jsafrane] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment