Fix a bug that the PodGroupCtrl can not list priorityclass

[tenzen-y]

Currently, we don't pass a lister for priorityClasses to podGroup for the scheduler-plugins. So the nil pointer error happens when the podGroupCtrl for the scheduler-plugins list priorityClasses in the following:

E0607 07:29:50.611829       1 runtime.go:79] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
goroutine 437 [running]:
k8s.io/apimachinery/pkg/util/runtime.logPanic({0x19cf5a0?, 0x2d22520})
	/go/pkg/mod/k8s.io/apimachinery@v0.25.7/pkg/util/runtime/runtime.go:75 +0x99
k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0xc00081c620?})
	/go/pkg/mod/k8s.io/apimachinery@v0.25.7/pkg/util/runtime/runtime.go:49 +0x75
panic({0x19cf5a0, 0x2d22520})
	/usr/local/go/src/runtime/panic.go:884 +0x212
github.com/kubeflow/mpi-operator/pkg/controller.(*SchedulerPluginsCtrl).calculatePGMinResources(0xc0008051d0, 0xc0014d971c, 0x1c7368d?)
	/go/src/github.com/kubeflow/mpi-operator/pkg/controller/podgroup.go:314 +0x1cc
github.com/kubeflow/mpi-operator/pkg/controller.(*SchedulerPluginsCtrl).newPodGroup(0xc001d86750?, 0xc000a19d40)
	/go/src/github.com/kubeflow/mpi-operator/pkg/controller/podgroup.go:249 +0x18a
github.com/kubeflow/mpi-operator/pkg/controller.(*MPIJobController).getOrCreatePodGroups(0xc00074cc60, 0xc000a19d40?)

So, I passed a lister for the priorityClass to podGroupCtrl for the scheduler-plugins.

[alculquicondor]

beyond the scope of this PR: why is calculatePGMinResources returning a pointer to ResourceList? ResourceList is already a map, which is already a pointer.

[tenzen-y]

It's due to historical context... When the v1 API is designed, they designed the MinResources as *corev1.ResourceList typed.

https://github.com/kubeflow/common/blob/4bcd3af369ff9d3ceaf72861c82d532ad2bc5655/pkg/apis/common/v1/types.go#L211

Honestly, I would change the *corev1.ResourceList typed to corev1.ResourceList typed. But we (WG Training) decided to keep using *corev1.ResourceList because changing the API has a breaking API change.

kubeflow/trainer#1813 (comment)

[alculquicondor]

Sounds good, let's leave it.

[alculquicondor]

When using volcano, the MPIJobController doens't need the priorityClassInformer?

[tenzen-y]

Yes, the controller doesn't need the priorityClassInformer.

• volcano
  

  mpi-operator/pkg/controller/podgroup.go

  Lines 170 to 175 in 2da8e05

  	func (v *VolcanoCtrl) calculatePGMinResources(_ *int32, mpiJob *kubeflow.MPIJob) *corev1.ResourceList {
  	if schedPolicy := mpiJob.Spec.RunPolicy.SchedulingPolicy; schedPolicy != nil {
  	return schedPolicy.MinResources
  	}
  	return nil
  	}

• scheduler-plugins
  

  mpi-operator/pkg/controller/podgroup.go

  Lines 297 to 315 in 2da8e05

  	func (s *SchedulerPluginsCtrl) calculatePGMinResources(minMember *int32, mpiJob *kubeflow.MPIJob) *corev1.ResourceList {
  	if schedPolicy := mpiJob.Spec.RunPolicy.SchedulingPolicy; schedPolicy != nil && schedPolicy.MinResources != nil {
  	return schedPolicy.MinResources
  	}
  	if minMember != nil && *minMember == 0 {
  	return nil
  	}
  	var order replicasOrder
  	for rt, replica := range mpiJob.Spec.MPIReplicaSpecs {
  	rp := replicaPriority{
  	priority: 0,
  	replicaType: rt,
  	ReplicaSpec: *replica,
  	}
  	pcName := replica.Template.Spec.PriorityClassName
  	if len(pcName) != 0 {
  	if priorityClass, err := s.PriorityClassLister.Get(pcName); err != nil {
  	klog.Warningf("Ignore replica %q priority class %q: %v", rt, pcName, err)

[tenzen-y]

Previously, we tried to implement calculatePGMinResources similar to the scheduler-plugins to the podGroupCtrl for the volcano.
But, we have given up on doing that because we (@tenzen-y and @alculquicondor) aren't familiar with volcano logic.

• Respect SchedulingPolicy #520 (comment)
• Respect SchedulingPolicy #520 (comment)

mpi-operator/pkg/controller/podgroup.go

Lines 36 to 72 in c8045ae

	// newPodGroup creates a new PodGroup for an MPIJob
	// resource. It also sets the appropriate OwnerReferences on the resource so
	// handleObject can discover the MPIJob resource that 'owns' it.
	func (c *MPIJobController) newPodGroup(mpiJob *kubeflow.MPIJob) *volcanov1beta1.PodGroup {
	minMember := workerReplicas(mpiJob) + 1
	queueName := mpiJob.Annotations[volcanov1beta1.QueueNameAnnotationKey]
	var minResources *corev1.ResourceList
	if schedulingPolicy := mpiJob.Spec.RunPolicy.SchedulingPolicy; schedulingPolicy != nil {
	if schedulingPolicy.MinAvailable != nil {
	minMember = *schedulingPolicy.MinAvailable
	}
	if len(schedulingPolicy.Queue) != 0 {
	queueName = schedulingPolicy.Queue
	}
	if schedulingPolicy.MinResources != nil {
	minResources = schedulingPolicy.MinResources
	}
	}
	if minResources == nil {
	minResources = c.calcPGMinResources(minMember, mpiJob.Spec.MPIReplicaSpecs)
	}
	return &volcanov1beta1.PodGroup{
	ObjectMeta: metav1.ObjectMeta{
	Name: mpiJob.Name,
	Namespace: mpiJob.Namespace,
	OwnerReferences: []metav1.OwnerReference{
	*metav1.NewControllerRef(mpiJob, kubeflow.SchemeGroupVersionKind),
	},
	},
	Spec: volcanov1beta1.PodGroupSpec{
	MinMember: minMember,
	Queue: queueName,
	PriorityClassName: calcPriorityClassName(mpiJob.Spec.MPIReplicaSpecs, mpiJob.Spec.RunPolicy.SchedulingPolicy),
	MinResources: minResources,
	},
	}
	}

[tenzen-y]

ref: #535

[alculquicondor]

Thanks for the reference :)

[tenzen-y]

@alculquicondor I addressed your suggestions. PTAL.

[alculquicondor]

Uhm... this approach has the downside of putting some dependencies in the mpi_job_controller code, but I think it's better than before.

/lgtm
/approve

[google-oss-prow]

[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