Support node type based allocation

[whynick1]

We use VagrantCluster and define a couple of node with VagrantFile as below:

config.vm.define "zookeeper-#{i}" do |zookeeper|
...
config.vm.define "server-#{i}" do |server|
...

As we have different GCE images (https://github.com/mitchellh/vagrant-google) for zookeeper node and server node, we want to find out a way to have ducktape allocate different types of node for different services.

Therefore, we try to add an optional parameter node_type to Service class, so that developer can specify type of node to allocate during integration test.

This change is back-compatible, as node_type is default to None.

[ghost]

@confluentinc It looks like @whynick1 just signed our Contributor License Agreement. 👍

Always at your service,

clabot

[criccomini]

@ewencp could use your feedback here. I'm a bit surprised that it seems ducktape doesn't understand different types of nodes. It appears that by default all nodes are just grabbed from a pool. Kafka seems to get around this by having everything (including tests and ZK) available in all images. I kind of feel we might be "doing it wrong" or missing something. Could use your guidance here.

[ewencp]

You're not doing it wrong, you're just running into some simplifying assumptions that we haven't had to update for almost 4 years now :)

We started out with a super simple model b/c with the Vagrant setup combined with the fact that originally we were just booting up a base image and then loading in the extra software, it was fine to assume everything was symmetric and we could have a simple pool.

Things have definitely gotten a bit more complex, but for most testing in AK and Confluent Platform, we still just assume uniformity. On the Confluent Platform side, this works out because we actually pre-bake an AMI and use vagrant-aws to launch a bunch of instances.

That said, I can see how this wouldn't be ideal for everyone :) In fact, we have run into a bit of this wrt OS support -- testing on mixed OS clusters (e.g. testing a Windows-based client against Linux-based services) requires a mixed cluster and some smarts on allocating nodes. See #155 for where most of the additions to support that were made.

tbh, those additions were almost 2 years ago and I'm forgetting the context and details on them, but general idea is that we wanted to extend nodes in the cluster with additional parameters and allow a node_spec on the request for cluster nodes that lets you specify what types of nodes you require. In that PR you can see an example -- instead of

@cluster(num_nodes=10)

you might use something like

 @cluster(node_spec={ducktape.cluster.remoteaccount.RemoteAccount.LINUX: 10})

OS was specifically the thing being targeted in that PR, but it totally makes sense to support other parameters as well. Probably a generic tagging setup is the right way to handle this. In addition to your use case, I actually want to see this happen for a few other use cases as well. Another place this would definitely be applicable is if you need heterogeneous nodes (e.g. for a perf test you might want beefier machines).

I'm more than happy to help get improvements in that support filtering the nodes. The main thing I'd want to make sure we pay attention to is how we express it in the @cluster annotation to be both ergonomic and sufficiently extensible.

[criccomini]

@ewencp if you can provide a semi-detailed implementation suggestion, I think @whynick1 can tackle it.

[whynick1]

Hi @ewencp , thanks for the feedback! I am excited to see the potential use cases. As @criccomini said, it will be great if you can provide some semi-detailed implementation suggests.

Meanwhile, I have two questions for you:

1. It's true that cluster_spec provide hint about how many nodes of each type the test will consume. However, besides telling which resource need for a cluster, we also need a way to specify which type of nodes to allocate for different services, like

zk = ZookeeperService(test_context, node_type="zookeeper", num_nodes=1)
my_server = MyServerService(test_context, node_type="server", num_nodes=3)
my_storage = MyStorageService(test_context, node_type="storage", num_nodes=3)

So, my plan is to add an optional node_type param to Service class, what you think?

2. For people uses Vagrant to manage virtual machines (like us), this is how we define a node

config.vm.define "my-node-1" do |node|

Here my-node-1 is the node name. When creating such node, it is put into the same pool (a list) as other nodes. In order to allocate by type, my plan is to use multiple pools (a dict of <node_type, pool>). If you agree, my question will be where do we get "node_type"?
In current commit, what I do is to extract from node name,

node_type = re.search(extract_pattern, node.name).group(1)

To me this is kind of less flexible. Do you have any recommendation?

[criccomini]

(please ignore reference.. copied wrong GH PR link :) )

[whynick1]

Waiting for the fix for @cluster, #207

[ewencp]

For 1, you should just be able to use the cluster_spec that's already passed into Service, see https://github.com/confluentinc/ducktape/blob/master/ducktape/services/service.py#L54 and how we resolve between num_nodes vs cluster_spec: https://github.com/confluentinc/ducktape/blob/master/ducktape/services/service.py#L97. The num_nodes is effectively just legacy compatibility (or alternatively, ease of use for the simple uniform-cluster case). The cluster_spec is going to be more flexible since you can constrain across more than a single parameter at a time.

One interesting design question is how we handle things if there are multiple dimensions and the caller only specifies one (e.g. if we have varying sizes of linux machines, but I only ask for linux machine, how does the test runner decide which node to allocate?). tbh, I'm wary of allowing this to get much more complex than some very small number of dimensions (OS was the obvious one for us, but I think generic type can work as well; not sure if that eventually becomes too limiting). The reason I'm wary is that we ideally wouldn't need a full-blown cluster scheduler built-in to ducktape. If we start to approach that direction (e.g. we're specifying full-on memory, cpu, disk, etc constraints), then probably that should just farm out the real work to a real cluster scheduler like k8s via a K8SCluster implementation, or something like that, which can expose "infinite" resources (maybe only constrained by number of active "nodes" which I guess in k8s would be active pods for this ducktape process) and just serve up nodes based on whatever happens to be requested. In that case, we'd want to set it up so you can specify in the service what you need, but the @cluster would probably just be a number of nodes.

For some historical context, we in fact never really wanted to require the @cluster annotation for tests, obviously the ideal thing would be to just have the test know how many nodes it needed based on what the Services utilized. In fact, that's how it used to work. But with test parameterizations, set_up/tear_down that are generic, and the need to construct some Services dynamically in those tests (e.g. because they require parameter info like security configs that we use to test different security setups with a single test), we lost the ability to know number of nodes up front automatically. I actually think we could fix this in future versions and get rid of the @cluster annotation entirely -- we'd need set_up/tear_down to handle parameters and remove dynamic service allocation during the test. The latter constraint sometimes makes writing a test a little bit less natural (you have to move Service allocation away from where you'd intuitively want it in a simple test that doesn't even bother with anything other than the test_X() method), but I think the benefits are worth it (no manual node counting, I think this also allows you to reuse the same test method against different cluster configurations, e.g. allowing you to use a single test method to do scale testing, test against different mem/cpu configs, etc).

For 2, today the node type allocation is very basic and just baked into the Cluster implementation (see https://github.com/confluentinc/ducktape/blob/master/ducktape/cluster/json.py#L103) and the allocation is similar to what you describe, but currently based only on the OS type (see https://github.com/confluentinc/ducktape/blob/master/ducktape/cluster/node_container.py#L33). NodeContainer is supposed to be very generic, so we would need to decide how much we want to bake into backend-specific Clusterclasses vs requiring implementations to just fit into a standard model. Again this ties back to how complicated this is going to get, how you handle tests that may omit constraints (worst case being your test suite has some outliers like you wanttype="highmem"` but most just want whatever node you can give them). I'm open to different options here -- I don't know there's a "right" solution, we'd have to figure out what seems best based on the ways we think this would end up being used. For exmaple your "type" approach is different from what many people might think to go to (e.g. specific resource constraints like mem/cpu/disk).

For vagrant specifically, personally I would try to figure out a way to not tie it to the naming as that's inflexible, requires you to specify "serialization" format if you ever want to support multiple tags, etc. However, due to the way the VagrantCluster currently loads info, I'm not sure of a much better way to do it. The best I can see via quick scan of options is to use config.ssh.forward_env or something like that to specify via env var cluster type info in a standard env var, then extract from that (whether via the vagrant ssh-config command or by doing a pass of ssh'ing to each node and collecting the info). That said, again, this depends on how flexible a model we want. If we can really guarantee we'll only ever want one "type" parameter rather than, e.g., multiple tags, then the approach you describe might be workable.

wrt impl, let me loop back around, though tbh it's been so long since looking at this code in detail that I'm not sure how much I can just summarize without basically implementing myself. Hopefully pointers above are a good starting point and I can take a quick pass at current code to call out gaps/issues I might find easily.

[whynick1]

@ewencp Thank you so much for your comment. I try to make a brief summary for each sections and provide some of my thought, as below:

Q1: You should just be able to use the cluster_spec that's already passed into Service
A1: Previously, I expect something like @cluster(nodes_spec={'storage':1, 'server':3}). Base on your suggest, I think we can reuse cluster_spec by adding ClusterSpec.from_dict() method, so that we can do @cluster(cluster_spec=ClusterSpec.from_dict({'storage':3, 'server':1})). Does that sounds good to you?

Q2: Generic type may eventually becomes too limiting considering multiple dimensions a node could have (size; type; os)?
A2: From design point of view, it is true that generic type is less extensiable and flexialbe than muliple tagging (that allows specify multiple dimensions of a node), however, it could simplify the design for now. My thought was type can be really generic, like you can say "linux_storage_10G" is a type, in which can fit most of use cases.

Q3: For historical reason, we lost the ability to know number of nodes up front automatically. But we want to solve that, and eventually get rid of the @cluster annotation entirely.
A3: I know @cluster is not required. What I am thinking here is simple. Currently, we can pass either num_nodes or cluster_spec to a Service class, and check if required nodes satisfied for current test.
Once we support specifying type in cluster_spec, we should also provide a way to check resource. Base on my response to question 1, it should look like @cluster(cluster_spec=ClusterSpec.from_dict({'storage':3, 'server':1}))

Q4: I'm open to different options, how to decide a node's type that fit into a standard model. We want to make it flexible rather than tie to the naming.
A4: I totally agree that tie node type to naming is not a good design. The reason I am doing that is because 1) I checked how allocate by os is implemented, it turn out to be based on naming. 2) I don't see an easier way to specify "type" in another fields in terms of using Vagrantfile.
Regarding ssh.forward_env(an array of host environment variables to forward to the guest) you mentioned, I agree it's a more flexible thus we no longer extract 'type' from instance name. However, as you may notice, this option is design for "ssh". If possible, we might want to use a Vagrantfile options -- “config.vm.type”, which does not exist for now( https://friendsofvagrant.github.io/v1/docs/vagrantfile.html). What do you think?

[criccomini]

@maxzheng @ewencp anyone interested in this patch? Been almost 2 months since last engagement. We are willing to work on it. Waiting for guidance based on @whynick1 's last comments. :(

[maxzheng]

@criccomini I am not working on developer tooling anymore, so it's up to @ewencp and the @confluentinc/tools team :)

[whynick1]

Hi @jarekr would you please leave a comment! 😃Thanks!
cc @criccomini

[criccomini]

Guys, very close to just forking this project. Responsiveness on this ticket is pretty low. Are you interested in this PR or not? @maxzheng @confluentinc/tools @jarekr @gwenshap

[ewencp]

@criccomini This is my fault. Ironically, we wish there were more contributions back to this repo to improve test framework generally (rather than, e.g., hacking around things in tests, which does happen...), but then when they arrive, we're not setup for them because they are so infrequent :)

As I'm sure you can appreciate, we also have limited reviewer bandwidth. Most folks that have been tagged here on this PR aren't actually reviewers for this code! We have an assigned team just for ownership purposes, but actually the core framework sees so few changes (whether that's a good or bad thing), that some of the folks mentioned really don't have the context to process or review these changes.

I need a bit of time to follow up and review (took me a full day to just process, evaluate, and write up earlier response), but I will allocate time to review followups from @whynick1. And I apologize for delay. Tomorrow looks shit, but I will follow up on this by Monday.

Longer term, @confluentinc/quality-eng is a new org that hasn't necessarily taken ownership yet, but is a bit more targeted and likely to be more responsive for cases like this. I think it'd be good to get them looped into this now, especially since we have community interested in testing like this, which is a rare thing!

[criccomini]

No worries! Totally understand, and I know how it goes. I really don't want to fork, but we're nearing a point on our end where we have to since we've developed a bunch of code around ducktape at this point. :P LMK how I can help.

[ewencp]

A1: Previously, I expect something like @cluster(nodes_spec={'storage':1, 'server':3}). Base on your suggest, I think we can reuse cluster_spec by adding ClusterSpec.from_dict() method, so that we can do @cluster(cluster_spec=ClusterSpec.from_dict({'storage':3, 'server':1})). Does that sounds good to you?

Yeah, that's roughly what I meant. I think we should consider dropping the need for using a ClusterSpec method -- can we look into whether it's feasible to just pass something like @cluster(cluster_spec={'storage': 3, 'server': 1}) since that's better UX for the test author?

But if that doesn't work, roughly what you're getting at works. (Note that even if we need to construct the ClusterSpec directly, for a completely generic mode like this we might be able to get the normal constructor to handle it so it is a bit more standard.

A2: From design point of view, it is true that generic type is less extensiable and flexialbe than muliple tagging (that allows specify multiple dimensions of a node), however, it could simplify the design for now. My thought was type can be really generic, like you can say "linux_storage_10G" is a type, in which can fit most of use cases.

Yeah, tradeoff makes sense. Mainly with things like this I just want to make sure we plan for possible extension and set ourselves up to be able to do so compatibly.

A3: I know @cluster is not required. What I am thinking here is simple. Currently, we can pass either num_nodes or cluster_spec to a Service class, and check if required nodes satisfied for current test.
Once we support specifying type in cluster_spec, we should also provide a way to check resource. > Base on my response to question 1, it should look like @cluster(cluster_spec=ClusterSpec.from_dict({'storage':3, 'server':1}))

Agreed that as long as we have the current issue with counting automatically, this works fine. And I'm happy to merge something based on your proposal. Was just pointing out that we might also want to think about that eventual future and where/how this metadata should be expressed. If we were able to get rid of the node count, then having to specify things this way isn't great. In general its an issue because rather than naturally expressing it with the Service creation (where you actually want to associate the information about what type of node to use). I guess since they are such different approaches though, maybe for now we can't make this cleaner and simpler for users.

A4: I totally agree that tie node type to naming is not a good design. The reason I am doing that is because 1) I checked how allocate by os is implemented, it turn out to be based on naming. 2) I don't see an easier way to specify "type" in another fields in terms of using Vagrantfile.

Yeah, I was trying to get a bit of design reasoning behind this update as well because the OS additions were a bit of a hack job that I'm not sure I'm happy with. The reason I was suggesting something other than just naming was due to previous comment about possibly specifying preset dimensions (e.g. cpu, memory, etc). If we aren't going down that path and staying very open ended, then yeah, naming is probably the only reasonable approach.

The other downside of using naming is that you can't do things like ask for any node that meets certain criteria. From a test scheduling and cluster utilization perspective, this might not be great in some cases. For example, if I have just a few tests that need very large nodes, given current model you still have to have those available for the entire test run, but those nodes could go unutilized. We can sort of address that if most tests just don't specify and will use any node. If you get into too many classes of nodes, I can see this still possibly being a problem.

Regarding ssh.forward_env(an array of host environment variables to forward to the guest) you mentioned, I agree it's a more flexible thus we no longer extract 'type' from instance name. However, as you may notice, this option is design for "ssh". If possible, we might want to use a Vagrantfile options -- “config.vm.type”, which does not exist for now( https://friendsofvagrant.github.io/v1/docs/vagrantfile.html). What do you think?

