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

fix: fix issue #1723 about spark-operator not working with volcano on OCP #1724

Merged
merged 1 commit into from
Nov 9, 2023
Merged

fix: fix issue #1723 about spark-operator not working with volcano on OCP #1724

merged 1 commit into from
Nov 9, 2023

Conversation

disaster37
Copy link
Contributor

Fix issue #1723

We fix some parts of code:

  • pkg/batchscheduler/volcano/volcano_scheduler.go: to return error when appear on podGroup create.
  • pkg/batchscheduler/volcano/volcano_scheduler.go and manifest/spark-operator-install/spark-operator-rbac.yaml to fix right on OCP.

It fix error on OCP

podgroups.scheduling.volcano.sh "spark-spark-test-pg" is forbidden: cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on: , <nil>

… OCP

Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>
@Aakcht
Copy link
Contributor

Aakcht commented Nov 9, 2023

Hi, @liyinan926 , any chance for reviewing this PR?

var (
err error
pg *v1beta1.PodGroup
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change doesn't seem necessary given that pg is not used outside the if-else scope.

Copy link
Contributor Author

@disaster37 disaster37 Nov 9, 2023

Choose a reason for hiding this comment

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

Hum I remeber why I add this. It's because without that, you init new err on if. So on line 154 (inside if)
_, err = v.volcanoClient.SchedulingV1beta1().PodGroups(app.Namespace).Create(context.TODO(), &podGroup, metav1.CreateOptions{})

The err can't be used outside of if / else and so it never returned by

if err != nil {
		return fmt.Errorf("failed to sync PodGroup with error: %s. Abandon schedule pods via volcano", err)
	}

So:

  • stay my code like this, or add err handler on if like this
if _, err = v.volcanoClient.SchedulingV1beta1().PodGroups(app.Namespace).Create(context.TODO(), &podGroup, metav1.CreateOptions{}); err != nil {
  return fmt.Errorf("failed to sync PodGroup with error: %s. Abandon schedule pods via volcano", err)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you prefer ?

@liyinan926 liyinan926 merged commit 8cb6c80 into kubeflow:master Nov 9, 2023
1 check passed
@Aakcht
Copy link
Contributor

Aakcht commented Nov 10, 2023

@disaster37 @liyinan926 Thanks for taking a look!

peter-mcclonski pushed a commit to TechnologyBrewery/spark-on-k8s-operator that referenced this pull request Apr 16, 2024
…lcano on OCP (kubeflow#1724)

* fix: fix issue kubeflow#1723 about spark-operator not working with volcano on OCP

Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>

* Update volcano_scheduler.go

---------

Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>
Signed-off-by: Peter McClonski <mcclonski_peter@bah.com>
sigmarkarl pushed a commit to spotinst/spark-on-k8s-operator that referenced this pull request Aug 7, 2024
…lcano on OCP (kubeflow#1724)

* fix: fix issue kubeflow#1723 about spark-operator not working with volcano on OCP

Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>

* Update volcano_scheduler.go

---------

Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>
jbhalodia-slack pushed a commit to jbhalodia-slack/spark-operator that referenced this pull request Oct 4, 2024
…lcano on OCP (kubeflow#1724)

* fix: fix issue kubeflow#1723 about spark-operator not working with volcano on OCP

Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>

* Update volcano_scheduler.go

---------

Signed-off-by: disaster37 <linuxworkgroup@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants