Conversation
RunnerSet is basically ReplicaSet for Runners. It is responsible for maintaining number of runners to match the desired one. That is, it creates missing runners from `.Spec.Template` and deletes redundant runners. Similar to ReplicaSet, this does not support rolling update of runners on its own. We might want to later add `RunnerDeployment` for that. But that's another story.
Adds the initial version of RunnerDeployment that is intended to manage RunnerSets(actions#1), like Deployment manages ReplicaSets. This is the initial version and therefore is bare bone. The only update strategy it supports is `Recreate`, which recreates the underlying RunnerSet when the runner template changes. I'd like to add `RollingUpdate` strategy once this is merged. This depends on actions#1 so the diff contains that of actions#1, too. Please see only the latest commit for review. Also see https://github.com/mumoshu/actions-runner-controller-ci/runs/471329823?check_suite_focus=true to confirm that `make tests` is passing after changes made in this commit.
summerwind
left a comment
summerwind
left a comment
There was a problem hiding this comment.
Thank you for the PR! I really like this 👍
This is a good starting point for a mechanism that makes Runner easier to scale.
Overall, there seems to be almost no problem, but I made some comments to improve some codes.
As discussed face-to-face, I'm considering to use StatefulSet to manage replica of runners. I'm not sure if we actually use this implementation at this time. 😉However, I'd like to merge this PR to compare the way of the implementation.
| if err := r.List(ctx, &allRunnerSets, client.InNamespace(req.Namespace)); err != nil { | ||
| if !errors.IsNotFound(err) { | ||
| return ctrl.Result{}, err | ||
| } |
There was a problem hiding this comment.
I think that this is not needed.
List interface will return empty list instead of 'NotFound' error.
| if metav1.IsControlledBy(&rs, &rd) { | ||
| myRunnerSets = append(myRunnerSets, &rs) | ||
| } | ||
| } |
There was a problem hiding this comment.
It might be better to use 'Field Indexer' to get related RunnerSets.
You can see an example of Field Indexer in the kubebuilder book:
https://book.kubebuilder.io/cronjob-tutorial/controller-implementation.html?highlight=field,indexer#2-list-all-active-jobs-and-update-the-status
There was a problem hiding this comment.
Thanks! I've never realized that controller-runtime had such a facility. Addressed in 31fb7cc
Enhances the deployment controller to use indexed lookups against runner sets for more scalability.
Removes an unnecessary condition from the deployment controller code. We assumed that the client would return a not-found error on an empty runnerset list it is clearly not the case.
2 participants
Adds the initial version of RunnerDeployment that is intended to manage RunnerSets(#1), like Deployment manages ReplicaSets.
This is the initial version and therefore is bare bone. The only update strategy it supports is
Recreate, which recreates the underlying RunnerSet when the runner template changes. I'd like to addRollingUpdatestrategy once this is merged.This depends on #1 so the diff contains that of #1, too. Please see only the latest commit for review.
Also see https://github.com/mumoshu/actions-runner-controller-ci/runs/471329823?check_suite_focus=true to confirm that
make testsis passing after changes made in this commit.