-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
core: disable mirroring on pool with "image" mode #13905
Conversation
805e257
to
02a2961
Compare
6719ab0
to
2ddf7ff
Compare
5fb0d1b
to
d6eef01
Compare
89ce927
to
7f431f4
Compare
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.
multi cluster mirroring ci test is failing
Still testing it with the CI. So added a wait:
|
f46a100
to
d3ffd23
Compare
d3ffd23
to
59cb485
Compare
pkg/daemon/ceph/client/pool.go
Outdated
return errors.Wrapf(err, "failed to remove cluster peer with UUID %q for the pool %q", peer.UUID, pool.Name) | ||
} | ||
|
||
for _, image := range images { |
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.
There could be a lot of images. I wonder if we need to clean this up in a job, similar to how it's proposed for force deletion of the subvolume groups.
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.
made changes to disable mirroring on only those images were it was enabled.
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.
Since we still don't have the force deletion of the subvolume PR yet, I propose that disabling mirroring for images in a job (if required), can be taken up once the other PR is ready.
81f49fd
to
b3d0a5c
Compare
1d81deb
to
d3caa25
Compare
2900443
to
800b9c1
Compare
800b9c1
to
c835b8a
Compare
c835b8a
to
186d8b6
Compare
186d8b6
to
9c98803
Compare
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.
LGTM
i forgot one case where we want to allow users to remove unwanted(dead) cluster mirroring peers. |
Only a few minor questions remain, removing review to unblock after others approve
Currently, if mirroring is disabled by the customer, then we remove all the peers on the pool (provided that the mirroring on the pool images is disabled). We don't have the automation to only remove specific/unwanted/dead peers as of now. But that looks like a valid use case and is not very difficult to implement. We can discuss more about this and maybe take it up in a future PR. |
9c98803
to
b56fda1
Compare
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.
LGTM
For image mode mirroring, if cephBlockPool.Pool.Spec.Mirroring.Enable is set to false, then remove the peer cluster and disable mirroring on all the pool if the user has disabled mirroring on all the pool images. If mirroring is not disabled on all the pool images, then reconcile will fail asking the users to manually disable mirroring on those images. Signed-off-by: sp98 <sapillai@redhat.com>
b56fda1
to
a37c476
Compare
return errors.Wrapf(err, "failed to list mirrored images for pool %q", pool.Name) | ||
} | ||
|
||
if len(*mirroredPools.Images) > 0 { |
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.
if we always need to disable it, do the code after this ever hit,
Or the flow is,
Disable mirroring -> disable mirroring manually on images-> the re-start reconcile
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.
For image
mode mirroring, we only want to disable it only when mirroring is disable on all the images on the pool.
len(*mirroredPools.Images) > 0
would mean that there are still images in this pool with mirroring enabled. So we will return the error with this message. Reconcile will keep on failing until the users manually disable the mirroring on each image on this pool.
For
image
mode mirroring, ifcephBlockPool.Pool.Spec.Mirroring.Enable
is set to false, then remove the peer cluster and disable mirroring on all the pool if the user has disabled mirroring on all the pool images.if mirroring is not disabled on all the pool images, then reconcile will fail asking the users to manually disable mirroring on those images.
Some initial Testing:
Checklist: