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

[release-v1.43] Manual backport of 'Allow creating clones without source PVC (#2306)' #2367

Closed
wants to merge 1 commit into from
Closed

[release-v1.43] Manual backport of 'Allow creating clones without source PVC (#2306)' #2367

wants to merge 1 commit into from

Conversation

alromeros
Copy link
Collaborator

What this PR does / why we need it:

Manual backport of #2306

Release note:

NONE

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jul 21, 2022
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign aglitke after the PR has been reviewed.
You can assign the PR to them by writing /assign @aglitke in a comment when ready.

The full list of commands accepted by this bot can be found 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

@alromeros alromeros force-pushed the allow-cloning-without-source-v1.43 branch 2 times, most recently from 03fb068 to a8f75fa Compare July 21, 2022 13:55
@alromeros
Allow creating clones without source PVC (kubevirt#2306)
c6c7e85 
* Modify datavolume admission webhook to enable creating clones without source PVC

This commit modifies the datavolume admission webhook to follow a more descriptive approach, enabling the creation of clones without a source PVC.

This clone will later be handled by the datavolume-controller until the source PVC is created.

Signed-off-by: Alvaro Romero <alromero@redhat.com>

* Modify the datavolume-controller to improve the handling of clones without source

Since we are allowing the creation of clones without source PVC in the admission webhook, we need to improve the handling of these clones once in the datavolume-controller.

This commit modifies said controller, so we do proper error handling and validation until the source PVC is created.

Signed-off-by: Alvaro Romero <alromero@redhat.com>

* Add unit tests to check the creation of clones without source PVC

Signed-off-by: Alvaro Romero <alromero@redhat.com>

* Include a mechanism to reconcile clones without source once the source PVC is created

This commit introduces a new datavolume-controller watch so, if a clone without source is created, we make sure to reconcile it once a proper PVC is created.

It also updates/includes unit tests for proper coverage.

Signed-off-by: Alvaro Romero <alromero@redhat.com>

* Include functional tests to cover the creation of clones without source PVC

Signed-off-by: Alvaro Romero <alromero@redhat.com>

* Minor refactoring of clone-related code in DataVolume reconciler to improve readability

After enabling the creation of clones without source PVC in the datavolume controller, the clone-related logic outside its reconciler has increased in size and become sparse.

This commit rearranges all this code and condenses it into the clone reconciler, without changing the loop behavior.

Signed-off-by: Alvaro Romero <alromero@redhat.com>

* Modify datavolume-mutate webhook to reject the creation of clones if the source PVC's namespace doesn't exist

In previous commits, a mechanism to allow the creation of clones without source PVC was added, without ever checking if the source PVC's namespace exists or not.

This behavior could lead to permission conflicts between the user and the source's namespace since the webhook skipped all the related validation.

This commit modifies the datavolume-mutate admission webhook to reject the clone if the source PVC's namespace doesn't exist.

Signed-off-by: Alvaro Romero <alromero@redhat.com>

* Update unit tests for proper coverage of clone-validation functions

This commit adds and updates several unit tests to improve the coverage of the clone-validation mechanism after several functions were moved to the controller.

It also introduces minor changes on related code in the datavolume-controller and functional tests following PR review.

Signed-off-by: Alvaro Romero <alromero@redhat.com>
@alromeros alromeros force-pushed the allow-cloning-without-source-v1.43 branch from a8f75fa to c6c7e85 Compare July 21, 2022 14:37
@alromeros
Copy link
Collaborator Author

/retest-required

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Jul 26, 2022 •
edited
Loading

@alromeros: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-containerized-data-importer-e2e-ceph-gc c6c7e85 link true /test pull-containerized-data-importer-e2e-ceph-gc

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@brybacki
Copy link
Contributor

/retest-required

@brybacki
Copy link
Contributor

@alromeros did you have to do many manual changes to adapt the 2306 to this release?

@brybacki brybacki mentioned this pull request Aug 26, 2022
Merged
@alromeros
Copy link
Collaborator Author

@brybacki Yeah I had to adapt some newer clone-related code to the old datavolume controller. It wasn't a lot but it required some rework.

@alromeros
Copy link
Collaborator Author

/hold

As in #2366, holding since we have yet to reach a consensus on whether to backport this feature or not. The general idea seemed to be not doing it, which I'm not against, but I'll leave the PR open for some time just in case.

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 26, 2022
@kubevirt-bot
Copy link
Contributor

@alromeros: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2022
@alromeros
Copy link
Collaborator Author

Since #2405 was merged, this backport is no longer necessary. I'm closing the PR.

@alromeros alromeros closed this Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aglitke aglitke Awaiting requested review from aglitke

@awels awels Awaiting requested review from awels

Assignees
No one assigned
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alromeros @kubevirt-bot @brybacki