feat: automatic approval with capacity management #288
Conversation
MatousJobanek
requested review from
tinakurian,
rajivnathan and
alexeykazakov
| // capacity thresholds and the actual use if there is any suitable member cluster. | ||
| func getClusterIfApproved(cl client.Client, crtConfig *crtCfg.Config, userSignup *toolchainv1alpha1.UserSignup, getMemberClusters cluster.GetMemberClustersFunc) (bool, string, error) { | ||
| config := &toolchainv1alpha1.HostOperatorConfig{} | ||
| if err := cl.Get(context.TODO(), types.NamespacedName{Namespace: userSignup.Namespace, Name: "config"}, config); err != nil { |
There was a problem hiding this comment.
Well.. I guess we don't want to explicitly load the host operator config in every place where we need the configuration.
Can we abstract access to the config?
Something like config.HostOperatorConfig() function somewhere. And reuse this function everywhere we need access to the config. The function for now could load the corresponding "config" resource but later we can convert it to a controller which will watch the "config" resource and update the internal cache if it's changed. Not sure if we really need such cache though. Maybe the go client cache is already good enough. But we can think about that cache later.
There was a problem hiding this comment.
Yes yes yes, I had the very same concern when I was writing the code.
Actually, I started working on a controller that does exactly the same what you proposed with the cache 😄, I just want to submit it as a separated PR and when this automatic approval is done. See this: https://github.com/MatousJobanek/host-operator/tree/hostoperatorconfig-controller/pkg/controller/hostoperatorconfig
For now, I decided to load it here and then replace it with calling the cache.
There was a problem hiding this comment.
OK. Makes sense. Let's keep it for a separate PR then!
MatousJobanek
force-pushed
the
automatic-approval
branch
from
a3d05f0
to
47aee03
Compare
Co-authored-by: Tina Kurian <tkurian@redhat.com>
There was a problem hiding this comment.
looks great @MatousJobanek thanks for addressing my comments!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, rajivnathan, tinakurian 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 |
Codecov Report
@@ Coverage Diff @@
## master #288 +/- ##
=========================================
Coverage ? 83.58%
=========================================
Files ? 25
Lines ? 2151
Branches ? 0
=========================================
Hits ? 1798
Misses ? 275
Partials ? 78
|
…n#288) * remove minishft support in makefile * updated readme to remove minishift reference * remove login-as-admin * PR suggestions added * added indentation * updated indentation * if indentation updated
Introduces automatic approval connected to capacity management.
The logic checks if the user can be approved and provisioned to any member cluster. It the user can be approved then it picks a suitable member cluster the user will be provisioned to.
If the user is approved manually then if the target cluster is not already specified for UserSignup then it just gets a member cluster with enough capacity.
If the user is not approved manually, then it loads HostOperatorConfig to check if automatic approval is enabled or not. If it is then it checks capacity thresholds and the actual use to check if there is any suitable member cluster available.
If there is no suitable member cluster (but the user can be approved), then it returns an error and waits for the next reconcile.
Builds on top of #290 so the only commit to be reviewed is: 47aee03
TODO:
Postponed for separated PR:
when automatic approval is enabled then it should approve and provision the oldest unapproved UserSignup.