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 cg controllers for cephfs #1484

Merged
merged 9 commits into from
Jul 26, 2024
Merged

add cg controllers for cephfs #1484

merged 9 commits into from
Jul 26, 2024

Conversation

youhangwang
Copy link
Member

move pr #1401 to this one as the main branch have changed a lot due to upgrade kubebuilder

@youhangwang youhangwang mentioned this pull request Jul 9, 2024
Closed
BenamarMk
BenamarMk reviewed Jul 15, 2024
View reviewed changes
Copy link
Member

@BenamarMk BenamarMk left a comment

Choose a reason for hiding this comment

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

The initial review looks good. There are still many placeholders, which is acceptable for now. A detailed review will follow next.

.gitignore Outdated Show resolved Hide resolved
api/v1alpha1/replicationgroupsource_types.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
internal/controller/cephfscg/cghandler.go Outdated Show resolved Hide resolved
internal/controller/cephfscg/cghandler.go Outdated Show resolved Hide resolved
@BenamarMk
Copy link
Member

@youhangwang I completed the review of the source part. Overall, looks good to me. I didn't want to overwhelm the PR with nit comments for now. What is left is the destination part, which is more complicated. Then, I'll try to run it locally.

BenamarMk
BenamarMk reviewed Jul 22, 2024
View reviewed changes
Copy link
Member

@BenamarMk BenamarMk left a comment

Choose a reason for hiding this comment

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

The ReplicationDestination code looks good to me. The next step is to run it, validate its functionality, and observe its behavior.

cmd/main.go Outdated Show resolved Hide resolved
internal/controller/vrg_volsync.go Outdated Show resolved Hide resolved
@youhangwang
add cg controllers for cephfs
58d1e2a 
Signed-off-by: youhangwang <youhangwang@foxmail.com>
@youhangwang
fix lint
b4e1ad5 
Signed-off-by: youhangwang <youhangwang@foxmail.com>
@youhangwang
fix test
5bfcf57 
Signed-off-by: youhangwang <youhangwang@foxmail.com>
@youhangwang
fix comments
f24de47 
Signed-off-by: youhangwang <youhangwang@foxmail.com>
@youhangwang
remove Async check in NewVSCGHandler
e7a7c43 
Signed-off-by: youhangwang <youhangwang@foxmail.com>
@youhangwang
fix comments
9c9c171 
Signed-off-by: youhangwang <youhangwang@foxmail.com>
@youhangwang
fix make manifests
3e82303 
Signed-off-by: youhangwang <youhangwang@foxmail.com>
@ShyamsundarR
Copy link
Member

jFYI added a tracker issue for any pending items that would not be addressed in this PR w.r.t CG support in Ramen, #1501

@youhangwang
fix comments
774692f 
Signed-off-by: youhangwang <youhangwang@foxmail.com>
@youhangwang
fix lint
b8e7389 
Signed-off-by: youhangwang <youhangwang@foxmail.com>
@youhangwang
Copy link
Member Author

Manual tested the following use cases:

  • NO VolumeGroupSnapshot CRD, NO ReplicationGroupSource Created
  • NO VolumeGroupSnapshot CRD, ReplicationGroupSource Created
  • Volsync Disabled
  • Volsync Enabled
  • CG Disabled
  • CG Enabled

BenamarMk
BenamarMk approved these changes Jul 26, 2024
View reviewed changes
Copy link
Member

@BenamarMk BenamarMk left a comment

Choose a reason for hiding this comment

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

LGTM.

@BenamarMk
Copy link
Member

The CG-related code has been tested, and the happy path works in drenv. Some missing elements will be added to the tracker. We will merge it as is. The VolSync consistency group code will NOT execute unless the annotation ramendr.openshift.io/consistency-group is set in DRPC, which eventually propagates to the VRG.

@BenamarMk BenamarMk merged commit aefe73f into RamenDR:main Jul 26, 2024
16 checks passed
BenamarMk
BenamarMk reviewed Jul 26, 2024
View reviewed changes
internal/controller/vrg_volsync.go

var err error

cg, ok := rdSpec.ProtectedPVC.Labels[ConsistencyGroupLabel]
Copy link
Member

Choose a reason for hiding this comment

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

(nit) This line can be moved inside the if block

Copy link
Member Author

Choose a reason for hiding this comment

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

will change it in another pr once #1472 merged.

internal/controller/vrg_volsync.go
@@ -146,6 +167,33 @@ func (v *VRGInstance) reconcilePVCAsVolSyncPrimary(pvc corev1.PersistentVolumeCl
return true
}

cg, ok := pvc.Labels[ConsistencyGroupLabel]
Copy link
Member

Choose a reason for hiding this comment

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

(nit) Same here. This line can be moved inside the if block

internal/controller/vrg_volsync.go
rdinCGs := []ramendrv1alpha1.VolSyncReplicationDestinationSpec{}

for _, rdSpec := range v.instance.Spec.VolSync.RDSpec {
cg, ok := rdSpec.ProtectedPVC.Labels[ConsistencyGroupLabel]
Copy link
Member

Choose a reason for hiding this comment

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

and this

raghavendra-talur pushed a commit to raghavendra-talur/ramen that referenced this pull request Jul 30, 2024
@youhangwang @raghavendra-talur
add cg controllers for cephfs (RamenDR#1484)
a6b3720 
Add Consistency Group (CG) support for cephfs-related workload. The implementation follows the design
in this document RamenDR#1356.

Signed-off-by: youhangwang <youhangwang@foxmail.com>
(cherry picked from commit aefe73f)
ShyamsundarR added a commit to red-hat-storage/ramen that referenced this pull request Jul 30, 2024
@ShyamsundarR
Merge pull request #332 from raghavendra-talur/rtalur-backport-1484
e0ae26f 
Backport of upstream PR RamenDR#1484
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raghavendra-talur raghavendra-talur raghavendra-talur left review comments

@ShyamsundarR ShyamsundarR ShyamsundarR left review comments

@BenamarMk BenamarMk BenamarMk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@youhangwang @BenamarMk @ShyamsundarR @raghavendra-talur