[TC57] Unit test to verify deletion of rebuild_snap once rebuild finished #200

mynktl · 2019-02-16T06:32:25Z

Changes:

Unit-test to verify deletion of rebuild_snap once dataset becomes healthy.
Handling of rebuild_snap deletion in timer_thread.
- To track the stale clone during runtime, I have introduced a new variable stale_clone_exist in zvol_rebuild_info_t. stale_clone_exist is protected by rebuild_mtx of zvol_state_t.
- Once uzfs_zvol_rebuild_scanner completes the rebuilding process fully, it will try to delete the stale clone. If this deletion fails then it will set stale_clone_exist to 1.
- timer_thread will periodically check for the stale clone (if zv->rebuild_info.stale_clone_exists != 0). If stale clone found on any dataset then timer_thread will try to delete the rebuild_clone and rebuild_snap. Upon successful removal of rebuild_clone and rebuild_snap, timer_thread will update stale_clone_exist variable with value 0.

Signed-off-by: mayank mayank.patel@cloudbyte.com

…shed Signed-off-by: mayank <mayank.patel@cloudbyte.com>

vishnuitta · 2019-02-17T13:48:47Z

lib/libzpool/zrepl_mgmt.c

 	if (ret != 0) {
 		LOG_ERR("Rebuild_clone snap destroy failed on:%s"
-		    " with err:%d", clone_zv->zv_name, ret);
+		    " with err:%d", zv->zv_name, ret);
 	}

 	uzfs_zvol_release_internal_clone(zv, snap_zv, clone_zv);


if clone_zv is NULL, there can be panic in this release_internal_clone.

vishnuitta · 2019-02-17T13:49:18Z

lib/libzpool/zrepl_mgmt.c

@@ -545,10 +545,10 @@ uzfs_zvol_destroy_snapshot_clone(zvol_state_t *zv, zvol_state_t *snap_zv,
 	    clone_zv->zv_name, clonename, zv->zv_name);

 	/* Destroy clone's snapshot */
-	ret = uzfs_destroy_internal_all_snap(clone_zv);
+	ret = uzfs_destroy_all_internal_snapshots(clone_zv);
 	if (ret != 0) {


I think we should break if ret is nonzero. Any reason why we are not breaking from here?

I meant - return if ret is nonzero

yes. it should return to caller if uzfs_destroy_all_internal_snapshots return non-zero.

vishnuitta · 2019-02-17T13:58:50Z

lib/libzpool/zrepl_mgmt.c

@@ -545,10 +545,10 @@ uzfs_zvol_destroy_snapshot_clone(zvol_state_t *zv, zvol_state_t *snap_zv,
 	    clone_zv->zv_name, clonename, zv->zv_name);


here as well, we are using clone_zv and snap_zv.. so, these two can't be NULL

yes. In this function, there are no checks regarding non-null of clone and snap zv. Error checking should be added to this function. I will update this function with error checking of all zv.

vishnuitta · 2019-02-17T14:51:14Z

lib/libzpool/zrepl_mgmt.c

+	    REBUILD_SNAPSHOT_CLONENAME);
+
+	ret = uzfs_open_dataset(zv->zv_spa, clone_subname, &l_clone_zv);
+	if (ret == 0) {


we should handle error here. Any reason why we are not handling error?

No. I will update this.

vishnuitta · 2019-02-17T15:07:29Z

lib/libzpool/zrepl_mgmt.c

+ * cloned volume and backing snapshot.
+ */
+int
+uzfs_zinfo_destroy_stale_clone(zvol_info_t *zinfo)


if open_dataset or hold_dataset of clone fails, its better to retry next time by releasing snap_zv and returning error.

vishnuitta · 2019-02-17T23:27:35Z

lib/libzpool/zrepl_mgmt.c

 	if (ret != 0) {
 		LOG_ERR("Rebuild_clone snap destroy failed on:%s"
-		    " with err:%d", clone_zv->zv_name, ret);
+		    " with err:%d", zv->zv_name, ret);
 	}

 	uzfs_zvol_release_internal_clone(zv, snap_zv, clone_zv);


all the callers of uzfs_zvol_release_internal_clone are setting snapshot_zv and clone_zv to NULL. So, release need to happen. Lets add this as comment for later usage.

even if all snapshots deletion fails, we need to release and exit.

Signed-off-by: mayank <mayank.patel@cloudbyte.com>

vishnuitta · 2019-02-18T08:50:58Z

lib/libzpool/zrepl_mgmt.c

