rbdmirror: enable rbd rados namespace mirroring #14701
Conversation
|
Two points still need suggestions,
|
5d9750f
to
2627fcb
Compare
58a1631
to
312677d
Compare
|
Testing: Disable |
312677d
to
d10c9fe
Compare
d0fa69f
to
a22b0f8
Compare
| } | ||
|
|
||
| // Schedule snapshots | ||
| if len(cephBlockPoolRadosNamespace.Spec.Mirroring.SnapshotSchedules) > 0 { |
There was a problem hiding this comment.
I have concerns about how this conditional interacts with the EnableSnapshotSchedules() call to removeSnapshotSchedules(). It seems to me that if a user removes the last snapshot schedule, Rook might not remove it.
Generally, operators can avoid many bugs by always processing logic regardless of whether lists have items or not.
There was a problem hiding this comment.
Make sense, We have all over in the code if c.poolSpec.Mirroring.SnapshotSchedulesEnabled() {
should we refactor it?
|
Should we rename all the functions paramaters where Currently used |
a22b0f8
to
0dfdb65
Compare
0dfdb65
to
a609d0e
Compare
3b98fef
to
d516e7b
Compare
|
Squash the commits? |
| !!! note | ||
| And Disabling the cephblockpool namespace would require to disable mirroring on all the | ||
| rados namespace present underneath. |
There was a problem hiding this comment.
I'm having trouble interpreting what this means, and I think users will also be confused. Can we add clarity by specifying the CRD names where appropriate? We could add field names also if needed.
I can make this suggestion, but TBH I am still confused about what the note means in terms of what actions I should take to disable mirroring.
| !!! note | |
| And Disabling the cephblockpool namespace would require to disable mirroring on all the | |
| rados namespace present underneath. | |
| !!! note | |
| Disabling the CephBlockPool namespace requires disabling mirroring on all | |
| CephBlockPoolRadosNamespaces present underneath. |
What does "Disabling the CephBlockPool namespace" mean? Should this be "Disabling mirroring for the CephBlockPool"? Something else?
I am also wondering whether users should be informed of the inverse. This talks about disabling the feature, but what about enabling the feature? Are there considerations there that need to be noted?
There was a problem hiding this comment.
No for enabling we dont need to specify it, that is not restricted or an exception
| - `remoteNamespace`: Name of the rados namespace on the peer cluster where the namespace should get mirrored. | ||
| - `snapshotSchedules`: schedule(s) snapshot at the **rados namespace** level. One or more schedules are supported. | ||
| - `interval`: frequency of the snapshots. The interval can be specified in days, hours, or minutes using d, h, m suffix respectively. | ||
| - `startTime`: optional, determines at what time the snapshot process starts, specified using the ISO 8601 time format. |
There was a problem hiding this comment.
Two things:
- An example interval and start time would be helpful for users.
- I don't see any note about required/optional on 3 of these fields, where that is noted above.
There was a problem hiding this comment.
An example interval and start time would be helpful for users.
I have explained in the example, under ### Mirroring
I don't see any note about required/optional on 3 of these fields, where that is noted above.
All fields are optional, not sure if we do that with any where else
| - `enabled`: whether mirroring is enabled on that rados namespace (default: false) | ||
| - `mode`: mirroring mode to run, possible values are "pool" or "image" (required). Refer to the [mirroring modes Ceph documentation](https://docs.ceph.com/docs/master/rbd/rbd-mirroring/#enable-mirroring) for more details | ||
| - `remoteNamespace`: Name of the rados namespace on the peer cluster where the namespace should get mirrored. | ||
| - `snapshotSchedules`: schedule(s) snapshot at the **rados namespace** level. One or more schedules are supported. |
There was a problem hiding this comment.
Something that is probably important is to note that this is an array/list.
Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md
Outdated
Show resolved
Hide resolved
d516e7b
to
1e23b27
Compare
Actually, if you go through the commits messages, |
1e23b27
to
104dc7a
Compare
Unit tests should never be a separate commit from functionality. No exception. IMO, commit 2 is sufficiently important for correctness of commit 1 so can be squashed. |
104dc7a
to
2a1ad11
Compare
Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md
Outdated
Show resolved
Hide resolved
Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md
Outdated
Show resolved
Hide resolved
2a1ad11
to
f1603d9
Compare
ca400e8
to
f2cffd2
Compare
Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md
Outdated
Show resolved
Hide resolved
| mode: image | ||
| # specify the schedule(s) on which snapshots should be taken | ||
| # snapshotSchedules: | ||
| # - interval: 24h # daily snapshots |
There was a problem hiding this comment.
What is the default snapshot schedule? Is it daily like this example? If so, how about uncommenting it? Even though it's the default, it's more readable that way in the example.
There was a problem hiding this comment.
Not sure about the default
f2cffd2
to
0dfbf94
Compare
0dfbf94
to
2f4eb30
Compare
Modify the CR to allow mirroring of an rados namespace to a differently named namespace on the remote cluster 1) enable rados namesapce mirroring only if the blockpool mirrroing is enabled 2) disable blockpool mirroing only if all the namesapce mirroing is disabled if the rbd mirroring fails and ceph version is not supported provide a error message with supported version details and reason of failing Signed-off-by: parth-gr <partharora1010@gmail.com>
2f4eb30
to
2cef47a
Compare
Modify the CR to allow mirroring of an rados namespace
to a differently named namespace on the remote cluster.
Tasks:
Pend work maybe future PR:
OPen question:
If we disable mirroring at cluster1 how we can notify to disable at cluster2 too
Checklist: