kinvolk / lokomotive

🪦 DISCONTINUED Further Lokomotive development has been discontinued. Lokomotive is a 100% open-source, easy to use and secure Kubernetes distribution from the volks at Kinvolk
https://kinvolk.io/lokomotive-kubernetes/
Apache License 2.0
321 stars 49 forks source link

Consolidate on one name for count of pools #512

Open surajssd opened 4 years ago

surajssd commented 4 years ago

We have controller_count for controller pool and count for worker pools, to specify the number of nodes you need in those respective pools.

I think we should have consistency around here.

invidian commented 4 years ago

I'd say we should treat controllers as yet another worker pool, so we can re-use all the fields. Maybe under new field controllers_pool?

surajssd commented 4 years ago

IDK what the implications of such a change would be but good idea. We can use some golang struct embedding for that.

johananl commented 4 years ago

I'd say we should treat controllers as yet another worker pool, so we can re-use all the fields. Maybe under new field controllers_pool?

It doesn't make sense to me to treat controllers as worker pools since they aren't worker pools. Making the UX worse to make things nicer under the hood doesn't make sense to me. I'm fine with making things DRY-er under the hood but we should always think about the users first. We should also consider when things make sense at the implementation level vs. when we are trying to e.g. "force" some data structure to be something it's not. Again - better some duplication than wrong abstraction.

johananl commented 4 years ago

A thought regarding the naming of the "count" knobs: perhaps we don't need to consolidate these since they control things which are different in essence. Controllers have unique characteristics: they have an odd number, they are not identical (e.g. each controller is a different etcd member with different state), they are always stateful etc. Worker pools are a different beast entirely: you can have multiple pools, each pool can be of a different size etc.

Imagine you have no clue about the Lokomotive implementation. You're a user now. How you would expect things to behave?

What if we had something like the following?

cluster "stuff" {
  controller_count = 3

  worker_pool "pool" {
    size = 17
  }
}

"Size" could make sense especially when we have auto scaling since you typically have 3 size-related params:

Another tip which can help with naming is trying to put something in a sentence:

:heavy_check_mark: My cluster has a controller count of 3. :x: My cluster has a count of 3. :heavy_check_mark: The size of my worker pool is 17. :x: The count of my worker pool is 17. :confused: The worker count of my worker pool is 17.

Following the two :heavy_check_mark: sentences I can see that it feels natural to have a parameter called controller_count under cluster (because the cluster has a controller count of X) and a parameter called size under worker_pool (because the worker pool has a size of X).

iaguis commented 4 years ago

Making the UX worse to make things nicer under the hood doesn't make sense to me.

I wholeheartedly agree.

invidian commented 4 years ago

I was thinking about something like:

cluster "stuff" {
  controllers {
    size = 3
  }

  worker_pool "pool" {
    size = 17
  }
}

This gives the same experience for configuring the controllers as for the worker pools.