Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add back v1beta1 manifests #718

Merged
merged 1 commit into from
May 25, 2022
Merged

Add back v1beta1 manifests #718

merged 1 commit into from
May 25, 2022

Conversation

RaunakShah
Copy link
Contributor

@RaunakShah RaunakShah commented May 25, 2022 •
edited by xing-yang
Loading

What type of PR is this?
/kind api-change

What this PR does / why we need it:

This PR adds back the v1beta1 manifests. Generated files are not added back as part of this PR.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add VolumeSnapshot v1beta1 manifests back so that existing persisted v1beta1 objects are accessible via the new v1 API, however, VolumeSnapshot v1beta1 APIs are no longer served.
Action Item: Please update to VolumeSnapshot v1 APIs as soon as possible.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 25, 2022
@RaunakShah
Copy link
Contributor Author

/assign @xing-yang

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 25, 2022
RaunakShah
RaunakShah commented May 25, 2022
View reviewed changes
client/config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml Outdated
format: date-time
type: string
error:
description: error is the last observed error during snapshot creation, if any. This field could be helpful to upper level controllers(i.e., application controller) to decide whether they should continue on waiting for the snapshot to be created based on the type of error reported. The snapshot controller will keep retrying when an error occurrs during the snapshot creation. Upon success, this error field will be cleared.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we okay changing the spec here to pass the CI?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@xing-yang
Copy link
Collaborator

Can you change to "kind/api-change" and add a release note?

xing-yang
xing-yang reviewed May 25, 2022
View reviewed changes
client/hack/README.md Outdated
@@ -54,6 +54,8 @@ Once you run the script, you will get an output as follows:

## update-crd.sh

NOTE: We need to keep both v1beta1 and v1 snapshot APIs but set served and storage version at v1beta1 to false. Please copy back the v1beta1 manifest back to the files as this script will remove it.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at v1beta1 -> of v1beta1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@k8s-ci-robot 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 May 25, 2022
@RaunakShah RaunakShah force-pushed the revert-v1beta1-removal branch from e05486f to 5c47a8c Compare May 25, 2022 16:12
@gtxu gtxu mentioned this pull request May 25, 2022
Closed
@xing-yang xing-yang added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels May 25, 2022
@xing-yang
Copy link
Collaborator

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RaunakShah, xing-yang

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 25, 2022
@xing-yang xing-yang mentioned this pull request May 25, 2022
Merged
@k8s-ci-robot k8s-ci-robot merged commit 190f518 into kubernetes-csi:master May 25, 2022
@xing-yang xing-yang mentioned this pull request May 25, 2022
Merged
@Madhu-1
Copy link
Contributor

Madhu-1 commented May 26, 2022

@xing-yang @RaunakShah with this change can we still create a snapshot with v1beta1 with v6.0.1? or do we need to switch to v1 snapshots with v6.0.x

@RaunakShah RaunakShah mentioned this pull request May 26, 2022
Merged
@Madhu-1
Copy link
Contributor

Madhu-1 commented May 27, 2022

@RaunakShah any update on this one, am not able to create snapshots/snapshotclass with v1beta1 CRD.

@Madhu-1 Madhu-1 mentioned this pull request Jun 6, 2022
Merged
7 tasks
@mauriciopoppe
Copy link
Member

I'm out of the loop, why did we need to add the v1beta1 API again?

@xing-yang
Copy link
Collaborator

@mauriciopoppe We have to keep the v1beta1 APIs there because we can't guarantee that a customer's environment does not already have v1beta1 APIs before the upgrade. See discussions below on why we need to keep the old APIs:
kubernetes/kubernetes#101694 (comment)

@xing-yang
Copy link
Collaborator

Updated the release note to clarify this:

Add VolumeSnapshot v1beta1 manifests back so that existing persisted v1beta1 objects are accessible via the new v1 API, however, VolumeSnapshot v1beta1 APIs are no longer served.
Action Item: Please update to VolumeSnapshot v1 APIs as soon as possible.

@mauriciopoppe
Copy link
Member

I think we could also have a script that reads all the VolumeSnapshots with the v1 API and does a write to the API server with the same v1 API so that existing v1beta1 objects are stored as v1. With that we could stop storing the v1beta CRDs.

@xing-yang
Copy link
Collaborator

I think we could also have a script that reads all the VolumeSnapshots with the v1 API and does a write to the API server with the same v1 API so that existing v1beta1 objects are stored as v1. With that we could stop storing the v1beta CRDs.

Good idea. Do you want to work on this script?

@mauriciopoppe
Copy link
Member

I think we can discuss about this item on the next CSI Impl meeting, added an item for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xing-yang xing-yang xing-yang left review comments

@chrishenzie chrishenzie Awaiting requested review from chrishenzie

@humblec humblec Awaiting requested review from humblec

Assignees

@xing-yang xing-yang

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RaunakShah @xing-yang @k8s-ci-robot @Madhu-1 @mauriciopoppe