-
Notifications
You must be signed in to change notification settings - Fork 58
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
Fixes issue when multiple VRGs conflict with the PVCs being protected #1535
base: main
Are you sure you want to change the base?
Changes from 3 commits
ed18630
e127357
ef385a5
6e45f89
025adc3
37aae20
b015270
48b3643
f8ed0e5
93481df
02ba28b
b511e77
b00e284
e198936
13e0c77
be4cd85
a3cf1e9
e907603
1ba302c
49fa212
5bddec4
0a467d6
4fb500d
88c742a
e1fc2e2
1530d0c
dcf5dba
9933744
871da2d
4e64b18
4e00bfc
8b7f6d0
a06fcf0
5e1bb80
b2e2615
fa145f6
40e9b01
005a0bc
40fe77c
7aa8ae4
18e3c21
41d0c76
f70cee5
fed1215
6f52e8f
ee98edb
756c2fb
61a078f
67e6cbc
9002195
940d012
20d5e90
89c2ddd
5d0e348
201b273
e63bcb3
2a39429
6035794
44584e5
af53a2a
8924dc7
c4f5f54
b12d40e
5246b03
28a5e87
d55abec
bb9fa7a
ee7617c
88da933
51d331a
5187293
9ac2f37
145065b
634d247
35e5d26
0a895a5
cad86be
44c1776
948ff76
46d441c
6fda9b2
88168ee
0ee14df
24e0133
0134dea
627ccc8
1204662
d17d112
fcdddfd
fb1d4fb
5fa81f5
99093c5
5661d38
a4d0b54
38654c6
8a70425
86f5d17
1c04e61
7e88713
5482b46
eede468
7043af7
5f9c5bb
77dc276
7049474
cd6cffb
3692dc5
c10641d
b911b9a
da3dc20
030adfa
79be579
9611e95
87c5a1d
62faf94
af60e2a
04f99b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
// SPDX-FileCopyrightText: The RamenDR authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package util | ||
|
||
import ( | ||
"sync" | ||
) | ||
|
||
// NamespaceLock implements atomic operation for namespace. It will have the namespace | ||
// having multiple vrgs in which VRGs are being processed. | ||
type NamespaceLock struct { | ||
namespace string | ||
mux sync.Mutex | ||
} | ||
|
||
// NewNamespaceLock returns new NamespaceLock | ||
func NewNamespaceLock() *NamespaceLock { | ||
return &NamespaceLock{} | ||
} | ||
|
||
// TryToAcquireLock tries to acquire the lock for processing VRG in a namespace having | ||
// multiple VRGs and returns true if successful. | ||
// If processing has already begun in the namespace, returns false. | ||
func (nl *NamespaceLock) TryToAcquireLock(namespace string) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @raghavendra-talur : Namespace arg is not required as it would be already acquired. |
||
nl.mux.Lock() | ||
defer nl.mux.Unlock() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @raghavendra-talur : This should be removed. |
||
|
||
if nl.namespace == namespace { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @raghavendra-talur : Handle this according to struct. |
||
return false | ||
} | ||
|
||
if nl.namespace == "" { | ||
nl.namespace = namespace | ||
} | ||
|
||
return true | ||
} | ||
|
||
// Release removes lock on the namespace | ||
func (nl *NamespaceLock) Release(namespace string) { | ||
nl.mux.Lock() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @raghavendra-talur : Remove lock line. |
||
defer nl.mux.Unlock() | ||
nl.namespace = "" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// SPDX-FileCopyrightText: The RamenDR authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package util_test | ||
|
||
import ( | ||
. "github.com/onsi/ginkgo/v2" | ||
. "github.com/onsi/gomega" | ||
"github.com/ramendr/ramen/internal/controller/util" | ||
) | ||
|
||
var _ = Describe("Testing Locks", func() { | ||
nsLock := util.NewNamespaceLock() | ||
Expect(nsLock.TryToAcquireLock("test")).To(BeTrue()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @raghavendra-talur : May have to use go func or anonymous functions. |
||
Expect(nsLock.TryToAcquireLock("test")).To(BeFalse()) | ||
nsLock.Release("test") | ||
Expect(nsLock.TryToAcquireLock("test")).To(BeTrue()) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,7 @@ type VolumeReplicationGroupReconciler struct { | |
kubeObjects kubeobjects.RequestsManager | ||
RateLimiter *workqueue.RateLimiter | ||
veleroCRsAreWatched bool | ||
locks *rmnutil.NamespaceLock | ||
} | ||
|
||
// SetupWithManager sets up the controller with the Manager. | ||
|
@@ -115,6 +116,8 @@ func (r *VolumeReplicationGroupReconciler) SetupWithManager( | |
r.Log.Info("Kube object protection disabled; don't watch kube objects requests") | ||
} | ||
|
||
r.locks = rmnutil.NewNamespaceLock() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pass the vrg ns as arg. |
||
|
||
return ctrlBuilder.Complete(r) | ||
} | ||
|
||
|
@@ -437,6 +440,12 @@ func (r *VolumeReplicationGroupReconciler) Reconcile(ctx context.Context, req ct | |
"Please install velero/oadp and restart the operator", v.instance.Namespace, v.instance.Name) | ||
} | ||
|
||
err = v.vrgParallelProcessingCheck(adminNamespaceVRG) | ||
|
||
if err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The err will not be not nil anytime. |
||
return ctrl.Result{Requeue: true}, err | ||
} | ||
|
||
v.volSyncHandler = volsync.NewVSHandler(ctx, r.Client, log, v.instance, | ||
v.instance.Spec.Async, cephFSCSIDriverNameOrDefault(v.ramenConfig), | ||
volSyncDestinationCopyMethodOrDefault(v.ramenConfig), adminNamespaceVRG) | ||
|
@@ -1626,3 +1635,32 @@ func (r *VolumeReplicationGroupReconciler) addKubeObjectsOwnsAndWatches(ctrlBuil | |
|
||
return ctrlBuilder | ||
} | ||
|
||
func (v *VRGInstance) vrgParallelProcessingCheck(adminNamespaceVRG bool) error { | ||
ns := v.instance.Namespace | ||
|
||
if !adminNamespaceVRG { | ||
vrgList := &ramendrv1alpha1.VolumeReplicationGroupList{} | ||
listOps := &client.ListOptions{ | ||
Namespace: ns, | ||
} | ||
|
||
err := v.reconciler.APIReader.List(context.Background(), vrgList, listOps) | ||
if err != nil { | ||
v.log.Error(err, "Unable to list the VRGs in the", " namespace ", ns) | ||
|
||
return err | ||
} | ||
|
||
// if the number of vrgs in the ns is more than 1, lock is needed. | ||
if len(vrgList.Items) > 1 { | ||
if isLockAcquired := v.reconciler.locks.TryToAcquireLock(ns); !isLockAcquired { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since TryToAcquireLock is a blocking call, not of response will not be helpful. Think on alternative. |
||
// Acquiring lock failed, VRG reconcile should be requeued | ||
return err | ||
} | ||
defer v.reconciler.locks.Release(ns) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The release of the locks should be done once PVC is verified for the ownership label. |
||
} | ||
} | ||
|
||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raghavendra-talur : Pass ns string and use for struct init.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just gave a thought on what we discussed. volumereplicationgroup_controller -- SetupWithManager (locks are initialized) r.locks = rmnutil.NewNamespaceLock() ns cannot be passed here as this call is well before Reconcile is invoked. So instead of string with ns, set of namespaces(locked) ones would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, it could be