Introduce Actions; fully support multi-node with n>0 workers #164
Conversation
k8s-ci-robot
added
cncf-cla: yes
| // task to be executed | ||
| Task Task | ||
| // node where the task should be executed | ||
| Node *config.Node |
There was a problem hiding this comment.
PlannedTask has a Node reference, but also a Task and this Task holds a list of Nodes.
ideally an Action should have a list of Tasks.
and a Task should execute on a list of Nodes.
we should think of how to omit the PlannedTasks object.
There was a problem hiding this comment.
@neolit123 The distinction between Task and PlannedTask is the key to manage variable list of actions in a way that can adapt to different cluster topologies
Task are defined at programming time - before knowing the cluster topology -, and they have a NodeSelector function, not a list of nodes.
PlannedTask is the "runtime version" of a Task, that is created when assigning a "logical task" to a Node in the actual cluster topology (a node that matches NodeSelector criteria).
| // NB. we are using a string to combine all the item considered into something | ||
| // that can be easily sorted using a lexicographical order | ||
| func (p *PlannedTask) ExecutionOrder() string { | ||
| return fmt.Sprintf("Node.ProvisioningOrder: %d - Node.Name: %s - actionIndex: %d - taskIndex: %d", |
There was a problem hiding this comment.
i might need a diagram to better understand how this work j/k.
in all seriousness, i think the sorting introduces a non-deterministic factor that could be avoided.
some questions:
- why do we even need the sorting?
- what if we want to execute tasks with an order not based on the node names?
There was a problem hiding this comment.
why do we even need the sorting?
what if we want to execute tasks with an order not based on the node names?
Sorting is used to provide a predictable, "kubeadm friendly" and consistent task exection order that can automatically adapt to any requested list of actions and to any cluster topology supported by kind.
Such order is not based on the node name only, but on a combination of 4 attributes considered with a well known priority: node provisioning order, node name, action order, task order (I improved comments to make this more clear).
By combining above parameters you get a task execution order consistent with all the major kubeadm workflows (init, join control-plane, join) or (upgrade apply, upgrade node control-plane, upgrade node).
See discussion below with @ereslibre about implementation details, but in any case the ordering is 100% deterministic and it is already covered by unit tests signal.
As noted in NewExecutionPlan comments, I already know that this won't be enough for complex CI workflows, but this is a good step in that direction and this is why I'm putting order into this PR.
|
might have been better if we have a discussion over Zoom about this first. |
| // lower element should be executed before the other. | ||
| // It is required for making ExecutionPlan sortable. | ||
| func (t ExecutionPlan) Less(i, j int) bool { | ||
| return t[i].ExecutionOrder() < t[j].ExecutionOrder() |
There was a problem hiding this comment.
Isn't it a bit strange to relay on string sorting here given that ExecutionOrder() returns a string? Could we have a utility function that takes into account the node ProvisioningOrder() and the actionIndex and taskIndex? https://play.golang.org/p/z8ZGpyl_EPt
Maybe it's minor as long as the order is always preserved.
There was a problem hiding this comment.
the order is always preserved
The ordering is 100% deterministic, and this is covered by unit tests as well
Nevertheless, I'm ready to change implementation if this one seems counter-intuitive, but I'm not 100% sure the alternative reads better. see https://gist.github.com/fabriziopandini/e544842b8c38c90974b215167a77d934
There was a problem hiding this comment.
I'm going to simplify actions quite a bit in a follow-up O(very soon)
|
@neolit123 @ereslibre thanks for the round of feedbacks! @neolit123 of course ok for me to discuss this in zoom; might be we want to wait for next week to get more people involved, but up to you |
SGTM. thanks for the replies. |
|
xref: #147 (comment) |
|
I've rebased this to fix merge conflicts, in the process I did break controlplane meta, but will fix that after this merges in the general bikeshedding that will ensue 🙃 |
|
/retest |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, fabriziopandini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
…rawsadm Avoid creating bootstrap user/group for EKS
This PR introduces actions in
kindActions consist of set of repeatable tasks to be executed on a
kindcluster.Usage of actions allows defining composable, high-level workflows that can automatically adapt to different cluster topologies.
In order to make this more concrete, the first set of action supporting multi-node with n>0 workers was implemented in this PR.
Fixes: #131
Note for reviewers
This PR depends on #147; please consider only last commit
/cc @BenTheElder @neolit123
@rdodev