Flavour Aggregation #83
Conversation
|
Hi @Iqqdd99 . Thanks for your PR! It looks like some checks have failed, more specifically the artifact generation one. This happens when you modify the CRDs without building them through make (run Moreover, I see you pushed 4 commits and I would kindly ask you to squash your commit when opening PR so we can keep the history of the repository clean. Thanks! |
| @@ -137,6 +137,9 @@ type FlavourSpec struct { | |||
| // This field is used to specify the optional fields that can be retrieved from the Flavour. | |||
| // In the future it will be expanded to include more optional fields defined in the REAR Protocol or custom ones. | |||
| OptionalFields OptionalFields `json:"optionalFields"` | |||
|
|
|||
| // This field is used to represent the available quantity of transactions of the Flavour. | |||
| QuantityAvailable int `json:"quantityAvailable,omitempty"` | |||
There was a problem hiding this comment.
What is the meaning of this field? Why do we need it?
There was a problem hiding this comment.
This paramenter is just a counter of the number of resources agregated. If the flavours are going to be an interface to the exterior it should be able to have information of how much resources are available.
There was a problem hiding this comment.
Ok, now I know what it is about but I would like to correct the comment as it goes as "available quantity of transactions" which is not clear what it means.
Nevertheless, I still don't understand who need this parameter, why and how.
Is it really well designed? What if a consumer want to buy a partition of this aggregation which is not a multiple of the original nodes of the provider's cluster?
I feel we need to talk about this better and deeper as it could be a braking change for the current and future architecture.
There was a problem hiding this comment.
Yes, we need to have a deep conversation about how the flavour will behave when partitioned.
| @@ -38,6 +40,108 @@ import ( | |||
| // resources calculation. | |||
|
|
|||
| // Start starts the controller. | |||
|
|
|||
| func canMergeFlavours(flavour1, flavour2 *nodecorev1alpha1.Flavour) bool { | |||
There was a problem hiding this comment.
Is this method meant for all kind of Flavors? I believe that it should be just used for specific Flavor types like VMs or K8s clusters.
There was a problem hiding this comment.
So I would add a check on the Flavor type at the beginning of the method to address this concern.
There was a problem hiding this comment.
We need to define the different types of Aggregation to address all the possible use cases.
Spec.Type is already checked in the method.
There was a problem hiding this comment.
Well, I didn't mean that.
As we're going to provide SaaS as flavours, those cannot be aggregated like hardware resources.
So, when I talk about "check on the flavor Type at the beginning", I mean we should allow the aggregation only to specific resource types (VMs, K8s-clusters). Not only when flavor1 and flavo2 have the same type.
There was a problem hiding this comment.
I think that we need to add a field which refers to the specific resource type and set it in the Flavour definition.
| @@ -38,6 +40,108 @@ import ( | |||
| // resources calculation. | |||
|
|
|||
| // Start starts the controller. | |||
|
|
|||
| func canMergeFlavours(flavour1, flavour2 *nodecorev1alpha1.Flavour) bool { | |||
There was a problem hiding this comment.
Also, I'm not understanding why we have those constraints. Can't we merge two flavors of the same type if those have different characteristics (like CPU, RAM, etc)? Why so?
There was a problem hiding this comment.
In this first approach the main focus was to just aggregate nodes with the same specifications because currently, the information stored in flavours is the same stored in nodes.
There was a problem hiding this comment.
Yes I understood your point.
Does this mean that if a provider has nodes with different characteristics (like CPU and RAM) it cannot aggregate the corresponding flavors?
If so, what's the meaning of the aggregation eventually?
There was a problem hiding this comment.
Yes, at the moment.
The meaning of the flavour aggregation is to have an abstraction of all the available resources to be consumed. This will allow us to have a cleaner list of the resouerces and also a much tighter control of what is characteristics are published.
| for len(flavours) > 0 { | ||
| mergedFlavour := flavours[0] | ||
| mergedFlavour.Spec.QuantityAvailable = 1 | ||
|
|
There was a problem hiding this comment.
If we want to support Aggregation, what do we mean by that?
I thought we needed to combine different flavors into a big one.
E.g. Aggregated Flavor CPU = Flavor 1 CPU + Flavor 2 CPU; Aggregated Flavor RAM = Flavor 1 RAM + FLAVOR 2 RAM, etc.
Am I missing something?
There was a problem hiding this comment.
The aggregation of heterogeneus resources could be another kind of aggregation but we need to specify and design it.
There was a problem hiding this comment.
What do you mean by "heterogeneous resources"?
Resources of the same "type" but with different characteristics? Or resources of different type?
There was a problem hiding this comment.
Resources of the same type but different characteristics.
There was a problem hiding this comment.
Hi @Iqqdd99 .
Thanks for solving the pipeline issue, but I still see you have 5 commits.
I kindly ask you to squash them into a single one, contact me if you need support.
Moreover, I recommend to fetch updates form the upstream repo as we put some modifications recently.
Additionally, after performing a review, I saw some changes that are not very clear to me.
Could you try to provide an explanation or to fix the code if needed?
Thanks!
Added logic for node aggregation into flavours.