[FLINK-26290] Introduce serviceAccount as direct field, remove taskSlots

[tweise]

No description provided.

[tweise]

@gyfora

[tweise]

@gyfora as follow-up I would suggest that we can handle yaml number types and don't assume Configuration.setString will just work. That way users won't need to quote numbers..

[wangyang0918]

@tweise Thanks for creating this PR. Regarding the serviceAccount and taskSlots, could you please share the guideline which fields should or should not be first-class?

[wangyang0918]

Maybe we need to set KubernetesConfigOptions#KUBERNETES_SERVICE_ACCOUNT here. If the user wants to give the TaskManager pods less permissions, then they should create different service accounts for both and configure them in jobManager and taskManager fields.

Note: The reason why we need to configure service account for TaskManager pod is about the Kubernetes HA. TaskManagers retrieve leader information from K8s ConfigMap.

The e2e test is failing also because of this.

[tweise]

Thanks for catching the config parameter issue. Fix is pushed. This just reinforces why we should not expose such implementation details to user (CR should abstract how we interact with k8s and also work for standalone mode).

[wangyang0918]

We also need to add the field serviceAccount to JobManagerSpec and TaskManagerSpec. Right? Otherwise, user could not configure service account for JobManager pod separately or have different service accounts for both.

[tweise]

Since separate service accounts for jm and tm pods are a rare advanced usage, they can be covered with the pod templates, which are already available separately for jm and tm.

[wangyang0918]

Make sense to me.

[tweise]

@tweise Thanks for creating this PR. Regarding the serviceAccount and taskSlots, could you please share the guideline which fields should or should not be first-class?

We don't have a strict guideline on that yet, but covered it a bit here: https://lists.apache.org/thread/x4ykd83bj330x7gqohdw0m8x0v54m083 and also on the FLIP: https://cwiki.apache.org/confluence/display/FLINK/FLIP-212%3A+Introduce+Flink+Kubernetes+Operator#FLIP212:IntroduceFlinkKubernetesOperator-InitialFeatureSet

[wangyang0918]

CR should abstract how we interact with k8s and also work for standalone mode.

This is a good point and explains why we need to make serviceAccount as first class field.

[Aitozi]

Hi @tweise I left one concern for this PR

I think we should avoid to introduce option as first class field when there is already config in Flink have the same effect, It brings the proxy work and may make user confused.

As described in https://lists.apache.org/thread/3q5bmr9253opv5b122s9nokp8yqq5rmw

I agree with the sentiment that whenever possible we should use the native configuration directly (either Flink native settings or k8s pod template)

So I think the serviceAccount can also reuse the KubernetesConfigOptions#KUBERNETES_SERVICE_ACCOUNT
KubernetesConfigOptions#JOB_MANAGER_SERVICE_ACCOUNT KubernetesConfigOptions#TASK_MANAGER_SERVICE_ACCOUNT. This can also work for the standalone mode. It can render the Pod template with the Flink effective configuration.

[wangyang0918]

@Aitozi I am afraid the service account related config options(e.g. KubernetesConfigOptions#KUBERNETES_SERVICE_ACCOUNT) could not take effect for standalone cluster.

[Aitozi]

@wangyang0918 I have not seen the standalone mode Reconciler in the code repo. But IMO, standalone mode will generate the JobManager deployment, TaskManager deployment, Service resource. These can be render by the effective config or directly declare in the pod templates.

[gyfora]

I think what Thomas did makes a lot of sense. The service account config is pretty confusing and easy to get wrong. This way we can set the relevant settings depending on the mode (native/standalone).

+1, I am rebasing this on the config changes and merging afterwards