feat: add volcano jobs phase metric

[Prepmachine4]

fix: #2493

I add all job phase shows in job apis.

const (
	// Pending is the phase that job is pending in the queue, waiting for scheduling decision
	Pending JobPhase = "Pending"
	// Aborting is the phase that job is aborted, waiting for releasing pods
	Aborting JobPhase = "Aborting"
	// Aborted is the phase that job is aborted by user or error handling
	Aborted JobPhase = "Aborted"
	// Running is the phase that minimal available tasks of Job are running
	Running JobPhase = "Running"
	// Restarting is the phase that the Job is restarted, waiting for pod releasing and recreating
	Restarting JobPhase = "Restarting"
	// Completing is the phase that required tasks of job are completed, job starts to clean up
	Completing JobPhase = "Completing"
	// Completed is the phase that all tasks of Job are completed
	Completed JobPhase = "Completed"
	// Terminating is the phase that the Job is terminated, waiting for releasing pods
	Terminating JobPhase = "Terminating"
	// Terminated is the phase that the job is finished unexpected, e.g. events
	Terminated JobPhase = "Terminated"
	// Failed is the phase that the job is restarted failed reached the maximum number of retries.
	Failed JobPhase = "Failed"
)

The metric record event happend in jobInformer.Informer() received event.

cc.jobInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
	AddFunc:    cc.addJob,
	UpdateFunc: cc.updateJob,
	DeleteFunc: cc.deleteJob,
})

And the processing of record metrics is not use cache lock that maybe produces some inaccurate data but will improve some performance.

But if it traverses all jobs every time during updates, I think there might be some trouble. Should we switch to incremental updates or scheduled metrics at intervals?

[googs1025]

This implementation is too complex; we just need to add metrics at the state transition points.

[googs1025]

In practical scenarios, we only focus on the final states of success and failure, and we are not concerned with or do not need other states because they are not final states.

[googs1025]

You just need to find the place where the controller last modified the status in these two related places and make the changes.
status.State.Phase = vcbatch.Completed
status.State.Phase = vcbatch.Failed

[googs1025]

/assign

[Prepmachine4]

You just need to find the place where the controller last modified the status in these two related places and make the changes. status.State.Phase = vcbatch.Completed status.State.Phase = vcbatch.Failed

ok

[hwdef]

/ok-to-test

[Prepmachine4]

I tested the metrics, and they were in line with expectations.

[googs1025]

squash the commit to one

[Prepmachine4]

squash the commit to one

If i squash the commit to one, it seems like I need to re-create a PR, or is there another way?

[googs1025]

squash the commit to one

If i squash the commit to one, it seems like I need to re-create a PR, or is there another way?

Why do we need to resubmit a PR when using git rebase or git squash? This is to update your git log view locally, and has nothing to do with the PR itself.

[Prepmachine4]

squash the commit to one

If i squash the commit to one, it seems like I need to re-create a PR, or is there another way?

Why do we need to resubmit a PR when using git rebase or git squash? This is to update your git log view locally, and has nothing to do with the PR itself.

ok ,i have squashed the commit to one.

[hwdef]

/lgtm

[hwdef]

Where are these logs?

[Prepmachine4]

Where are these logs?

https://github.com/volcano-sh/volcano/actions/runs/11341604441?pr=3650

https://github.com/volcano-sh/volcano/actions/runs/11341604441/artifacts/2056794858

[hwdef]

So weird.
No problem compiling your code locally

[hwdef]

/close

[volcano-sh-bot]

@hwdef: Closed this PR.

In response to this:

/close

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.

[hwdef]

/reopen

[volcano-sh-bot]

@hwdef: Reopened this PR.

In response to this:

/reopen

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.

[hwdef]

Hi, thanks for your contributions! Please fix the CI

I tried to fix it, but I don't know why the 'default' queue is not open. Can you give me some advice on where I should start. .

Encountered this error again, It may be that volcano has just been deployed and the test process has been executed. The queue of volcano has not yet been opened.

Please add

sleep 30s

in

volcano/.github/workflows/e2e_spark.yaml

Line 68 in 320698e

minikube kubectl -- replace --force -f ./installer/volcano-development.yaml

target:

...
    - name: Build lastest volcano images
      run: |
        eval $(minikube docker-env)
        make TAG=latest update-development-yaml
        make TAG=latest images
        docker images | grep volcano
        cat ./installer/volcano-development.yaml  | grep image:
        minikube kubectl -- replace --force -f ./installer/volcano-development.yaml
        sleep 30s
...

[hwdef]

I made a mistake, the reason why the queue has no status is because the controller failed to start. Not for the reason I mentioned above

The reason for this is mentioned below. It seems that the controller does not use the latest code.

It seems that the issue is caused by the controller image used in the 'E2E Spark Integration Test' run not being the latest version.

[hwdef]

Maybe it’s because the pull policy of the image is always.

[Prepmachine4]

Maybe it’s because the pull policy of the image is always.

change to IfNotPresent?

[hwdef]

Yes, But I didn't think of a better replacement method. Please try add this

sed -i 's/imagePullPolicy: Always/imagePullPolicy: IfNotPresent/g' installer/volcano-development.yaml

at

volcano/.github/workflows/e2e_spark.yaml

Line 64 in 320698e

make TAG=latest update-development-yaml

target:

    - name: Build lastest volcano images
      run: |
        eval $(minikube docker-env)
        make TAG=latest update-development-yaml
        sed -i 's/imagePullPolicy: Always/imagePullPolicy: IfNotPresent/g' installer/volcano-development.yaml
        make TAG=latest images
        docker images | grep volcano
        cat ./installer/volcano-development.yaml  | grep image:
        minikube kubectl -- replace --force -f ./installer/volcano-development.yaml

Do you have any better suggestions?

[hwdef]

/lgtm

[JesseStutler]

Please see my review opinions also:

1. I think we should use gauge, not counter.
2. In command line, controller set the enable-metrics to false:
   fs.BoolVar(&s.EnableMetrics, "enable-metrics", false, "Enable the metrics function; it is false by default")
   but in helm chart template that --enable-metrics is written to be true, we should make this configurable, for example using {{ .Values.EnableMetrics}} and then add EnableMetrics:true in values.yaml
3. You should not delete the metrics in scheduler, scheduler does not have these metrics.

[Prepmachine4]

Please see my review opinions also:

1. I think we should use gauge, not counter.
2. In command line, controller set the enable-metrics to false:
   fs.BoolVar(&s.EnableMetrics, "enable-metrics", false, "Enable the metrics function; it is false by default")
   but in helm chart template that --enable-metrics is written to be true, we should make this configurable, for example using {{ .Values.EnableMetrics}} and then add EnableMetrics:true in values.yaml
3. You should not delete the metrics in scheduler, scheduler does not have these metrics.

Thank you for your suggestions. I have made changes to points 2 and 3, but I think using counter is more appropriate because these two metrics only have increment operations.

[hwdef]

/lgtm