Skip to content

Add CSI Cloning enhancement.#35

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
huffmanca:csi-cloning
Oct 1, 2019
Merged

Add CSI Cloning enhancement.#35
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
huffmanca:csi-cloning

Conversation

@huffmanca
Copy link
Copy Markdown
Contributor

@huffmanca huffmanca commented Sep 20, 2019

Includes the enhancement file for including CSI Cloning. This is largely based on the upstream KEP.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 20, 2019
@huffmanca
Copy link
Copy Markdown
Contributor Author

@tsmetana , @gnufied , @jsafrane ,

Looking for review and feedback on this enhancement request.

Copy link
Copy Markdown
Contributor

@jsafrane jsafrane left a comment

Choose a reason for hiding this comment

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

I personally think that OpenShift enhancements should not cover upstream work. We should link upstream KEPs and documentation, but copying them here is IMO not useful.

We should cover work we're going to do to enable cloning in OpenShift, so:

  • We as engineering know what to do (we need rebase of this and that, we need this e2e test).
  • QA knows what is in scope of the feature and what is out of scope, so they can test it.
  • Documentation team can find upstream docs.

The comments below are trying to move the enhancement in this direction and are valid only if the direction itself is correct :-).

Comment thread storage/csi-cloning.md Outdated
Comment thread storage/csi-cloning.md Outdated
Comment thread storage/csi-cloning.md Outdated
Comment thread storage/csi-cloning.md Outdated
### User Stories [optional]

#### Story 1
As a cluster user, I want to easily test changes to my production data base without risking issues to my customer facing applications
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should write stories for us, i.e. "as OCS developer, I want to release my CSI driver with cloning support so ...".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've removed the previous stories and created a single one for us.

At present, due to the limited audience and availability, I've only included the single story.

Comment thread storage/csi-cloning.md
Comment thread storage/csi-cloning.md
Comment thread storage/csi-cloning.md

## Design Details

### Test Plan
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should mention we want e2e tests with Red Hat's CSI sidecars, maybe link openshift/origin#23560 and https://jira.coreos.com/browse/STOR-223

Link to usptream e2e tests could be useful so we know what we want to run and QA (or someone) can check that we actually completed the goal.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've updated this section to include both of the links above. It also includes a link to where I believe CSI cloning is tested, and the README on defining additional CSI Drivers for testing.

@huffmanca huffmanca force-pushed the csi-cloning branch 2 times, most recently from 011a240 to 29c13f1 Compare September 25, 2019 19:17
@huffmanca
Copy link
Copy Markdown
Contributor Author

@jsafrane ,

Thank you for the in-depth review. I've updated the PR to better reflect the usage for OCP, and less the upstream proposal. This PR should now be ready for rereview.

Copy link
Copy Markdown
Contributor

@jsafrane jsafrane left a comment

Choose a reason for hiding this comment

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

I have only couple of minor comments

Comment thread storage/csi-cloning.md Outdated
Comment thread storage/csi-cloning.md Outdated
Comment thread storage/csi-cloning.md
@jsafrane
Copy link
Copy Markdown
Contributor

lgtm
@openshift/storage anyone else want to take a look?

Comment thread storage/csi-cloning.md Outdated
@bertinatto
Copy link
Copy Markdown
Member

LGTM too, only a minor question.

Comment thread storage/csi-cloning.md Outdated
@bertinatto
Copy link
Copy Markdown
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2019
@bertinatto
Copy link
Copy Markdown
Member

/assign @knobunc
for approval

@knobunc
Copy link
Copy Markdown
Contributor

knobunc commented Oct 1, 2019

/approve

@openshift-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, huffmanca, knobunc

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

The pull request process is described here

Details 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2019
@openshift-merge-robot openshift-merge-robot merged commit 75be422 into openshift:master Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants