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.

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

Enabling 'snapshotter' feature in external cluster mode #832

Conversation

aruniiird
Copy link
Contributor

A very minor change to enable Snapshotter feature in external cluster mode.

Signed-off-by: Arun Kumar Mohan amohan@redhat.com

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 14, 2020
@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 14, 2020
@openshift-ci-robot
Copy link

Hi @aruniiird. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@aruniiird
Copy link
Contributor Author

Adding a WIP PR, as the changes are yet to be tested...

CC: @Madhu-1 , @humblec

@humblec
Copy link
Contributor

humblec commented Oct 21, 2020

@aruniiird I have a dump question: Isnt it applicable for both internal and external mode ? because we are adding the resource in the reconcile function which is equally applicable for both. Am I missing anything here?

Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>
@aruniiird aruniiird force-pushed the snapshotter-for-external-cluster branch from 38d82af to 37fec69 Compare October 23, 2020 10:42
@aruniiird aruniiird changed the title [WIP] Enabling 'snapshotter' feature in external cluster mode Enabling 'snapshotter' feature in external cluster mode Oct 23, 2020
@openshift-ci-robot openshift-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 23, 2020
@aruniiird
Copy link
Contributor Author

Removing the WIP tag, as the feature is working on ceph version which supports ceph fs subvolume info command.
We used the following ceph version,

ceph versions
{
    "mon": {
        "ceph version 14.2.8-111.el8cp (2e6029d57bc594eceba4751373da6505028c2650) nautilus (stable)": 3
    },
    "mgr": {
        "ceph version 14.2.8-111.el8cp (2e6029d57bc594eceba4751373da6505028c2650) nautilus (stable)": 3
    },
    "osd": {
        "ceph version 14.2.8-111.el8cp (2e6029d57bc594eceba4751373da6505028c2650) nautilus (stable)": 3
    },
    "mds": {
        "ceph version 14.2.8-111.el8cp (2e6029d57bc594eceba4751373da6505028c2650) nautilus (stable)": 3
    },
    "rgw": {
        "ceph version 14.2.8-111.el8cp (2e6029d57bc594eceba4751373da6505028c2650) nautilus (stable)": 3
    },
    "overall": {
        "ceph version 14.2.8-111.el8cp (2e6029d57bc594eceba4751373da6505028c2650) nautilus (stable)": 15
    }
}

@jarrpa , can you please take a look...

Attaching the screen shots

RBD Snapshot

external-mode-rbd-snapshot

CephFS Snapshot

external-mode-cephfs-snapshot

@aruniiird
Copy link
Contributor Author

@aruniiird I have a dumb question: Isn't it applicable for both internal and external modes? because we are adding the resource in the reconcile function which is equally applicable for both. Am I missing anything here?

@humblec , the code path taken for converged/internal mode and external mode cluster are different. You can see it here. So I have to add the ensureSnapshotClasses method to external mode as well.
You are right, the snapshotter feature should be applicable for both modes, but on the initial PR we added it only to the internal mode.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2020
@openshift-ci-robot
Copy link

@iamniting: changing LGTM is restricted to collaborators

In response to this:

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.

@openshift-merge-robot
Copy link
Contributor

@aruniiird: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/red-hat-storage-ocs-ci-e2e-aws 37fec69 link /test red-hat-storage-ocs-ci-e2e-aws

Full PR test history. Your PR dashboard.

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.

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, jarrpa, raghavendra-talur

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

@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 23, 2020
@openshift-merge-robot openshift-merge-robot merged commit cdd969a into red-hat-storage:master Oct 23, 2020
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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants