Skip to contentSkip to conversation

cman: improve cman/qdisk interactions

PATCH New Fabio M. Di Nitto 3 participants 4 messages Sep 7, 2011 – Sep 8, 2011

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 cman: improve cman/qdisk interactions

- libcman/cman: add new quorum API call to update name and votes of a quorum device
- cman: simplify common code to free quorum_device infrastructure and handle quorum recalculation
- cman: do better logging/error reports of the quorum API usage
- cman: use strdup instead of malloc+strcpy (code is more readable)
- libcman: perform better error checking in register_quorum_device/update_quorum_device

- Allow qdisk to update device name in cman using a new libcman quorum API call
- Perform slight better error checking of some update opertaions

Resolves: rhbz#735917

---
 cman/daemon/cnxman-socket.h |    1 +
 cman/daemon/commands.c      |  138 +++++++++++++++++++++++++++++++++----------
 cman/lib/libcman.c          |   19 +++++-
 cman/lib/libcman.h          |    4 +
 cman/qdisk/main.c           |   28 ++++++++-
 5 files changed, 153 insertions(+), 37 deletions(-)
cman/daemon/cnxman-socket.h +1 -0
index e8b7378..d243b40 100644
@@ -32,6 +32,7 @@
 #define CMAN_CMD_REG_QUORUMDEV      0x800000b5
 #define CMAN_CMD_UNREG_QUORUMDEV    0x800000b6
 #define CMAN_CMD_POLL_QUORUMDEV     0x800000b7
+#define CMAN_CMD_UPDATE_QUORUMDEV   0x800000b8
 #define CMAN_CMD_TRY_SHUTDOWN       0x800000bb
 #define CMAN_CMD_SHUTDOWN_REPLY     0x000000bc
 #define CMAN_CMD_UPDATE_FENCE_INFO  0x800000bd
cman/daemon/commands.c +107 -31

192 lines not on this page · raw message

cman/lib/libcman.c +15 -4
index daaad07..a89c731 100644
@@ -1002,14 +1002,15 @@ int cman_replyto_shutdown(cman_handle_t handle, int yesno)
 	return 0;
 }
 
-
-int cman_register_quorum_device(cman_handle_t handle, char *name, int votes)
+static int cman_set_quorum_device(cman_handle_t handle,
+				     int ops,
+				     char *name, int votes)
 {
 	struct cman_handle *h = (struct cman_handle *)handle;
 	char buf[strlen(name)+1 + sizeof(int)];
 	VALIDATE_HANDLE(h);
 
-	if (strlen(name) > MAX_CLUSTER_MEMBER_NAME_LEN)
+	if ((!name) || (strlen(name) > MAX_CLUSTER_MEMBER_NAME_LEN) || (votes < 0))
 	{
 		errno = EINVAL;
 		return -1;
@@ -1017,7 +1018,12 @@ int cman_register_quorum_device(cman_handle_t handle, char *name, int votes)
 
 	memcpy(buf, &votes, sizeof(int));
 	strcpy(buf+sizeof(int), name);
-	return info_call(h, CMAN_CMD_REG_QUORUMDEV, buf, strlen(name)+1+sizeof(int), NULL, 0);
+	return info_call(h, ops, buf, strlen(name)+1+sizeof(int), NULL, 0);
+}
+
+int cman_register_quorum_device(cman_handle_t handle, char *name, int votes)
+{
+	return cman_set_quorum_device(handle, CMAN_CMD_REG_QUORUMDEV, name, votes);
 }
 
 int cman_unregister_quorum_device(cman_handle_t handle)
@@ -1053,6 +1059,11 @@ int cman_get_quorum_device(cman_handle_t handle, struct cman_qdev_info *info)
 	return ret;
 }
 
