Implement a culling controller for Notebooks #6807
Conversation
google-oss-prow
bot
requested review from
elikatsis,
MartinForReal and
thesuperzapper
apo-ger
force-pushed
the
feature-apoger-notebook-controller-culling
branch
2 times, most recently
from
9d3f293
to
5ad3f07
Compare
apo-ger
force-pushed
the
feature-apoger-notebook-controller-culling
branch
from
5ad3f07
to
d61c5da
Compare
Changes: * Move the idleness/culling logic into a separate controller as part of the Notebooks Controller/Operator. * Introduce an "notebooks.kubeflow.org/last_activity_check_timestamp". annotation in each Notebook CR to keep the timestamp of the last performed idleness check The controller can then compare this timestamp with the current time to ensure that notebooks will get reconciled every IDLENESS_CHECK_PERIOD minutes. The culling-controller will: * reconcile only notebooks CRs * set/update culling annotations - 'notebooks.kubeflow.org/last_activity' - 'notebooks.kubeflow.org/last_activity_check_timestamp' * perform idleness checks every 'IDLENESS_CHECK_PERIOD' minutes and set the 'kubeflow-resource-stopped' annotation, if a notebook needs to be culled. Refs: kubeflow#6767 Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>
apo-ger
force-pushed
the
feature-apoger-notebook-controller-culling
branch
from
d61c5da
to
b4e0869
Compare
There was a problem hiding this comment.
@apo-ger left some comments. The code mostly LGTM!
I also tested the controller and indeed it's making the requests by respecting the actual check period. Nice work!
Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>
Add a log message at the beginning of the reconciliation loop to make it clear that a Reconcile was called for a notebook. Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>
apo-ger
force-pushed
the
feature-apoger-notebook-controller-culling
branch
from
63eae84
to
0a08ff6
Compare
* Introduce make rule for running the controller locally with culling enabled * Introduce a dev_culling_authorization_policy which must be applied when testing the culling-controller locally Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>
Signed-off-by: Apostolos Gerakaris <apoger@arrikto.com>
apo-ger
force-pushed
the
feature-apoger-notebook-controller-culling
branch
from
72f1318
to
91fe6b4
Compare
|
Thanks for this work @apo-ger! /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kimwnasptd 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 |
|
@kimwnasptd nice! Thank you for the review! |
|
Very good code using metav1.HasAnnotation returning either true or false, instead of checking for the presence of an annotation with an own logic block. I will integrate that in my PR as well, cool. @kimwnasptd Could you have a look at my PR? Took cues on annotations handling before this update from apo-gers code in the now-deleted culler helper. |
This PR splits the idleness/culling logic into a separate controller inside of the Notebooks Controller/Operator.
The culling-controller will:
notebooks.kubeflow.org/last_activitynotebooks.kubeflow.org/last_activity_check_timestampIDLENESS_CHECK_PERIODminutes and set thekubeflow-resource-stoppedannotation, if a notebook needs to be culled.Refs: #6767
Signed-off-by: Apostolos Gerakaris apoger@arrikto.com