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

Ceph: Create resources automatically for external Cluster #500

Merged
merged 4 commits into from May 22, 2020
Merged

Ceph: Create resources automatically for external Cluster #500

merged 4 commits into from May 22, 2020

Conversation

rajatsing
Copy link

@rajatsing rajatsing commented May 8, 2020 •
edited

The JSON BLOB will be passed from the UI and the appropriate resources will be created one by one.

This PR is also dependent on @aruniiird PR #505 . I will keep rebasing my PR until its merged.

Signed-off-by: RAJAT SINGH rajasing@redhat.com

@rajatsing rajatsing force-pushed the automatic-resource-creation branch from 3db9970 to 3eaa1f2 Compare May 8, 2020 09:17
@rajatsing rajatsing changed the title Ceph: Create resources automatically form secrets [WIP] Ceph: Create resources automatically form secrets May 8, 2020
@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 May 8, 2020
@rajatsing rajatsing force-pushed the automatic-resource-creation branch from 3eaa1f2 to 85813d2 Compare May 8, 2020 10:03
@rajatsing rajatsing changed the title [WIP] Ceph: Create resources automatically form secrets [WIP] Ceph: Create resources automatically May 8, 2020
@rajatsing rajatsing changed the title [WIP] Ceph: Create resources automatically [WIP] Ceph: Create resources automatically for external Cluster May 8, 2020
@rajatsing rajatsing force-pushed the automatic-resource-creation branch 6 times, most recently from f454cfd to d17fbba Compare May 11, 2020 09:09
@rajatsing rajatsing force-pushed the automatic-resource-creation branch from d17fbba to 8ce338c Compare May 11, 2020 12:59
@rajatsing rajatsing force-pushed the automatic-resource-creation branch 3 times, most recently from 5874597 to 81f0716 Compare May 12, 2020 17:27
@rajatsing
Copy link
Author

rajatsing commented May 12, 2020 •
edited

I have pulled changes from @aruniiird PR for OBC storageClass because I didn't want to wait to write some logic and get it inspected. Will keep rebasing it with mine.
@jarrpa can you please take a look?.
I have to just write the logic for CephFs and RBD storageclass creation and that's it. Can you please leave your thoughts?.
thanks

@rajatsing rajatsing force-pushed the automatic-resource-creation branch 2 times, most recently from 9670d55 to d1988c0 Compare May 12, 2020 18:17
@rajatsing rajatsing requested a review from leseb May 12, 2020 18:42
jarrpa
jarrpa requested changes May 12, 2020
View reviewed changes
Copy link
Member

@jarrpa jarrpa left a comment

Choose a reason for hiding this comment

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

A reasonable start. Still some logic to work out. Also, update the PR description to indicate you're now depending on two other PRs.

pkg/controller/storagecluster/initialization_reconciler.go Show resolved Hide resolved
pkg/controller/storagecluster/initialization_reconciler.go Outdated Show resolved Hide resolved
pkg/apis/ocs/v1/storagecluster_types.go Outdated Show resolved Hide resolved
pkg/apis/ocs/v1/storagecluster_types.go Outdated Show resolved Hide resolved
pkg/controller/storagecluster/initialization_reconciler.go Outdated Show resolved Hide resolved
pkg/controller/storagecluster/initialization_reconciler.go Outdated Show resolved Hide resolved
pkg/controller/storagecluster/reconcile.go Outdated Show resolved Hide resolved
@rajatsing rajatsing force-pushed the automatic-resource-creation branch from d1988c0 to 9c27181 Compare May 13, 2020 17:32
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2020
@rajatsing rajatsing force-pushed the automatic-resource-creation branch from 9f7531b to 4aee23c Compare May 20, 2020 15:36
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2020
jarrpa
jarrpa requested changes May 21, 2020
View reviewed changes
Copy link
Member

@jarrpa jarrpa left a comment

Choose a reason for hiding this comment

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

Thank you for starting the unit test! I've included a framework of what the test should look like, feel free to expand on it if you come across anything else that you think we can assert. There's also one aspect of commit cleanup that needs to be done.

pkg/apis/ocs/v1/storagecluster_types.go Outdated Show resolved Hide resolved
pkg/controller/storagecluster/initialization_reconciler.go Outdated Show resolved Hide resolved
pkg/controller/storagecluster/initialization_reconciler.go Show resolved Hide resolved
pkg/apis/ocs/v1/zz_generated.openapi.go Outdated Show resolved Hide resolved
pkg/controller/storagecluster/initialization_reconciler_test.go Outdated Show resolved Hide resolved
pkg/controller/storagecluster/initialization_reconciler_test.go Outdated Show resolved Hide resolved
pkg/controller/storagecluster/initialization_reconciler_test.go Outdated Show resolved Hide resolved
deploy/crds/ocs_v1_storagecluster_crd.yaml Outdated Show resolved Hide resolved
@rajatsing rajatsing force-pushed the automatic-resource-creation branch from 4aee23c to 098b974 Compare May 22, 2020 16:21
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2020
jarrpa
jarrpa requested changes May 22, 2020
View reviewed changes
Copy link
Member

@jarrpa jarrpa left a comment

Choose a reason for hiding this comment

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

Tests look good, thanks! One tiiiiiny nit, sorry. ;P

Now we just need to rebase and clean up the commits.

pkg/controller/storagecluster/initialization_reconciler_test.go Outdated Show resolved Hide resolved
@rajatsing rajatsing force-pushed the automatic-resource-creation branch 2 times, most recently from 44231ec to 2515be3 Compare May 22, 2020 20:35
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2020
@rajatsing rajatsing force-pushed the automatic-resource-creation branch from 2515be3 to dc8fe6c Compare May 22, 2020 21:08
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2020
jarrpa and others added 4 commits May 22, 2020 17:29
@jarrpa
Fix outdated CSV files
0966215 
A previous PR accidentally removed from data from the CSV. This commit
brings it all back.

Signed-off-by: Jose A. Rivera <jarrpa@redhat.com>
@jarrpa
Ceph: Create resources for external cluster
be86eed 
The JSON BLOB will be passed from the UI and appropiate resources will be created automatically for the external cluster.

Signed-off-by: RAJAT SINGH <rajasing@redhat.com>
@jarrpa
Unit tests for Resource Creation
aed37db 
Added tests for the EnsureExternalStorageClusterResources function.

Signed-off-by: RAJAT SINGH <rajasing@redhat.com>
@jarrpa
Ceph: CSV changes for external Secret
745a4b0 
Signed-off-by: RAJAT SINGH <rajasing@redhat.com>
@jarrpa jarrpa force-pushed the automatic-resource-creation branch from dc8fe6c to 745a4b0 Compare May 22, 2020 22:42
@jarrpa
Copy link
Member

jarrpa commented May 22, 2020

/lgtm
/override ci/prow/ocs-operator-e2e-aws

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

@jarrpa: Overrode contexts on behalf of jarrpa: ci/prow/ocs-operator-e2e-aws

In response to this:

/lgtm
/override ci/prow/ocs-operator-e2e-aws

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-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jarrpa, rajatsingh25aug

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

@jarrpa jarrpa added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 22, 2020
@openshift-ci-robot
Copy link

openshift-ci-robot commented May 22, 2020 •
edited

@rajatsingh25aug: 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 dc8fe6c link /test red-hat-storage-ocs-ci-e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@jarrpa
Copy link
Member

jarrpa commented May 22, 2020

The ocs-operator-ci failure looks to be a problem with the CI. Running the test locally succeeded, so overriding it.

/override ci/prow/ocs-operator-ci

@openshift-ci-robot
Copy link

@jarrpa: Overrode contexts on behalf of jarrpa: ci/prow/ocs-operator-ci

In response to this:

The ocs-operator-ci failure looks to be a problem with the CI. Running the test locally succeeded, so overriding it.

/override ci/prow/ocs-operator-ci

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.

@jarrpa jarrpa merged commit c60ec85 into red-hat-storage:master May 22, 2020
@rajatsing
Copy link
Author

Thanks all for this PR, Learned a lot 👯
cc @jarrpa @leseb @aruniiird

@rajatsing rajatsing deleted the automatic-resource-creation branch June 19, 2020 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leseb leseb leseb requested changes

@umangachapagain umangachapagain umangachapagain requested changes

@aruniiird aruniiird aruniiird requested changes

@jarrpa jarrpa jarrpa approved these changes

@davidvossel davidvossel Awaiting requested review from davidvossel

Assignees

@jarrpa jarrpa

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@rajatsing @umangachapagain @leseb @jarrpa @openshift-ci-robot @aruniiird