Change MB expire strategy #39
Conversation
toshipp
force-pushed
the
mb-expire
branch
3 times, most recently
from
e50469b to
37b8a40
Compare
toshipp
force-pushed
the
mb-expire
branch
4 times, most recently
from
8a470fa to
39e6efe
Compare
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| backup, err := resMgr.CreateUniqueBackupFor(ctx, pvc, func(backup *mantlev1.MantleBackup) { | ||
| backup.Spec.Expire = "1d" |
There was a problem hiding this comment.
It's not necessary to set the last arg because this function set the default value of Expire field.
There was a problem hiding this comment.
I considered explicitly specifying the value makes the test more stable. If someone changes the default to 2d in testutil, this test will fail. But adding some duration to the previous value is more readable, IMO, I'll update this.
internal/ceph/util.go
Outdated
|
|
||
| var ErrSnapshotNotFound = errors.New("snapshot not found") | ||
|
|
||
| func IsRBDSnapshotNamed(snap string) func(RBDSnapshot) bool { |
There was a problem hiding this comment.
How about renaming snap to name? For instance, I believe s.Name == name is easier to understand than s.Name == snap.
| return fmt.Errorf("snap already exists: %s@%s", key, snap) | ||
| } | ||
|
|
||
| f.lastSnapId++ |
There was a problem hiding this comment.
This code implies we can't have a snapshot that id is 0 although the id of the first snapshot is 0 in Ceph. How about renaming lastSnapId to nextSnapId and moving this code after L57?
|
|
||
| By("checking removing the same snapshot fails") | ||
| err = f.RBDSnapRm("pool", "image", "snap1") | ||
| Expect(err).To(HaveOccurred()) |
There was a problem hiding this comment.
Could you also check RBDSnapLs returns only snap2 here?
|
@toshipp Although I'm in the middle of review, I left some comments about the first commit to "CephCmd in mantlebackup_controller". |
|
@toshipp Please resolve conflicts caused by merging the last PR. |
|
@satoru-takeuchi |
We designed MantleBackupConfig expires MantleBackup, but it was not good because replicated MantleBackups on the secondary will not be able to expired if the primary and the secondary are disconnected. To fix this, we introduce Expire on the MB itself. Signed-off-by: Toshikuni Fukaya <toshikuni-fukaya@cybozu.co.jp>
The expiration duration can now be given in the MB spec, set it according to MBC spec. Signed-off-by: Toshikuni Fukaya <toshikuni-fukaya@cybozu.co.jp>
Signed-off-by: Toshikuni Fukaya <toshikuni-fukaya@cybozu.co.jp>
MantleBackup can be created manually, so it's not necessary to inherit validation from MantleBackupConfig. However, there's no reason to change it, so we'll use the same validation. Signed-off-by: Toshikuni Fukaya <toshikuni-fukaya@cybozu.co.jp>
It intends to replace executeCommand in mantlebackup_controller. Signed-off-by: Toshikuni Fukaya <toshikuni-fukaya@cybozu.co.jp>
The previous code simulates the executeCommand function and sets static timestamps for RBD snapshots. This approach poses a problem for implementing expiration, as all MantleBackup resources would expire during testing if the timestamps become outdated. To address this, we need to create a more realistic fake CephCmd and use it. Signed-off-by: Toshikuni Fukaya <toshikuni-fukaya@cybozu.co.jp>
The expiration is done by reconciler if already expired. Otherwise schedule expiration by requesting GenericEvents instead of using requeue provided by controller-runtime. It is because we don't want to exit the reconciler to do other tasks. Signed-off-by: Toshikuni Fukaya <toshikuni-fukaya@cybozu.co.jp>
It should be also implemented in MB reconciler as well as expiration. Signed-off-by: Toshikuni Fukaya <toshikuni-fukaya@cybozu.co.jp>
The expiration is done by MB reconciler, delete the function from the sub command. Now the command name becomes backup. Signed-off-by: Toshikuni Fukaya <toshikuni-fukaya@cybozu.co.jp>
Signed-off-by: Toshikuni Fukaya <toshikuni-fukaya@cybozu.co.jp>
The struct is only used on the test and utilities using it can be replaced with the ceph package. Signed-off-by: Toshikuni Fukaya <toshikuni-fukaya@cybozu.co.jp>
|
@satoru-takeuchi |
We designed MBC to handle the expiration of backups, but it is not good because MBC does not delete secondary MB if the network connection is disconnected. To fix this, we add an expire field to MB.