Track usage in capacity status #57

alculquicondor · 2022-02-23T22:25:28Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

Add new status fields
Reconcile when there are QueuedWorkload updates

This is important for the observability of kueue operations

Which issue(s) this PR fixes:

Fixes #7

Special notes for your reviewer:

I'm putting a minimal check in the integration test. I plan to add a more comprehensive test as part of #68

k8s-ci-robot · 2022-02-23T22:25:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor

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:

~~OWNERS~~ [alculquicondor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ArangoGutierrez

testing it locally, so far looks good, for now just one nit

config/manager/kustomization.yaml

alculquicondor · 2022-02-24T22:32:29Z

/hold
(because I don't want a single LGTM to cause a merge of the PR)

/assign @ahg-g @ArangoGutierrez

denkensk

some comments.

denkensk · 2022-02-25T03:23:55Z

api/v1alpha1/capacity_types.go

+
+	// runningWorkloads is the number of workloads currently assigned to this
+	// capacity.
+	RunningWorkloads int32 `json:"runningWorkloads,omitempty"`


Running is a little confuse. Actually the workload is just assigned.

RunningWorkloads --> AssignedWorkloads

yes... not sure what I was thinking.

denkensk · 2022-02-25T03:27:28Z

pkg/capacity/capacity.go

+// Usage reports the used resources and number of workloads assigned to the
+// capacity.
+func (c *Cache) Usage(capObj *kueue.Capacity) (map[corev1.ResourceName]map[string]kueue.Usage, int, error) {
+	c.Lock()


We can choose to use RWLock here?

yes, it makes sense now that there are more than one reader

denkensk · 2022-02-25T03:44:27Z

test/integration/scheduler/scheduler_test.go

+			if err := k8sClient.Get(ctx, types.NamespacedName{Name: "prod-capacity"}, &capObj); err != nil {
+				return false
+			}
+			return capObj.Status.RunningWorkloads == 1


Also check the usage？

Will do as part of #68

denkensk · 2022-02-25T03:56:53Z

pkg/controller/core/capacity_controller.go

+		q.Add(requestForWorkloadCapacity(oldW))
+	}
+	newW := e.ObjectNew.(*kueue.QueuedWorkload)
+	if newW.Spec.AssignedCapacity != "" && newW.Spec.AssignedCapacity != oldW.Spec.AssignedCapacity {


I'm curious about the circumstances under which this Spec.AssignedCapacity would change

It shouldn't happen... but I tried to make the code resilient to that.

I guess one case could be reassignment after preemption, if the user decides to change the queue for the workload, or the queue changes capacity 🤷

ArangoGutierrez

tested locally and works, just a few comments

ArangoGutierrez · 2022-02-25T12:12:32Z

pkg/controller/core/capacity_controller.go

 }

 // SetupWithManager sets up the controller with the Manager.
 func (r *CapacityReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&kueue.Capacity{}).
+		Watches(&source.Kind{Type: &kueue.QueuedWorkload{}}, &assignedWorkloadHandler{}).


Suggested change

Watches(&source.Kind{Type: &kueue.QueuedWorkload{}}, &assignedWorkloadHandler{}).

Watches(&source.Kind{Type: &kueue.QueuedWorkload{}}, &assignedWorkloadHandler{}).

WithLogger(r.log).

I tried that before, but the manager adds extra names... so it looks like this:

capacity-reconciler.controller.capacity

Better just have controller.capacity to distinguish from the event handler logs.

Maybe I should rename the handler log to capacity-handler?

ArangoGutierrez · 2022-02-25T12:14:51Z

pkg/controller/core/capacity_controller.go

+		log.Error(err, "Failed getting usage from capacity cache")
+		// This is likely because the capacity was recently removed and we didn't
+		// process that event yet.
+		return ctrl.Result{}, err


Maybe in this scenario we want to requeue?

// Requeue tells the Controller to requeue the reconcile key. Defaults to false. Requeue bool

to trigger a new try to reconcile the state of the capacity?

No, because we should receive the event, which will put the Capacity in the workqueue.

Although it doesn't really matter, there is nothing to do if the capacity is deleted.

api/v1alpha1/capacity_types.go

pkg/capacity/capacity_test.go

pkg/controller/core/capacity_controller.go

ahg-g · 2022-02-25T21:46:57Z

Looks good to me if you want to squash, i can lgtm if there are no pending comments from others

ArangoGutierrez

/lgtm

- Add new status fields - Reconcile when there are QueuedWorkload updates Change-Id: I44b96a286f9871a76e5657f1dbb1b4b31738c2f4

alculquicondor · 2022-02-28T14:42:24Z

rebased

ahg-g · 2022-02-28T16:32:52Z

/lgtm

ahg-g · 2022-02-28T16:33:44Z

/retest

why do we keep getting this failure?

alculquicondor · 2022-02-28T16:39:39Z

/hold cancel

k8s-ci-robot requested review from ArangoGutierrez and denkensk February 23, 2022 22:25

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 23, 2022

alculquicondor force-pushed the capacity_status branch 2 times, most recently from 05ef630 to fe181b6 Compare February 23, 2022 22:39

ArangoGutierrez reviewed Feb 23, 2022

View reviewed changes

config/manager/kustomization.yaml Outdated Show resolved Hide resolved

alculquicondor force-pushed the capacity_status branch from fe181b6 to bbd8503 Compare February 24, 2022 22:14

alculquicondor changed the title ~~WIP Track usage in capacity status~~ Track usage in capacity status Feb 24, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 24, 2022

alculquicondor force-pushed the capacity_status branch from bbd8503 to b091b97 Compare February 24, 2022 22:31

k8s-ci-robot assigned ahg-g Feb 24, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 24, 2022

k8s-ci-robot assigned ArangoGutierrez Feb 24, 2022

alculquicondor force-pushed the capacity_status branch 2 times, most recently from dbefd6f to 27c3569 Compare February 24, 2022 22:43

denkensk reviewed Feb 25, 2022

View reviewed changes

ArangoGutierrez reviewed Feb 25, 2022

View reviewed changes

ahg-g reviewed Feb 25, 2022

View reviewed changes

ArangoGutierrez reviewed Feb 25, 2022

View reviewed changes

pkg/controller/core/capacity_controller.go Show resolved Hide resolved

ArangoGutierrez reviewed Feb 25, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2022

Track usage in capacity status

a83f769

- Add new status fields - Reconcile when there are QueuedWorkload updates Change-Id: I44b96a286f9871a76e5657f1dbb1b4b31738c2f4

alculquicondor force-pushed the capacity_status branch from 05282f1 to a83f769 Compare February 28, 2022 14:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2022

k8s-ci-robot merged commit d74ccc6 into kubernetes-sigs:main Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track usage in capacity status #57

Track usage in capacity status #57

alculquicondor commented Feb 23, 2022 •

edited

Loading

k8s-ci-robot commented Feb 23, 2022

ArangoGutierrez left a comment

alculquicondor commented Feb 24, 2022

denkensk left a comment

denkensk Feb 25, 2022

alculquicondor Feb 25, 2022

denkensk Feb 25, 2022

alculquicondor Feb 25, 2022

denkensk Feb 25, 2022

alculquicondor Feb 25, 2022

denkensk Feb 25, 2022

alculquicondor Feb 25, 2022

ArangoGutierrez left a comment

ArangoGutierrez Feb 25, 2022

alculquicondor Feb 25, 2022

ArangoGutierrez Feb 25, 2022

alculquicondor Feb 25, 2022

ahg-g commented Feb 25, 2022

ArangoGutierrez left a comment

alculquicondor commented Feb 28, 2022

ahg-g commented Feb 28, 2022

ahg-g commented Feb 28, 2022

alculquicondor commented Feb 28, 2022

	Watches(&source.Kind{Type: &kueue.QueuedWorkload{}}, &assignedWorkloadHandler{}).
	Watches(&source.Kind{Type: &kueue.QueuedWorkload{}}, &assignedWorkloadHandler{}).
	WithLogger(r.log).

Track usage in capacity status #57

Track usage in capacity status #57

Conversation

alculquicondor commented Feb 23, 2022 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

k8s-ci-robot commented Feb 23, 2022

ArangoGutierrez left a comment

Choose a reason for hiding this comment

alculquicondor commented Feb 24, 2022

denkensk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArangoGutierrez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g commented Feb 25, 2022

ArangoGutierrez left a comment

Choose a reason for hiding this comment

alculquicondor commented Feb 28, 2022

ahg-g commented Feb 28, 2022

ahg-g commented Feb 28, 2022

alculquicondor commented Feb 28, 2022

alculquicondor commented Feb 23, 2022 •

edited

Loading