If we can't generalize, then we don't need to do it. I was mainly raising this if we do something more than just a fixed string approach. If we decide to just go with strings and user-defined classes without any additional knowledge in the framework, then this wouldn't be needed.

[whynick1]

A1: Yeah, that's roughly what I meant. I think we should consider dropping the need for using a ClusterSpec method -- can we look into whether it's feasible to just pass something like @cluster(cluster_spec={'storage': 3, 'server': 1}) since that's better UX for the test author?

Agree! Let's do @cluster(cluster_spec={'storage': 3, 'server': 1}).

A4: Yeah, I was trying to get a bit of design reasoning behind this update as well because the OS additions were a bit of a hack job that I'm not sure I'm happy with. The reason I was suggesting something other than just naming was due to previous comment about possibly specifying preset dimensions (e.g. cpu, memory, etc). If we aren't going down that path and staying very open ended, then yeah, naming is probably the only reasonable approach.

Thanks for your explanation, and I understand your concern. It is true a generic type tagging with "multiple dimensions" would be more flexible than just based on "name".
Here are the potential implementations I can think of.

1. Leverage config.ssh.forward_env(https://www.vagrantup.com/docs/vagrantfile/) in Vagrantfile to tag node type.
   This would possibly work but will be kind of hacky.
2. Add config.vm.type setting to Vangrantfile.
   This requires work on Vagrant side. And more importantly, it is unnecessary. See 3)
3. Use existing "node type identifier" defined by different providers. (I would prefer)
   To explain why config.vm.type is unnecessary. Different vagrant provider has different ways to specify node type.

config.vm.provider "virtualbox" do |v|
  v.memory = 1024
  v.cpus = 2
end

config.vm.provider :google do |google, override|
    google.machine_type = xxxx

config.vm.provider :aws do |aws, override|
    aws.instance_type = xxxx

This means, possibly we can directly read these setting (machine_type or instance_type, or memory + cpu) from node in ducktape, store all of them in a node params dict. Then, when Service ask for node, compare each node's params dict.
An example of Service class claim cluster_spec:

node_type = {'machine_type': 'n1-standard-1', 'additional_disks': true}
num_nodes = 3
new WaltzStorageService(cluster_spec={node_type, num_nodes)

btw, I can start working on this next Monday at the earliest. But beforehand, I do want to hear your thought. 😃 @ewencp cc @criccomini

[ewencp]

@whynick1 Yeah, so re: A4, my point was more of a tagging/matching approach. We rely a ton today on the vagrant cluster type, but really that's a detail (and increasingly I'd like to see VagrantCluster become an implementation detail). For example, docker or ec2 clusters would also work well, but potentially offer flexibility/scalability that Vagrant doesn't.

So I had been thinking more along the lines of constraints that cluster resource managers tend to enforce, e.g. (cpu >= 2vcpu, mem >= 2GB), etc. The tradeoff is that you have to define all the potential resource constraints up-front. Additionally, you might get, e.g., a node that far exceeds your requirements and performs differently (e.g. for perf tests, you need to constrain carefully, e.g. mem>=2GB, mem<=3GB or something like that).

I'm not tied to either approach, it probably makes sense to experiment a bit and deprecate if necessary. I'm just prone to extreme care because deprecating and removing functionality can be time consuming and costly, and we're not even at a 1.0 for ducktape yet :)

[whynick1]

So I had been thinking more along the lines of constraints that cluster resource managers tend to enforce, e.g. (cpu >= 2vcpu, mem >= 2GB), etc.

Sounds good to me! I think we can provide user with

node_type = {'cpu_core': '=2', 'mem': '>=2GB', 'disk>=100GB'}
num_nodes = 3
new WaltzStorageService(cluster_spec={node_type, num_nodes)

Another thing I want to discuss with you is, how we tag machine with certain type (a multiple dimension type, including cpu, memory, disk, etc).
Unfortunately, vagrant does not provide a way to expose machine configurations (vagrant ssh-config command only list ssh related info).
Instead of reading it from Vagrantfile as beblow:

config.vm.provider "virtualbox" do |v|
  v.memory = 1024
  v.cpus = 2
end

we can directly read from machine! Like:

[machine-0]$ cat /proc/cpuinfo | grep 'cpu cores'
cpu cores    : 1
[machine-0]]$ cat /proc/meminfo | grep 'MemTotal'
MemTotal:        3622628 kB

