Add clustering interface #195

edwintorok · 2018-01-25T16:07:29Z

This adds a new xcp.cluster library for the new clustering interface and doesn't touch any of the existing interfaces:

git diff origin/master --stat
 cluster/cluster_client.ml    |  14 ++++++++++
 cluster/cluster_idl.ml       |   3 +++
 cluster/cluster_interface.ml | 214 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 cluster/jbuild               |  40 ++++++++++++++++++++++++++++
 4 files changed, 271 insertions(+)

The spec file doesn't require any changes.

The purpose of this PR is to reduce the diff against master to make merging feature/REQ477 easier.

The commit history here reflects the development, could consider squashing them if preferred.

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

Signed-off-by: Edwin Török <edvin.torok@citrix.com>

This fits better with the naming of the enable and disable functions. Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

This more accurately describes its meaning. Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

The value of this field can be derived from membership of enabled_members, so there's no need to try to maintain this field. Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

This is needed in case of IP address changes while the node was disabled. Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

This is needed in the case that all other nodes a node was aware of have changed address. Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

The modification to Remote.config adds a 'start' parameter. Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

Signed-off-by: Callum McIntyre <callum.mcintyre@citrix.com>

This is private to the implementation of xapi-clusterd Signed-off-by: Edwin Török <edvin.torok@citrix.com>

This should allow to find which XAPI task causes which xapi-clusterd RPC calls, and also to track xapi-clusterd RPC calls across hosts. Signed-off-by: Edwin Török <edvin.torok@citrix.com>

lindig

Minor comments.

lindig · 2018-01-25T16:22:24Z

cluster/cluster_interface.ml

+[@@deriving rpcty]
+
+type address = IPv4 of string
+[@doc ["An IP address"]]


Should this say "An IP address a.b.c.d" or similar to indicate that this is IPv4 and not a hostname or IPv6?

lindig · 2018-01-25T16:30:58Z

cluster/cluster_interface.ml