+int cman_update_quorum_device(cman_handle_t handle, char *name, int votes)
+{
+	return cman_set_quorum_device(handle, CMAN_CMD_UPDATE_QUORUMDEV, name, votes);
+}
+
 int cman_get_fenceinfo(cman_handle_t handle, int nodeid, uint64_t *time, int *fenced, char *agent)
 {
 	struct cman_handle *h = (struct cman_handle *)handle;
cman/lib/libcman.h +4 -0
index feb10a2..9f97875 100644
@@ -420,6 +420,9 @@ int cman_barrier_delete(cman_handle_t handle, const char *name);
 /*
  * Add your own quorum device here, needs an admin socket
  *
+ * register_quorum and update_quorum arguments are mandatory.
+ * name has to be a valid null-terminated string and votes >= 0.
+ *
  * After creating a quorum device you will need to call 'poll_quorum_device'
  * at least once every (default) 10 seconds (this can be changed in CCS)
  * otherwise it will time-out and the cluster will lose its vote.
@@ -428,6 +431,7 @@ int cman_register_quorum_device(cman_handle_t handle, char *name, int votes);
 int cman_unregister_quorum_device(cman_handle_t handle);
 int cman_poll_quorum_device(cman_handle_t handle, int isavailable);
 int cman_get_quorum_device(cman_handle_t handle, struct cman_qdev_info *info);
+int cman_update_quorum_device(cman_handle_t handle, char *name, int votes);
 
 /*
  * Sets the dirty bit inside cman. This indicates that the node has
cman/qdisk/main.c +26 -2
index c1598fa..2f0c2ca 100644
@@ -690,6 +690,17 @@ register_device(qd_ctx *ctx)
 				ctx->qc_votes : 0);
 }
 
+static int
+update_device(qd_ctx *ctx)
+{
+	return cman_update_quorum_device(
+			ctx->qc_cman_admin,
+			(ctx->qc_flags&RF_CMAN_LABEL) ?
+				ctx->qc_cman_label : ctx->qc_device,
+			(!(ctx->qc_flags & RF_MASTER_WINS) ||
+			 ctx->qc_status == S_MASTER) ?
+				ctx->qc_votes : 0);
+}
 
 static int 
 adjust_votes(qd_ctx *ctx)
@@ -2119,9 +2130,22 @@ main(int argc, char **argv)
 
 	if (!_running)
 		goto out;
-	
+
 	/* This registers the quorum device */
-	register_device(&ctx);
+	ret = register_device(&ctx);
+	if (ret) {
+		if (errno == EBUSY) {
+			logt_print(LOG_NOTICE, "quorum device is already registered, updating\n");
+			ret = update_device(&ctx);
+			if (ret) {
+				logt_print(LOG_ERR, "DEBUG: unable to update quorum device info\n");
+				goto out;
+			}
+		} else {
+			logt_print(LOG_ERR, "Unable to register quorum device!\n");
+			goto out;
+		}
+	}
 
 	io_nanny_start(ch_user, ctx.qc_tko * ctx.qc_interval);
 
Signature · 1 line
1.7.4.4
#1 lore raw

PATCH 1/1 cman: improve cman/qdisk interactions

Hi,

Good design -- couple of things...

Quoted 1 line · Fabio M. Di Nitto

- cman: use strdup instead of malloc+strcpy (code is more readable)

* Not really a necessary change, but ok.

Quoted 6 lines

- libcman: perform better error checking in register_quorum_device/update_quorum_device

- Allow qdisk to update device name in cman using a new libcman quorum API call
- Perform slight better error checking of some update opertaions

Resolves: rhbz#735917

* All in all, this seems like a lot of patch for a very tiny problem or set of problems in a very narrow use case (renaming quorum device).

cman/daemon/commands.c static int do_cmd_try_shutdown(struct connection *con, char *cmdbuf) · line 1095

Quoted 8 lines
cman/daemon/commands.c +8 -0
+	if (quorum_device->name)
+		free(quorum_device->name);
+
+	free(quorum_device);
+
+	quorum_device = NULL;
+
+	return;

* return is implied in functions returning void (no big deal; stylistic stuff)

Quoted 4 lines
cman/daemon/commands.c +4 -0
+static void quorum_device_update_votes(int votes)
+{
+	int oldvotes;
+

cman/daemon/commands.c static int do_cmd_try_shutdown(struct connection *con, char *cmdbuf) · line 1112

Quoted 10 lines
cman/daemon/commands.c +10 -0
+	/* Update votes even if it existed before */
+	oldvotes = quorum_device->votes;
+	quorum_device->votes = votes;
+
+	/* If it is a member and votes decreased, recalculate quorum */
+	if (quorum_device->state == NODESTATE_MEMBER &&
+	    oldvotes != votes) {
+		recalculate_quorum(1, 0);
+	}
+}

* Code does not match comments; 'oldvotes != votes' is not a check for decrease, and recalculate_quorum(1, 0) allows both increases and decreases to votes. Is the comment wrong or the code? (I think the comment is wrong...)

cman/qdisk/main.c main(int argc, char **argv) · line 2143

Quoted 7 lines
cman/qdisk/main.c +7 -0
+		if (errno == EBUSY) {
+			logt_print(LOG_NOTICE, "quorum device is already registered, updating\n");
+			ret = update_device(&ctx);
+			if (ret) {
+				logt_print(LOG_ERR, "DEBUG: unable to update quorum device info\n");
+				goto out;
+			}

* Is this a debug or an error message? An error message with DEBUG: in it is confusing; can we have one or the other?

Signature · 1 line
Lon Hohberger - Red Hat, Inc.
#2 lore raw

PATCH 1/1 cman: improve cman/qdisk interactions

Quoted 8 lines · Lon Hohberger

Hi,

Good design -- couple of things...

Quoted 1 line · Fabio M. Di Nitto

- cman: use strdup instead of malloc+strcpy (code is more readable)

* Not really a necessary change, but ok.

Yeah I agree, I just didn?t like malloc + strcpy and lack of memset to guarantee 0 byte end string.

Quoted 10 lines
Quoted 7 lines

- libcman: perform better error checking in register_quorum_device/update_quorum_device

- Allow qdisk to update device name in cman using a new libcman quorum API call
- Perform slight better error checking of some update opertaions

Resolves: rhbz#735917

* All in all, this seems like a lot of patch for a very tiny problem or set of problems in a very narrow use case (renaming quorum device).

It looks big only because I turn a lot of code in register_quorum_device shared with update_quorum_device

So far there are 2 problems clearly identified. One is purely cosmetic (renaming the device and show it correctly), the other might be less cosmetic, where, after renaming a device, qdiskd cannot update votes in cman anylonger. This is because the vote updates are pushed via register_quorum, that after a rename, will always return -EBUSY.

I agree that both are corner cases for most users, but it?s also easy to fix.

Quoted 13 lines

cman/daemon/commands.c static int do_cmd_try_shutdown(struct connection *con, char *cmdbuf) · line 1095

Quoted 10 lines
cman/daemon/commands.c +8 -0
+	if (quorum_device->name)
+		free(quorum_device->name);
+
+	free(quorum_device);
+
+	quorum_device = NULL;
+
+	return;

* return is implied in functions returning void (no big deal; stylistic stuff)

Yes, I like it explicit but either way I can drop it.

Quoted 12 lines
Quoted 12 lines
cman/daemon/commands.c +11 -0
+static void quorum_device_update_votes(int votes)
+{
+	int oldvotes;
+
+	/* Update votes even if it existed before */
+	oldvotes = quorum_device->votes;
+	quorum_device->votes = votes;
+
+	/* If it is a member and votes decreased, recalculate quorum */
+	if (quorum_device->state == NODESTATE_MEMBER &&
+	    oldvotes != votes) {
Quoted 8 lines

cman/daemon/commands.c static int do_cmd_try_shutdown(struct connection *con, char *cmdbuf) · line 1112

Quoted 3 lines
cman/daemon/commands.c +3 -0
+		recalculate_quorum(1, 0);
+	}
+}

* Code does not match comments; 'oldvotes != votes' is not a check for decrease, and recalculate_quorum(1, 0) allows both increases and decreases to votes. Is the comment wrong or the code? (I think the comment is wrong...)

I believe the comment is incorrect. That code has been copied pristine from register_quorum and shared with update_quorum.

Quoted 13 lines

cman/qdisk/main.c main(int argc, char **argv) · line 2143

Quoted 9 lines
cman/qdisk/main.c +7 -0
+		if (errno == EBUSY) {
+			logt_print(LOG_NOTICE, "quorum device is already registered, updating\n");
+			ret = update_device(&ctx);
+			if (ret) {
+				logt_print(LOG_ERR, "DEBUG: unable to update quorum device info\n");
+				goto out;
+			}

* Is this a debug or an error message? An error message with DEBUG: in it is confusing; can we have one or the other?

Plain oversight... it should be an error message since we are aborting startup.

Fabio

#3 lore raw

PATCH 1/1 cman: improve cman/qdisk interactions

In reply to Lon Hohberger
Quoted 11 lines · Lon Hohberger

Hi,

Good design -- couple of things...

Quoted 1 line · Fabio M. Di Nitto

- cman: use strdup instead of malloc+strcpy (code is more readable)

* Not really a necessary change, but ok.

Quoted 2 lines

- libcman: perform better error checking in register_quorum_device/update_quorum_device

Quoted 9 lines
Quoted 4 lines

- Allow qdisk to update device name in cman using a new libcman quorum API call
- Perform slight better error checking of some update opertaions

Resolves: rhbz#735917

* All in all, this seems like a lot of patch for a very tiny problem or set of problems in a very narrow use case (renaming quorum device).

I've chatted with Fabio about this. It's my opinion that this is actually the least intrusive way of fixing the problem. The other ways involve overloading existing API calls and adding state, which add not much less code and also lot more in the way of potential confusions and problems.

Chrissie