, then attach these configurations to each node in vagrant cluster to help allocation based on constraints ('>=', '<=', '=', etc.).
This way has the advantage of

1. Flexible: whichever provider used to bring up VM machine, we can always read machine config the same way.
2. Extensible: in future, if we want to add another dimension to "node type", we just need to change cat /proc/meminfo | grep 'MemTotal'

@ewencp Does this make sense to you? 😃
cc @criccomini

[criccomini]

I like the approach @whynick1 is proposing as it is totally cross-platform. It will be a bit slower, but IMO that's OK.

Can we get some love on this? Can someone approve @whynick1 's proposed design so we can get started?

@confluentinc/tools @ewencp @gwenshap

[criccomini]

@whynick1 is going to move forward with the proposed implementation. If you guys don't want it, we'll just fork ducktape.

[ewencp]

Yeah, this approach seems like it works well. I suspect longer term we'll end up with some sort of mix because within a suite of tests it might be handy to be able to alias an actual set of requirements (e.g. ['disk >= 100G', 'mem>=8G']) to a descriptive name like largeorbigdisk`, so we'll still probably end up with some pattern for making that easy.

In terms of annotating nodes with that info, yeah, I think detection like this should work, though you do need bootstrapping info. For example, info like OS type will necessarily have to come from the cluster implementation before having access because otherwise we don't know how to connect to it (ssh vs rdp) or what commands to run (the examples given won't work on windows).

There's actually a related use case where tagging the nodes gets even a bit more complicated -- if you want to run tests against existing services, e.g. you already have a Kafka cluster and want to back KafkaService with that instead of having it allocate out of the standard pool. In that use case, we have yet another source of information for tagging nodes (and we probably want to be able to tag arbitrarily, not just via a fixed set of resource types, though perhaps the fixed built-in set is good baseline).

[whynick1]

@ewencp Supported node allocation based on (multi-dimension) machine-type according to previous discussion. Before we fixing all the tests, just want to get some feedbacks from you.

# Vagrantfile defines gce instance to setup. These are the nodes to allocate

# Services specify minimum node configurations of the test
my_service_cluster_spec = ClusterSpec.from_dict( {'cpu':1, 'mem':'1GB', 'disk':'25GB', 'additional_disks':{'/dev/sdb':'100GB'}, 'num_nodes':3})}
MyService(self.test_context, my_service_cluster_spec, ...)

# Additionally, we can add @cluster to check resource before test (fail instantly if cannot be satisfy)
@cluster(cluster_spec=ClusterSpec.from_list(
  [{'cpu':1, 'mem':'1GB', 'disk':'25GB', 'additional_disks': {'/dev/sdb':'100GB'}, 'num_nodes':3}, 
   {'cpu':1, 'mem':'2GB', 'disk':'10GB', 'num_nodes':2},
   {'cpu':1, 'mem':'512MB', 'disk':'10GB', 'num_nodes':1}]))
def waltz_xxx_test(self, param_0, param_1, param_2): ...

1. What support after change?

• All change is backwards compatible
• Instead of just linux/window type, node(class RemoteAccount) to allocate now has a class MachineType attribute
• Service can request node base on MachineType including cpu core, memory size, boot disk size and additional disks
• cpu compare numbers of cpu cores; all other compare size. (support GB/MB/KB, e.g. 1.5GB; 200MB)
• Currently, allocation is only based on lower bound (minimum requirement), means 'cpu':1 == 'cpu>=1'

2. What's the allocation strategy?

