pool/volume create/import/destroy for uzfs #43

vishnuitta · 2018-05-02T20:51:20Z

In uZFS, pool and volume creation/destroy and import paths are not complete and patchy.
Below are changes:

Bring up the uZFS engine without importing any pools, and, by reading any cache file if provided
Create uZFS and ZINFO related structures during pool import, volume creation
Hold the dataset during handshake of replica volume with target
clear dataset during destroy of pool or volume, and during export of pool
uZFS pool initialization done along with creation of txg threads
uZFS pool destroy done along with destroy of txg threads
Avoided above steps for snapshots
Removed test_export gtest code as those test cases are covered in script and other test cases. Otherwise, fixes are required here also due to change in the way target IP is made mandatory.
Along with stabilization of current testcases and travis, added:
IOs on volumes which are imported by pool import
CLI tests for creating volumes with targetIP option

Without target IP option for 'zfs create' command, volume will be created, but, uZFS and ZINFO related data structures won't be created.

Signed-off-by: Vishnu Itta <[email protected]>

jkryl

Nice changes! thanks Vitta

jkryl · 2018-05-07T08:15:01Z

cmd/uzfs_test/uzfs_test.c

@@ -654,14 +654,17 @@ open_pool(spa_t **spa)
 }

 void
-open_ds(spa_t *spa, zvol_state_t **zv)
+open_ds(spa_t *spa, char *ds, zvol_state_t **zv)


it is a mystery to me that the previous code was compilable without ds being declared as local var or passed as parameter. Could you explain?

there seems to be a global variable with same name.. :)

jkryl · 2018-05-07T08:15:27Z

cmd/uzfs_test/uzfs_test.c

 	int err;
 	err = uzfs_open_dataset(spa, ds, zv);
 	if (err != 0) {
 		printf("ds open errored.. %d\n", err);
 		exit(1);
 	}
+	(void) snprintf(name, sizeof (name), "%s/%s", spa_name(spa), ds);


I don't see name[] used anywhere.

true.. will remove it

jkryl · 2018-05-07T08:49:03Z

tests/cbtest/gtest/test_zrepl_prot.cc

 	}

 	/*
 	 * Send command to read data and read reply header. Reading payload is
 	 * left to the caller.
 	 */
-	void read_data_start(size_t offset, int len, zvol_io_hdr_t *hdr_inp)
+	void read_data_start(int m_data_fd, int &m_ioseq, size_t offset, int len,


Just if we can rename m_data_fd -> data_fd and m_ioseq -> ioseq (and the same change for write_data_start, write_data_and_verify_resp, write_two_chunks_and_verify_resp, read_data_and_verify_resp). The reason is that the "m_" prefix implies a class member variable, while in this case it is ordinary method parameter.

Thinking more about it. Let's move these methods completely out of the class and make functions from them. There was a value to have them as methods when they were accessing class member fields m_data_fd and m_io_seq, but since now we pass all context information to them through parameters, there is not a good reason why these methods should not be converted to utility functions outside of the class.

sure.. will remove "m_" prefix.. will also check if these can be moved out of class

jkryl · 2018-05-07T08:58:31Z

tests/cbtest/gtest/test_zrepl_prot.cc

+
+		do_handshake(m_zvol_name1, m_host1, m_port1, m_control_fd1,
+		    ZVOL_OP_STATUS_OK);
+		m_zrepl->kill();


I'm wondering why do we restart zrepl in setup hook. The purpose of this function is to set up test environment and not to test anything here. Besides that, we already have a test case for creating a zvol before and after restart if I remember correctly. So I would prefer the simplest code here, which is to create both zvols at once without restart.

other test case where zrepl is killed won't send IOs @jkryl .. I noticed a code issue while fixing some other issue and checked why this code was not catching it. Hence, added this test case.

jkryl · 2018-05-07T09:10:41Z

tests/cbtest/gtest/test_zrepl_prot.cc

-		}
+//		} catch (std::runtime_error re) {
+//			;
+//		}


I'm not sure if this is a good idea. But let's try it :-) In theory this command should never fail, but we know that even zpool destroy sometimes fails with EBUSY.

this being test code, idea is - lets see if this errors out and fix the reason for it

jkryl · 2018-05-07T09:19:19Z

cmd/zrepl/mgmt_conn.c

-			fprintf(stderr, "Unknown zvol: %s\n", zvol_name);
+		if (((zinfo = uzfs_zinfo_lookup(zvol_name)) == NULL) ||
+		    (zinfo->mgmt_conn != conn)) {
+			ZREPL_ERRLOG("Unknown zvol: %s\n", zvol_name);


let's avoid using ZREPL_ERRLOG in mgmt_conn.c file. syslog logging is going to be removed from zrepl in the future and I already cleaned mgmt_conn.c file from them. All messages here should be either error messages to stderr or debug messages using DBGCONN (which end up on stderr as well). This particular message should be fprintf(stderr, ...) since it is not a debug message.

ok.. I will change it.. one reason I changed this is - I'm not seeing the logs coming to /var/log/syslog.. where will these logs goes?

jkryl · 2018-05-07T09:20:36Z

cmd/zrepl/mgmt_conn.c

- * Try to obtain controller address from zfs property.
- */
-static int
-get_controller_ip(objset_t *os, char *buf, int len)


I remember I had to pull a few zfs header files because of this function to mgmt_conn.c. Most likely dmu* and dsl* header files are not needed anymore. Would be good if we can clean them up from this file as part of this change.

makes sense.. will do that

jkryl · 2018-05-07T09:24:42Z

cmd/zrepl/zrepl.c

-	printf("zrepl command args ... \nwhere 'command' is one of:\n\n");
-	printf("\t import <pool_name> [-t ip address)]\n");
-	printf("\t start [-t ip address)]\n");
-}


Now when we removed start and import subcommands and zrepl starts without any parameters, I have a question what is the expected way of importing a pool holding replicas by sidecar. Is it:

cmd/zrepl/zrepl # after zrepl is up and running (probably execute in loop with sleep 1): cmd/zpool/zpool import <name>

?

jkryl · 2018-05-07T09:26:03Z

.travis.yml

@@ -22,6 +22,8 @@ before_install:
    # packages for tests
    - sudo apt-get install --yes -qq parted lsscsi ksh attr acl nfs-kernel-server fio
    - sudo apt-get install --yes -qq libgtest-dev cmake
+    # packages for debugging
+    - sudo apt-get install gdb


nice, I assume that this is to help us debug problems using gdb on travis directly, correct?

right @jkryl .. after this, idea is to have a script that loads and prints stacktrace of a coredump

…s in gtest

vishnuitta · 2018-05-07T13:17:54Z

updated the review comments @jkryl .. let me know if this looks good

jkryl

Looks good to me. thanks!

satbirchhikara

Looks good to me sir.

* pool/volume create/import/destroy/export for uzfs * testcase to do IOs on imported pools * CLI tests for target IP address * owning dataset in uZFS during handshake with target Signed-off-by: Vishnu Itta <[email protected]>

vishnuitta added 9 commits May 3, 2018 02:11

pool/volume create/import/destroy for uzfs

7033434

Passing allocated buffer to taskq, reading IP from nvlist during create

aae77aa

cstyle fix

23567a1

removed creating directory /etc/zfs

30eef77

travis issue fix

d4286b2

moving a function under non kernel macro

856c03a

testcase to do IOs on imported pools

5699a59

Signed-off-by: Vishnu Itta <[email protected]>

Merge branch 'zfs-0.7-release' into import_issue

207a41c

CLI tests for IP address, export issue fix without target IP

acf1f35

vishnuitta requested review from gila, satbirchhikara and jkryl May 5, 2018 22:48

vishnuitta added 8 commits May 6, 2018 04:19

cstyle fix

fcf6ab5

removing gtest_export, using targetip in fio testcase

6ef3708

crash in rebuild and travis changes to get core

ac3f062

travis file fix

53eb267

travis file fix

afc39be

rebuild test hanging due to no targetip

bc4b17d

rebuild testcases fixes

e0ce4ff

checks to avoid rebuild on wrong volumes

1f4e0a3

jkryl reviewed May 7, 2018

View reviewed changes

removing unused variable, header file, and making methods as function…

e9c4bb2

…s in gtest

jkryl approved these changes May 8, 2018

View reviewed changes

vishnuitta added 3 commits May 8, 2018 15:29

Merge branch 'zfs-0.7-release' into import_issue

921ef73

Merge branch 'zfs-0.7-release' into import_issue

cae8885

Resolving conflicts with merge

2bf426f

satbirchhikara approved these changes May 9, 2018

View reviewed changes

vishnuitta merged commit 19a1f73 into mayadata-io:zfs-0.7-release May 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pool/volume create/import/destroy for uzfs #43

pool/volume create/import/destroy for uzfs #43

vishnuitta commented May 2, 2018 •

edited

Loading

jkryl left a comment

jkryl May 7, 2018

vishnuitta May 7, 2018

jkryl May 7, 2018

vishnuitta May 7, 2018

jkryl May 7, 2018

vishnuitta May 7, 2018

jkryl May 7, 2018

vishnuitta May 7, 2018 •

edited

Loading

jkryl May 7, 2018

vishnuitta May 7, 2018

jkryl May 7, 2018

vishnuitta May 7, 2018

jkryl May 7, 2018

vishnuitta May 7, 2018

jkryl May 7, 2018

vishnuitta May 7, 2018

jkryl May 7, 2018

vishnuitta May 7, 2018 •

edited

Loading

vishnuitta commented May 7, 2018

jkryl left a comment

satbirchhikara left a comment

pool/volume create/import/destroy for uzfs #43

pool/volume create/import/destroy for uzfs #43

Conversation

vishnuitta commented May 2, 2018 • edited Loading

jkryl left a 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 May 7, 2018 • edited Loading

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 May 7, 2018 • edited Loading

Choose a reason for hiding this comment

vishnuitta commented May 7, 2018

jkryl left a comment

Choose a reason for hiding this comment

satbirchhikara left a comment

Choose a reason for hiding this comment

vishnuitta commented May 2, 2018 •

edited

Loading

vishnuitta May 7, 2018 •

edited

Loading

vishnuitta May 7, 2018 •

edited

Loading