Skip to content
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

Add number of AdmittedWorkloads to LocalQueue status #259

Closed
1 of 3 tasks
alculquicondor opened this issue May 12, 2022 · 14 comments · Fixed by #382
Closed
1 of 3 tasks

Add number of AdmittedWorkloads to LocalQueue status #259

alculquicondor opened this issue May 12, 2022 · 14 comments · Fixed by #382
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. kind/ux

Comments

@alculquicondor
Copy link
Contributor

What would you like to be added:

A field containing the number of active workloads.

We abandoned this idea earlier because the cache was not queue aware. But we already need to make it queue aware for the purpose of metrics #199

Why is this needed:

Improve UX around observability

/kind ux

Completion requirements:

This enhancement requires the following artifacts:

  • Design doc
  • API change
  • Docs update

The artifacts should be linked in subsequent comments.

@alculquicondor alculquicondor added the kind/feature Categorizes issue or PR as related to a new feature. label May 12, 2022
@nayihz
Copy link
Contributor

nayihz commented May 23, 2022

The active workloads are the workloads which have been admitted by scheduler. Do I understand it correctly?

newWorkload.Spec.Admission = admission
if err := s.cache.AssumeWorkload(newWorkload); err != nil {
return err
}
log.V(2).Info("Workload assumed in the cache")

@alculquicondor
Copy link
Contributor Author

not assumed, but actually part of the cache. But please don't work on this yet. I'm working on a change to add the metric with the same data. We can reuse that code to include it in the status.

@alculquicondor
Copy link
Contributor Author

I did some progress on this #259. After it merges, feel free to take over.

@ahg-g ahg-g changed the title Add number of active workloads to Queue status Add number of AdmittedWorkloads LocalQueue status Sep 1, 2022
@ahg-g
Copy link
Contributor

ahg-g commented Sep 1, 2022

This would be similar to the AdmittedWorkloads in ClusterQueue

@ahg-g
Copy link
Contributor

ahg-g commented Sep 1, 2022

/assign @kannon92

@k8s-ci-robot
Copy link
Contributor

@ahg-g: GitHub didn't allow me to assign the following users: kannon92.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @kannon92

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kannon92
Copy link
Contributor

kannon92 commented Sep 1, 2022

/assign @kannon92

@ahg-g ahg-g changed the title Add number of AdmittedWorkloads LocalQueue status Add number of AdmittedWorkloads to LocalQueue status Sep 1, 2022
@kannon92
Copy link
Contributor

kannon92 commented Sep 1, 2022

I am getting up to speed in this repo.

AdmittedWorkloads is the same as ActiveWorkloads?

@ahg-g
Copy link
Contributor

ahg-g commented Sep 1, 2022

yes, we are using the term admitted now.

@kannon92
Copy link
Contributor

kannon92 commented Sep 2, 2022

Hello. So I made progress on this but I realize I'm unclear on the ask. The issue reads very simple as add a new field.

So here is my research:

  1. Add a new field for AdmittedWorkloads in Apis
  2. Update the CRD to reflect the new field

If I read the ticket exactly, then I believe my work is done?

Is the intent that future features will update this API field? Or should I also tackle this as part of the issue?

@ahg-g
Copy link
Contributor

ahg-g commented Sep 2, 2022

Not only add the field, we need to do populate it as well. Kueue is the one responsible for updating the status of both LocalQueue and ClusterQueue. Take a look at the AdmittedWorkloads field in ClusterQueue and how we populate it as an example.

@kannon92
Copy link
Contributor

kannon92 commented Sep 6, 2022

Sounds good! I am making progress on this. Do we want these fields as added into metrics also?

@alculquicondor
Copy link
Contributor Author

No, we actually did that initially and reverted the change #293.

Just the status field should be good.

@kannon92
Copy link
Contributor

kannon92 commented Sep 8, 2022

Alright. I think I have something for you all to review. #382

Sorry about the spamming the repo with a few opens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. kind/ux
Projects
None yet
5 participants