• Current strategy is on a best-effort basis
• Attribute in MachineType has different weight: cpu > mem > disk > additional_disk
• Deal with highest-demand request first (<cpu:1, mem:8G> serve before <cpu:1, mem:4G>)
• Assign the weakest machine as possible (assign node with <cpu:1, mem:8G> rather than <cpu:1, mem:16G>)
• Worst time complexity O(N * M), where N is num available nodes; M is num requested nodes
• Test will fail instantly once a request cannot be satisfied

cc @criccomini

[criccomini]

@ewencp ping

[ewencp]

I'm going to try to take a pass at the code today, but wanted to ask a couple of questions re: ux.

I think in general what you outlined with these updates makes sense. My only concern is that it feels a bit clunky, but I think this is just because you are covering the most complicated case. In particular:

• I assume common case would not be to inline these definitions, but probably to share some common definitions, eg do something like STORAGE_NODE = {...}, defining the minimum requirements (so you could, e.g. allocated nodes with arbitrary CPU that still match)?
• I assume, similarly, that with the test specs, more frequently than not we'll end up with just a pre-defined spec, matching those in services? e.g. common case would be to just do something like @cluster(cluster_spec=ClusterSpec.from_list([{BASE_NODE, num_nodes: 3}, {STORAGE_NODE, 'num_nodes': 1}]). In fact, now writing this, I realize the above is not quite right and we might want some method on ClusterSpec that copies and changes number of nodes so those entries in the list can be more like BASE_NODE.with_num_nodes(3) that copies everything but overrides num_nodes to streamline this common use case.
• Given that common case, based on the allocation strategy, it is correct that you should never have trouble with the allocation/mapping, since they will always align? So in the common case, it'd still be pretty concise and wrt allocation you wouldn't really need to worry about availability as long as nodes that can match are disjoint (which again, seems like the common case as different node types probably optimize for different types of resources, unless you have broad classifications like SMALL_NODE and LARGE_NODE).

Part of this is just to make sure UX for the common case is nice for the user, but also so we can update documentation w/ both common and advanced cases.

wrt allocation strategy, generally a greedy approach looks good, just want to clarify this point:

Test will fail instantly once a request cannot be satisfied

Does this reflect the existing policy or does this mean compared to the existing approach that can handle lack of available nodes? i.e. does this make TestScheduler fail-fast if you have parallel test runner enabled and the next test can't be satisfied from remaining nodes? This is pretty critical because as you build up even a pretty small test suite, parallel test runner becomes critical to run in a reasonable amount of time.

[whynick1]

@ewencp Based on comment, I propose a more user-friendly pattern.

STORAGE_NODE = NodeSpec(LINUX, MachineType(cpu=1, disk=100G))
cluster_spec = ClusterSpec.from_nodes(STORAGE_NODE.with_num_nodes(3))
s = StorageService(cluster_spec)

@cluster(cluster_spec=ClusterSpec.from_nodes(STORAGE_NODE.with_num_nodes(3)),
                                             SERVER_NODE.with_num_nodes(2))
def my_test()
""" A test that involves both server & storage services.
"""

Let me know if that looks good to you.

you wouldn't really need to worry about availability as long as nodes that can match are disjoint (which again, seems like the common case as different node types probably optimize for different types of resources, unless you have broad classifications like SMALL_NODE and LARGE_NODE)

Yes, I think the allocation strategy should work for most of the case.

Does this reflect the existing policy or does this mean compared to the existing approach that can handle lack of available nodes?

The change I make should not change existing fail-fast policy. Is there any unit test that covers that?
If no, I can verify it manually (with --max-parallel).

[criccomini]

@ewencp NAG NAG NAG NAG :D

[ewencp]

@ewencp Based on comment, I propose a more user-friendly pattern.

STORAGE_NODE = NodeSpec(LINUX, MachineType(cpu=1, disk=100G))
cluster_spec = ClusterSpec.from_nodes(STORAGE_NODE.with_num_nodes(3))
s = StorageService(cluster_spec)

@cluster(cluster_spec=ClusterSpec.from_nodes(STORAGE_NODE.with_num_nodes(3)),
                                             SERVER_NODE.with_num_nodes(2))