+	    REBUILD_SNAPSHOT_CLONENAME);
+
+	ret = uzfs_open_dataset(zv->zv_spa, clone_subname, &l_clone_zv);
+	if (ret == 0) {


'else' of this?

Signed-off-by: mayank <mayank.patel@cloudbyte.com>

vishnuitta · 2019-02-18T16:07:05Z

lib/libzrepl/data_conn.c

+#endif
+
+	if (wquiesce) {
+		if (uzfs_zinfo_destroy_internal_clone(zinfo)) {


in else, better to set the stale_clone_exist to 0

vishnuitta

changes are good

vishnuitta · 2019-02-18T16:39:18Z

one minor comment to reset the flag to zero on successful deletion of snap/clone in dw_replica

Signed-off-by: mayank <mayank.patel@cloudbyte.com>

codecov-io · 2019-02-18T19:29:15Z

Codecov Report

Merging #200 into zfs-0.7-release will increase coverage by 0.23%.
The diff coverage is 77.5%.

@@                 Coverage Diff                 @@
##           zfs-0.7-release     #200      +/-   ##
===================================================
+ Coverage            52.09%   52.33%   +0.23%     
===================================================
  Files                  240      240              
  Lines                78204    78236      +32     
===================================================
+ Hits                 40742    40946     +204     
+ Misses               37462    37290     -172

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5ee05b...8bfe56d. Read the comment docs.

vishnuitta

changes are good

… cases (mayadata-io#200) Signed-off-by: mayank <mayank.patel@cloudbyte.com>

* [DE249]fix(zrepl): destroying all internal snapshots of rebuild clone dataset once rebuild completes (#199) * [TC57] fix(stale clone): delete stale volume in timer fn and its test cases (#200) * fix(memleak): freeing string in error case in uzfs_zinfo_destroy_stale_clone (#201) Signed-off-by: mayank <mayank.patel@cloudbyte.com>

[TC57] Unit test to verify deletion of rebuild_snap once rebuild fini…

9e1db47

…shed Signed-off-by: mayank <mayank.patel@cloudbyte.com>

mynktl requested review from pawanpraka1, vishnuitta and satbirchhikara February 16, 2019 06:32

vishnuitta reviewed Feb 17, 2019

View reviewed changes

Addressing review comments

f7746da

Signed-off-by: mayank <mayank.patel@cloudbyte.com>

vishnuitta reviewed Feb 18, 2019

View reviewed changes

mayank added 2 commits February 18, 2019 16:11

Addressing review comments

8bf467b

Signed-off-by: mayank <mayank.patel@cloudbyte.com>

Fixing non-debug build

a9c7829

Signed-off-by: mayank <mayank.patel@cloudbyte.com>

vishnuitta reviewed Feb 18, 2019

View reviewed changes

vishnuitta previously approved these changes Feb 18, 2019

View reviewed changes

Addressing review comments

8bfe56d

Signed-off-by: mayank <mayank.patel@cloudbyte.com>

mynktl dismissed vishnuitta’s stale review via 8bfe56d February 18, 2019 18:44

vishnuitta approved these changes Feb 19, 2019

View reviewed changes

vishnuitta merged commit 10610ef into mayadata-io:zfs-0.7-release Feb 19, 2019

mynktl added a commit to mynktl/cstor that referenced this pull request Feb 19, 2019

[TC57] fix(stale clone): delete stale volume in timer fn and its test…

c61df58

… cases (mayadata-io#200) Signed-off-by: mayank <mayank.patel@cloudbyte.com>

mynktl mentioned this pull request Feb 19, 2019

cherrypick(v0.8.x): Deletion of stale clone dataset #202

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TC57] Unit test to verify deletion of rebuild_snap once rebuild finished #200

[TC57] Unit test to verify deletion of rebuild_snap once rebuild finished #200

mynktl commented Feb 16, 2019 •

edited

vishnuitta Feb 17, 2019

mynktl Feb 18, 2019

vishnuitta Feb 17, 2019

vishnuitta Feb 17, 2019

mynktl Feb 18, 2019

vishnuitta Feb 17, 2019

mynktl Feb 18, 2019

vishnuitta Feb 17, 2019

mynktl Feb 18, 2019

vishnuitta Feb 17, 2019

mynktl Feb 18, 2019

vishnuitta Feb 17, 2019

vishnuitta Feb 17, 2019

mynktl Feb 18, 2019

vishnuitta Feb 18, 2019

mynktl Feb 18, 2019

vishnuitta Feb 18, 2019

mynktl Feb 18, 2019

vishnuitta left a comment

vishnuitta commented Feb 18, 2019

codecov-io commented Feb 18, 2019

vishnuitta left a comment

		@@ -545,10 +545,10 @@ uzfs_zvol_destroy_snapshot_clone(zvol_state_t zv, zvol_state_t snap_zv,
		clone_zv->zv_name, clonename, zv->zv_name);

[TC57] Unit test to verify deletion of rebuild_snap once rebuild finished #200

[TC57] Unit test to verify deletion of rebuild_snap once rebuild finished #200

Conversation

mynktl commented Feb 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishnuitta left a comment

Choose a reason for hiding this comment

vishnuitta commented Feb 18, 2019

codecov-io commented Feb 18, 2019

Codecov Report

vishnuitta left a comment

Choose a reason for hiding this comment

mynktl commented Feb 16, 2019 •

edited