+
+  let create = declare
+      "create"
+      ["Creates the cluster. The call takes the initial config of";


When we concatenate these strings, we get something like ".. of;the ..". Is this intended?

The ; won't be part of the concatenation, it separates the strings in the ocaml list.

That's right. So the concatenation would add a space?

Yes it concatenates the strings with spaces: https://github.com/mirage/ocaml-rpc/blob/474d9cd1a10de4f6078a6422e14a91ed84d8173b/lib/markdowngen.ml#L97

coveralls · 2018-01-25T16:36:58Z

Coverage remained the same at 54.268% when pulling 0a49d87 on edwintorok:private/edvint/cluster into 0d8714a on xapi-project:master.

IPv6 support is not implemented yet. Signed-off-by: Edwin Török <edvin.torok@citrix.com>

mseri · 2018-01-30T15:09:21Z

Can this be merged?

mseri · 2018-01-30T15:10:08Z

cluster/cluster_interface.ml

+let xml_path = "/var/xapi/cluster"
+
+type debug_info = string
+[@@doc ["An uninterpreted string associated with the operation."]]


Just for reference: now you can use actual docstrings here, and they will work as well

mseri · 2018-01-30T15:10:41Z

cluster/jbuild

+  (modules (:standard))
+  (flags (:standard -w -39 %s))
+  (libraries (xcp threads rpclib))
+  (wrapped false)


We should start wrapping xcp-idl libraries (this is not an issue or something to do now)

Please add the executable ~~rules for cluster_client and call it~~ cluster_cli in line with the other components. Do not add a public_name or an alias. If #198 is merged before this PR, please update the README

Ok, I'll update the PR, you are right we can drop cluster_idl.ml in that case.

lindig · 2018-01-30T15:12:41Z

Do we want the history to be cleaned up? This is new code - so in my opinion this could be also squashed or we could have one commit per file.

mseri · 2018-01-30T15:14:16Z

cluster/jbuild

+(library
+ ((name xcp_cluster)
+  (public_name xcp.cluster)
+  (modules (:standard))


~~I think you should do (modules (:standard \ cluster_client)) and~~ completely delete cluster_idl

Sorry, an oversight. I meant to add a cluster_cli as with the other components. Then remove that from the modules list, and add the executable rules

mseri · 2018-01-30T15:15:24Z

cluster/cluster_client.ml

@@ -0,0 +1,14 @@
+
+open Xcp_client


In line with comments in new clients, please don't open this globally, but add let open Xcp_client inside the rpc function body

mseri

Please add the changes to make the structure close to the one of the other components, and before merging let's squash the commits. I think we can do that from github at merge time

Signed-off-by: Edwin Török <edvin.torok@citrix.com>

A more complete CLI is available as part of the clustering daemon itself. Signed-off-by: Edwin Török <edvin.torok@citrix.com>

edwintorok · 2018-01-30T15:58:14Z

Pushed some cleanups, I agree that we should squash it all at merge time.
Note that I changed the API of the rpc function, it used to take a function for building the URL, the cluster daemon / xapi integration doesn't support anything other than message-switch at the moment, so I just made it similar to the other interfaces which take a url directly.

edwintorok · 2018-01-30T16:15:20Z

The failure is the same on as in #198 due to coverage script

mseri

Sorry, too much renaming. Client should still be called cluster_client.ml and interface called cluster_interface.ml. Then in the code using it we shouold use Cluster_client and Cluster_interface. This is what we do on all other idl components. I think this was due to lack of packing of the submodules, but at least the naming makes it clear where the modules are coming from and are consistent across the various interfaces.

edwintorok · 2018-01-30T16:45:46Z

Isn't it a bit redundant to have Xcp_cluster.Cluster_interface? Thought that Xcp_cluster.Interface would be better now that I dropped the wrapping.

mseri · 2018-01-30T16:49:20Z

It is, but I'd rather keep things consistent and we cannot change all the other interfaces for the moment

also explain why we call it LocalAPI (because we also have a Remote API elsewhere). Signed-off-by: Edwin Török <edvin.torok@citrix.com>

edwintorok · 2018-01-30T16:52:50Z

Ok, wrapping is back and renamed the files to have cluster_ prefix.

mseri · 2018-01-30T16:53:09Z

Feel free to merge

jonludlam and others added 20 commits January 25, 2018 16:01

Add the clustering interface

c3deb7e

Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>

cluster: jbuilderize

6a92655

Signed-off-by: Edwin Török <edvin.torok@citrix.com>

cluster: rename active_members to enabled_members

7ac7eee

This fits better with the naming of the enable and disable functions. Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

cluster: Correct description of disable function

8191518

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

cluster: rename 'is_running' to 'is_enabled'

f806e96

This more accurately describes its meaning. Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

cluster: remove 'is_enabled' field from cluster_config

afe6a88

The value of this field can be derived from membership of enabled_members, so there's no need to try to maintain this field. Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

CP-24908: cluster: Remote.rejoin: add address parameter

6f6e4cf

This is needed in case of IP address changes while the node was disabled. Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

CP-24203: cluster: add declare_changed_addrs

9ce4868

This is needed in the case that all other nodes a node was aware of have changed address. Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

CA-270008: cluster: modify Remote.config; add Remote.ping, Remote.stop

a846d60

The modification to Remote.config adds a 'start' parameter. Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

cluster: refactor all_members out of cluster_config

19c25b1

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

cluster: diagnostics: add is_enabled field

f7409c3

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

cluster: diagnostics: distinguish live_ and saved_cluster_config

0dd1667

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

cluster: diagnostics: add is_running field

2fc7648

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

cluster: diagnostics: support next_cluster_config field

0049e61

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

cluster: diagnostics: add config_valid field

ee45bcb

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

CA-271869: cluster: add config_{invalidate,validate,abort}

876f03f

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

cluster: add config_version to cluster_config

4a23769

Signed-off-by: Jonathan Davies <jonathan.davies@citrix.com>

[CP-25892] Make cluster parameters configurable

4506f09

Signed-off-by: Callum McIntyre <callum.mcintyre@citrix.com>

CA-273683: move remote interface back into xapi-clusterd

ec29c97

This is private to the implementation of xapi-clusterd Signed-off-by: Edwin Török <edvin.torok@citrix.com>

CP-26038: plumb through a debug field

ac33fa4

This should allow to find which XAPI task causes which xapi-clusterd RPC calls, and also to track xapi-clusterd RPC calls across hosts. Signed-off-by: Edwin Török <edvin.torok@citrix.com>

lindig reviewed Jan 25, 2018

View reviewed changes

cluster: be explicit that IP means IPv4

0a49d87

IPv6 support is not implemented yet. Signed-off-by: Edwin Török <edvin.torok@citrix.com>

mseri reviewed Jan 30, 2018

View reviewed changes

mseri requested changes Jan 30, 2018

View reviewed changes

edwintorok added 2 commits January 30, 2018 15:48

cluster: make xcp_cluster wrapped and drop cluster_idl

fcb056c

Signed-off-by: Edwin Török <edvin.torok@citrix.com>

cluster: add minimal CLI

377af31

A more complete CLI is available as part of the clustering daemon itself. Signed-off-by: Edwin Török <edvin.torok@citrix.com>

mseri requested changes Jan 30, 2018

View reviewed changes

cluster: undo wrapping/renaming

dee9951

also explain why we call it LocalAPI (because we also have a Remote API elsewhere). Signed-off-by: Edwin Török <edvin.torok@citrix.com>

mseri approved these changes Jan 30, 2018

View reviewed changes

edwintorok merged commit 51b6921 into xapi-project:master Jan 30, 2018

edwintorok deleted the private/edvint/cluster branch January 30, 2018 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clustering interface #195

Add clustering interface #195

edwintorok commented Jan 25, 2018

lindig left a comment

lindig Jan 25, 2018

lindig Jan 25, 2018

edwintorok Jan 25, 2018

lindig Jan 25, 2018

edwintorok Jan 25, 2018

coveralls commented Jan 25, 2018 •

edited

Loading

mseri commented Jan 30, 2018

mseri Jan 30, 2018

mseri Jan 30, 2018 •

edited

Loading

mseri Jan 30, 2018 •

edited

Loading

edwintorok Jan 30, 2018

lindig commented Jan 30, 2018

mseri Jan 30, 2018 •

edited

Loading

mseri Jan 30, 2018

mseri Jan 30, 2018

mseri left a comment

edwintorok commented Jan 30, 2018

edwintorok commented Jan 30, 2018

mseri left a comment •

edited

Loading

edwintorok commented Jan 30, 2018

mseri commented Jan 30, 2018 •

edited

Loading

edwintorok commented Jan 30, 2018

mseri commented Jan 30, 2018

Add clustering interface #195

Add clustering interface #195

Conversation

edwintorok commented Jan 25, 2018

lindig 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

coveralls commented Jan 25, 2018 • edited Loading

mseri commented Jan 30, 2018

Choose a reason for hiding this comment

mseri Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

mseri Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindig commented Jan 30, 2018

mseri Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mseri left a comment

Choose a reason for hiding this comment

edwintorok commented Jan 30, 2018

edwintorok commented Jan 30, 2018

mseri left a comment • edited Loading

Choose a reason for hiding this comment

edwintorok commented Jan 30, 2018

mseri commented Jan 30, 2018 • edited Loading

edwintorok commented Jan 30, 2018

mseri commented Jan 30, 2018

coveralls commented Jan 25, 2018 •

edited

Loading

mseri Jan 30, 2018 •

edited

Loading

mseri Jan 30, 2018 •

edited

Loading

mseri Jan 30, 2018 •

edited

Loading

mseri left a comment •

edited

Loading

mseri commented Jan 30, 2018 •

edited

Loading