def my_test()
""" A test that involves both server & storage services.
"""

Let me know if that looks good to you.

Looks good to me. Seems simple and straightforward to use.

Does this reflect the existing policy or does this mean compared to the existing approach that can handle lack of available nodes?

The change I make should not change existing fail-fast policy. Is there any unit test that covers that?
If no, I can verify it manually (with --max-parallel).

Ok, just couldn't be sure from the description. As long as we maintain the behavior, we should be good. I think kafka tests still don't have parallel test runner turned on, but Confluent's internal tests using ducktape have had them turned on for a couple of years -- we'd be happy to do a bunch of runs with a modified version of ducktape to check for any regressions.

wrt unit tests, tbh I'm not sure since I think the parallel test runner was merged ~3 years ago and hasn't seen much movement since. given general coverage numbers, i'm pretty sure we have some coverage of that code, but not sure about that specific functionality. we can visit this in the code review.

[jspong]

Hi, I'm new on the Confluent QE team. I apologize if I'm just missing vital information or misunderstanding something, but I'm concerned that using hardware specs this way will cause long-term maintenance problems.

Even with your example provided, the specs that are given are in fact being used as a proxy for a specific capability: you are planning to run zookeeper on machines with a given cpu and disk configuration, so you write your test to search for that cpu and disk configuration and assume that it's a zookeeper node. But this assumption may not hold forever. Additionally, by tying it into the source itself, it needs to hold for other people who want to run this test as well.

Mentioned above was the question of whether or not "type" would be too limiting, and hardware specs were given as an example of dimensions that could get complicated quickly. I do think "type" is too limiting, but because there is only one per vm, and you could use a provisioned vagrant environment for multiple tests, and there's no guarantee that you'd need exactly the same types for all the tests. What I think we should have instead is a tag system, that could be implemented as simply as:

config.vm.define "zookeeper-#{i}" do |zookeeper|
  zookeeper.provision "shell", inline: "echo zookeeper > /mnt/kafka/tags"
...
config.vm.define "server-#{i}" do |server|
  server.provision "shell", inline: "echo server,other_tag,its_a_csv > /mnt/kafka/tags"

please forgive my potentially broken Vagrantfile, but I hope the idea is clear.

Then in the remoteaccount.py:

# ...
self.tags = self._get_tags() if externally_routable_ip else []

def _get_tags(self):
  return self.ssh_output('cat /mnt/kafka/tags 2> /dev/null').split(',')

Then, your ClusterSpec can require machines with a specific set of tags and they won't be coupled to artificial constraints. Anything can be a zookeeper service if the person running the test tags it with "zookeeper".

There will (/may) be tests that actually require specific hardware configurations, but in my experience it's quite rare. Additionally, intent ("I need to run zookeeper on this machine") is inherently different than technical specs ("this machine has 100 GB of RAM") so rather than try to write one system that anticipates both right now, we can pretend that we don't need to worry about actual machine specs in the ClusterSpec, because as I understand it we don't.

[criccomini]

😩 can you guys get on the same page? 😢

I'm unclear whether this comment is just your opinion, or whether it's actually the team's preference. This is not what we've been discussing with @ewen. We are spending developer $ to work on this, but we need to not flail like this. Please get together and come up with what the heck you want us to do, and then let us know. If it's machine tags, fine. If it's hardware tags, fine.

Again, please clarify whether you have appropriate buy-in on your side for this approach, or whether this is just off-the-cuff thoughts from you.

@mohnishbasha @ewencp @jspong

[jspong]

Sorry; like I said, I’m new here. Those were just my thoughts. I will sync with @ewen and we’ll come to consensus so there should be no more mixed signals.

[criccomini]

@jspong @ewencp @mohnishbasha any update?

[criccomini]

Anyone?

[criccomini]

@jspong @ewencp @mohnishbasha halp

[criccomini]

Forked here for those interested:

https://github.com/wepay/ducktape

[cla-assistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}