feat(replica_controller): when delete v1 replica, call ReplicaRemove() #4266
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
davidcheng0922
force-pushed
the
issue-10294-disable-snapshot-hashing
branch
from
693bf92 to
b4c5ca9
Compare
derekbit
force-pushed
the
issue-10294-disable-snapshot-hashing
branch
2 times, most recently
from
449c1b2 to
b816dd5
Compare
davidcheng0922
force-pushed
the
issue-10294-disable-snapshot-hashing
branch
from
b816dd5 to
94f0076
Compare
derekbit
force-pushed
the
issue-10294-disable-snapshot-hashing
branch
2 times, most recently
from
1a1c016 to
80c86f7
Compare
|
@coderabbitai review |
derekbit
force-pushed
the
issue-10294-disable-snapshot-hashing
branch
from
80c86f7 to
2cfc6e4
Compare
✅ Actions performedReview triggered.
|
derekbit
force-pushed
the
issue-10294-disable-snapshot-hashing
branch
from
2cfc6e4 to
5bf4595
Compare
…move() Signed-off-by: David Cheng <davidcheng0922@gmail.com>
derekbit
force-pushed
the
issue-10294-disable-snapshot-hashing
branch
from
5bf4595 to
a1c874a
Compare
| engineClientProxy, err := rc.getEngineClientProxy(engine, engine.Status.CurrentImage) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Should close the connection
| engineClientProxy, err := rc.getEngineClientProxy(engine, engine.Status.CurrentImage) | |
| if err != nil { | |
| return err | |
| } | |
| engineClientProxy, err := rc.getEngineClientProxy(engine, engine.Status.CurrentImage) | |
| if err != nil { | |
| return errors.Wrapf(err, "failed to get the engine client proxy for ...) | |
| } | |
| defer engineClientProxy.Close() |
| if types.IsDataEngineV1(r.Spec.DataEngine) { | ||
| // remove replica from controller in v1 | ||
| // for v2, ReplicaRemove() equals to InstanceDelete() | ||
| // ReplicaRemove() will also cancel the related snapshot hash | ||
| if err := rc.ReplicaRemove(r); err != nil { | ||
| rc.logger.WithError(err).Warn("Failed to remove the replica") | ||
| } | ||
| } |
There was a problem hiding this comment.
Won't the ReplicaRemove conflict with the following c.InstanceDelete?
There was a problem hiding this comment.
Double check it
There was a problem hiding this comment.
I think it will not conflict
In v1
ReplicaRemove: remove replica from replicator backend, stop replica monitoring
InstanceDelete: process manager unregister replica process and release port
But I will re-test it after spdk upgrade issue sovled
2 participants
Which issue(s) this PR fixes:
longhorn/longhorn#10294
What this PR does / why we need it:
For v1 replica deleting, not only delete the instance and also remove the replica from the controller.
ReplicaRemovewill stop the related snapshot hashSpecial notes for your reviewer:
Additional documentation or context