feature: Add podgroups statistics #3751

JesseStutler · 2024-09-26T09:07:41Z

fix #3597, related proposal pr: #3750

Implementation

In syncQueue of queue controller, concurrently using UpdateStatus will cause conflict, use ApplyStatus instead, and need to add patch permission of vc-controller for patch/status. And we record the statistics of podgroups in each state in queue as metrics to be exported outside, we don't update the statistics of podgroups in queue's status now.
vcctl get -n [name] and vcctl list display the statistics of podgroups in each state from queue's status directly, instead we do one more step, query the podgroups owend in the queue, stat the counts of podgroups in each state at the vcctl side, and then display them.

Verification

vcctl list
- When there are no podgroups in queue, list is normal:
- When there are podgroups in queue, list can also get the statistics of podgroups:
vcctl get
- get is normal:
metrics

Monokaix · 2024-09-27T01:49:30Z

cmd/controller-manager/app/server.go

@@ -177,3 +188,10 @@ func isControllerEnabled(name string, controllers []string) bool {
 	// if we get here, there was no explicit inclusion or exclusion
 	return hasStar
 }
+
+func promHandler() http.Handler {


This is duplicated with the func in scheduler.

Monokaix · 2024-09-27T01:54:04Z

installer/helm/chart/volcano/templates/controllers.yaml

@@ -58,7 +58,7 @@ rules:
    verbs: ["get", "create", "delete", "update"]
  - apiGroups: ["scheduling.incubator.k8s.io", "scheduling.volcano.sh"]
    resources: ["podgroups", "queues", "queues/status"]
-    verbs: ["get", "list", "watch", "create", "delete", "update"]
+    verbs: ["get", "list", "watch", "create", "delete", "update", "patch"]


Is update still need?

hwdef

Is there something I'm missing? Where is the logic for deleting the update queue status

hwdef · 2024-09-30T06:44:24Z

cmd/controller-manager/app/options/options.go

@@ -37,6 +37,7 @@ const (
 	defaultMaxRequeueNum       = 15
 	defaultSchedulerName       = "volcano"
 	defaultHealthzAddress      = ":11251"
+	defaultListenAddress       = ":8080"


Would a different port be better?

Make a distinction with the scheduler port.

Port 8080 is too common and prone to conflicts.

JesseStutler · 2024-10-08T12:30:14Z

Is there something I'm missing? Where is the logic for deleting the update queue status

Sorry I don't get the point, you mean that when the queue is deleted, the metrics need also be deleted, right?

hwdef · 2024-10-09T03:18:45Z

Sorry I didn't express myself clearly.
Our previous discussion was not to update some states of the queue, such as the number of pending and running in the queue.
So should we delete this part of the code?

JesseStutler · 2024-10-09T03:35:37Z

Oh yes now the pending/running/unknown/inqueue/completed pg statistics won't be persisted into etcd, see here:

queueStatusApply := v1beta1apply.QueueStatus().WithState(queueStatus.State).WithAllocated(queueStatus.Allocated)
	queueApply := v1beta1apply.Queue(queue.Name).WithStatus(queueStatusApply)
	if _, err := c.vcClient.SchedulingV1beta1().Queues().ApplyStatus(context.TODO(), queueApply, metav1.ApplyOptions{FieldManager: controllerName}); err != nil {
		klog.Errorf("Failed to apply status of Queue %s: %v.", queue.Name, err)
		return err
	}

You can see above that the data called queueStatusApply which need to update is only state and allocated resource, won't update those statistics now.

Monokaix · 2024-10-26T01:16:49Z

pkg/cli/queue/util.go

+	Completed int
+}
+
+func (pgStats *PodGroupStatistics) StatPodGroupCountsForQueue(pg *v1beta1.PodGroup) {


It's a pg resource but in cli/queue pkg.

Monokaix · 2024-10-26T01:17:22Z

pkg/controllers/queue/queue_controller_handler.go

@@ -19,6 +19,7 @@ package queue
 import (
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/klog/v2"
+	"volcano.sh/volcano/pkg/controllers/metrics"


Please sort import.

Monokaix · 2024-10-26T01:20:23Z

pkg/controllers/queue/queue_controller_action.go

 		}
 	}

+	// Update the metrics


Can wrap a func.

Monokaix · 2024-10-28T01:33:44Z

pkg/controllers/queue/queue_controller_util.go

@@ -38,3 +43,11 @@ func IsQueueReference(ref *metav1.OwnerReference) bool {

 	return true
 }
+
+func UpdateQueueMetrics(queueName string, queueStatus *schedulingv1beta1.QueueStatus) {


Put these to metrics pkg is better.

Monokaix · 2024-10-28T01:39:17Z

pkg/controllers/queue/queue_controller_action.go

 	klog.V(4).Infof("Begin to sync queue %s.", queue.Name)
 	defer klog.V(4).Infof("End sync queue %s.", queue.Name)

 	podGroups := c.getPodGroups(queue.Name)
-	queueStatus := schedulingv1beta1.QueueStatus{}
+	newQueueStatus := schedulingv1beta1.QueueStatus{}


Do we need deepCopy from old queue or we may miss some fields?

Monokaix · 2024-10-29T07:26:54Z

/ok-to-test
/lgtm

…up to vc-controller Signed-off-by: JesseStutler <[email protected]>

hwdef

/lgtm

Monokaix · 2024-11-21T07:19:17Z

/approve

volcano-sh-bot · 2024-11-21T07:19:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Monokaix

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~~ [Monokaix]

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

volcano-sh-bot requested review from hzxuzhonghu and yuanchen8911 September 26, 2024 09:07

volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 26, 2024

JesseStutler force-pushed the czc_dev branch from a91a112 to 9315036 Compare September 26, 2024 13:07

Monokaix reviewed Sep 27, 2024

View reviewed changes

hwdef reviewed Sep 30, 2024

View reviewed changes

JesseStutler mentioned this pull request Oct 8, 2024

feat: add volcano jobs phase metric #3650

Merged

JesseStutler force-pushed the czc_dev branch 5 times, most recently from 8b51344 to 996a0de Compare October 25, 2024 09:49

volcano-sh-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 25, 2024

JesseStutler force-pushed the czc_dev branch 2 times, most recently from b8d8998 to f0bddff Compare October 25, 2024 14:23

Monokaix reviewed Oct 26, 2024

View reviewed changes

pkg/controllers/queue/queue_controller_action.go

}

}

// Update the metrics

Copy link

Member

Monokaix Oct 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can wrap a func.

JesseStutler force-pushed the czc_dev branch 2 times, most recently from 39756cb to 6e7924f Compare October 26, 2024 03:13

Monokaix reviewed Oct 28, 2024

View reviewed changes

JesseStutler force-pushed the czc_dev branch from 6e7924f to 59ce875 Compare October 28, 2024 12:32

volcano-sh-bot assigned Monokaix Oct 29, 2024

volcano-sh-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 29, 2024

Monokaix mentioned this pull request Nov 13, 2024

Controller reports "podgroup not found" repeatedly for SyncQueue action #3805

Closed

JesseStutler mentioned this pull request Nov 15, 2024

Request to be a member of Volcano community volcano-sh/community#62

Closed

JesseStutler force-pushed the czc_dev branch from 59ce875 to 626cae3 Compare November 15, 2024 06:15

volcano-sh-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 15, 2024

JesseStutler force-pushed the czc_dev branch 3 times, most recently from 40e1f3f to ddd1d8f Compare November 15, 2024 09:34

feature: add podgroup statistics and move metrics related with podgro…

f14999c

…up to vc-controller Signed-off-by: JesseStutler <[email protected]>

JesseStutler force-pushed the czc_dev branch from ddd1d8f to f14999c Compare November 15, 2024 09:52

JesseStutler requested a review from Monokaix November 15, 2024 13:20

hwdef reviewed Nov 18, 2024

View reviewed changes

volcano-sh-bot assigned hwdef Nov 18, 2024

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 18, 2024

volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2024

volcano-sh-bot merged commit da761e2 into volcano-sh:master Nov 21, 2024
17 checks passed

JesseStutler mentioned this pull request Dec 2, 2024

[Enhancement]Optimize volcano end-to-end scheduling large-scale pod performance #3852

Open

6 tasks

jxs1211 mentioned this pull request Dec 9, 2024

update pg status with applyStatus #3865

Open

JesseStutler mentioned this pull request Dec 23, 2024

queue.status.Running do not increased when create vcjob and pod is running successed. #3911

Closed

JesseStutler mentioned this pull request Feb 11, 2025

Request to be an Reviewer of Volcano community volcano-sh/community#66

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Add podgroups statistics #3751

feature: Add podgroups statistics #3751

JesseStutler commented Sep 26, 2024 •

edited

Loading

Monokaix Sep 27, 2024

Monokaix Sep 27, 2024

hwdef left a comment

hwdef Sep 30, 2024

JesseStutler commented Oct 8, 2024

hwdef commented Oct 9, 2024

JesseStutler commented Oct 9, 2024

Monokaix Oct 26, 2024

Monokaix Oct 26, 2024

Monokaix Oct 26, 2024

Monokaix Oct 28, 2024

Monokaix Oct 28, 2024

Monokaix commented Oct 29, 2024

hwdef left a comment

Monokaix commented Nov 21, 2024

volcano-sh-bot commented Nov 21, 2024

feature: Add podgroups statistics #3751

feature: Add podgroups statistics #3751

Conversation

JesseStutler commented Sep 26, 2024 • edited Loading

Implementation

Verification

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwdef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesseStutler commented Oct 8, 2024

hwdef commented Oct 9, 2024

JesseStutler commented Oct 9, 2024

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

Monokaix commented Oct 29, 2024

hwdef left a comment

Choose a reason for hiding this comment

Monokaix commented Nov 21, 2024

volcano-sh-bot commented Nov 21, 2024

JesseStutler commented Sep 26, 2024 •

edited

Loading