fix(chart): remove operator namespace default for job namespaces value

[t3mi]

🛑 Important:

Please open an issue to discuss significant work before you start. We appreciate your contributions and don't want your efforts to go to waste!

For guidelines on how to contribute, please review the CONTRIBUTING.md document.

Purpose of this PR

Allow opt-out for operator namespace in job namespaces value

Proposed changes:

• [BUG] regression - not possible to watch all namespaces #1988

Change Category

Indicate the type of change by marking the applicable boxes:

• Bugfix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that could affect existing functionality)
• Documentation update

Rationale

Checklist

Before submitting your PR, please review the following:

• I have conducted a self-review of my own code.
• I have updated documentation accordingly.
• I have added tests that prove my changes are effective or that my feature works.
• Existing unit tests pass locally with my changes.

Additional Notes

[vara-bonthu]

Thanks @t3mi !

@AndrewChubatiuk Could you please validate this PR as this a fix to your PR #1955?

[AndrewChubatiuk]

looks like this is not a fix, but a feature

[vara-bonthu]

looks like this is not a fix, but a feature

Thanks @AndrewChubatiuk! Author mentioned in the issue stating Before https://github.com/kubeflow/spark-operator/pull/1955, it was possible to set namespace to be an empty string meaning all namespaces were watched

@t3mi could you resolve the conflicts so we can proceed with merging the PR? Thank you!

[t3mi]

@vara-bonthu please allow CI to run to recheck the fix

[yuchaoran2011]

I don't see why the namespace where the operator runs should be treated differently compared to any other namespace. When the list of namespaces is empty, then no job namespace should be configured. If the user intends to use the operator namespace as a job namespace, then it should be explicitly configured

[t3mi]

@yuchaoran2011 PR #1955 introduced such functionality - job namespaces value defaults to operator namespace when nothing was specified.
I don't have any preference - either current changes or your proposed tweak will work for my use case.

@vara-bonthu I've updated the PR to bump the chart version and instead changed the logic per @yuchaoran2011 suggestion, please re-trigger the CI job.

[vara-bonthu]

I'm slightly confused by the discussion. The current issue is that our latest version of the code does not automatically monitor any namespaces for running jobs; instead, it requires users to explicitly configure the namespaces in the Spark Operator through values. This is an anti-pattern, as the Spark Operator should by default watch all namespaces if the configuration is left empty, as shown in the values.yaml here:

spark-operator/charts/spark-operator-chart/values.yaml

Line 56 in 5e9c0b4

sparkJobNamespaces: []

and it's also breaking the existing functionality.

Data teams within the organization end up requesting the platform team to add more namespaces to monitor as they onboard more teams and namespaces. This requires the platform team to redeploy the Spark Operator Helm chart for every such request.

Here are a couple of best practices:

• Do not use the same namespace for running Spark jobs where the Spark Operator is deployed. Users should create a new namespace for running the jobs with minimum policies. Spark Operator namespace is defined by platform team and it may not be allowed for the teams to use it for running jobs.
• By default, the Spark Operator should monitor all namespaces if sparkJobNamespaces: [] is left empty.
• Users should have the option to override the sparkJobNamespaces: [] by defining their preferred namespaces to monitor, if they choose

[t3mi]

That's correct and proposed changes remove default (to watch only in spark operator namespaces) set in the code - so if sparkJobNamespaces is empty, operator will watch all namespaces but it would be up to the data/platform team to setup necessary permissions for spark service account in namespaces where spark apps live. Otherwise, static list of namespaces could be set during operator install and necessary permissions would be created by the chart.

[vara-bonthu]

/approve

[vara-bonthu]

Waiting for another approval @yuchaoran2011 and this can be merged after

[vara-bonthu]

@t3mi, a previous PR has been merged which now causes a conflict. Please resolve this conflict and update the Chart version accordingly.

[t3mi]

@vara-bonthu rebased with version bump

[vara-bonthu]

/lgtm

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: t3mi, vara-bonthu

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 [vara-bonthu]

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