Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce Actions; fully support multi-node with n>0 workers #164
Introduce Actions; fully support multi-node with n>0 workers #164
Changes from all commits
7f095d4
4d6abfe
f467af0
9532774
c6f53d1
91c7a92
1371151
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neolit123 The distinction between
Task
andPlannedTask
is the key to manage variable list of actions in a way that can adapt to different cluster topologiesTask
are defined at programming time - before knowing the cluster topology -, and they have aNodeSelector
function, not a list of nodes.PlannedTask
is the "runtime version" of aTask
, that is created when assigning a "logical task" to a Node in the actual cluster topology (a node that matchesNodeSelector
criteria).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it a bit strange to relay on string sorting here given that
ExecutionOrder()
returns astring
? Could we have a utility function that takes into account the nodeProvisioningOrder()
and theactionIndex
andtaskIndex
? https://play.golang.org/p/z8ZGpyl_EPtMaybe it's minor as long as the order is always preserved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to simplify actions quite a bit in a follow-up O(very soon)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.