You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/04/27 10:21:55 UTC
[GitHub] [flink-kubernetes-operator] Aitozi opened a new pull request, #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Aitozi opened a new pull request, #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184
This PR is meant to complete the validator for flink session job. This also enables the webhook for session job.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora commented on a diff in pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#discussion_r866630569
##########
helm/flink-kubernetes-operator/templates/webhook.yaml:
##########
@@ -83,7 +83,7 @@ metadata:
cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/flink-operator-serving-cert
name: flink-operator-{{ .Release.Namespace }}-webhook-configuration
webhooks:
-- name: vflinkdeployments.flink.apache.org
+- name: flinkoperator.flink.apache.org
Review Comment:
Could we simply name this `flinkoperator-webhook` or something like this?
##########
flink-kubernetes-webhook/src/main/java/org/apache/flink/kubernetes/operator/admission/FlinkValidator.java:
##########
@@ -20,41 +20,74 @@
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.NotAllowedException;
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.Operation;
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.validation.Validator;
+import org.apache.flink.kubernetes.operator.crd.CrdConstants;
import org.apache.flink.kubernetes.operator.crd.FlinkDeployment;
+import org.apache.flink.kubernetes.operator.crd.FlinkSessionJob;
import org.apache.flink.kubernetes.operator.validation.FlinkResourceValidator;
import com.fasterxml.jackson.databind.ObjectMapper;
-import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
+import io.fabric8.kubernetes.api.model.GroupVersionKind;
+import io.fabric8.kubernetes.api.model.KubernetesResource;
+import io.fabric8.kubernetes.client.DefaultKubernetesClient;
+import io.fabric8.kubernetes.client.KubernetesClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.Optional;
import java.util.Set;
/** Validator for FlinkDeployment creation and updates. */
-public class FlinkValidator implements Validator<GenericKubernetesResource> {
+public class FlinkValidator implements Validator<KubernetesResource> {
Review Comment:
I don't quite understand why it would be somethis generic and sometimes plain KuberentesResource but safest to keep it as you have done now. We anyways map it to our custom classes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#discussion_r866638733
##########
helm/flink-kubernetes-operator/templates/webhook.yaml:
##########
@@ -83,7 +83,7 @@ metadata:
cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/flink-operator-serving-cert
name: flink-operator-{{ .Release.Namespace }}-webhook-configuration
webhooks:
-- name: vflinkdeployments.flink.apache.org
+- name: flinkoperator.flink.apache.org
Review Comment:
Sure, updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] morhidi commented on a diff in pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#discussion_r866469218
##########
flink-kubernetes-webhook/src/main/java/org/apache/flink/kubernetes/operator/admission/FlinkValidator.java:
##########
@@ -20,41 +20,74 @@
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.NotAllowedException;
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.Operation;
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.validation.Validator;
+import org.apache.flink.kubernetes.operator.crd.CrdConstants;
import org.apache.flink.kubernetes.operator.crd.FlinkDeployment;
+import org.apache.flink.kubernetes.operator.crd.FlinkSessionJob;
import org.apache.flink.kubernetes.operator.validation.FlinkResourceValidator;
import com.fasterxml.jackson.databind.ObjectMapper;
-import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
+import io.fabric8.kubernetes.api.model.GroupVersionKind;
+import io.fabric8.kubernetes.api.model.KubernetesResource;
+import io.fabric8.kubernetes.client.DefaultKubernetesClient;
+import io.fabric8.kubernetes.client.KubernetesClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.Optional;
import java.util.Set;
/** Validator for FlinkDeployment creation and updates. */
-public class FlinkValidator implements Validator<GenericKubernetesResource> {
+public class FlinkValidator implements Validator<KubernetesResource> {
Review Comment:
Bit confused, it's either FlinkDeployment of FlinkSessionjob that we're validating here, so I would expect a CustomResource here @gyfora?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] morhidi commented on a diff in pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#discussion_r866302913
##########
examples/basic-session-job.yaml:
##########
@@ -19,16 +19,10 @@
apiVersion: flink.apache.org/v1beta1
kind: FlinkDeployment
metadata:
- name: basic-session-cluster-with-ha
+ name: basic-session-cluster
spec:
image: flink:1.14
flinkVersion: v1_14
- flinkConfiguration:
Review Comment:
Thanks, it helps!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#discussion_r866442081
##########
helm/flink-kubernetes-operator/templates/webhook.yaml:
##########
@@ -83,7 +83,7 @@ metadata:
cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/flink-operator-serving-cert
name: flink-operator-{{ .Release.Namespace }}-webhook-configuration
webhooks:
-- name: vflinkdeployments.flink.apache.org
+- name: vflinkoperator.flink.apache.org
Review Comment:
It originally starts with 'v', so I kept it, I think it means validator?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#discussion_r866453467
##########
flink-kubernetes-webhook/src/main/java/org/apache/flink/kubernetes/operator/admission/FlinkValidator.java:
##########
@@ -20,41 +20,74 @@
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.NotAllowedException;
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.Operation;
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.validation.Validator;
+import org.apache.flink.kubernetes.operator.crd.CrdConstants;
import org.apache.flink.kubernetes.operator.crd.FlinkDeployment;
+import org.apache.flink.kubernetes.operator.crd.FlinkSessionJob;
import org.apache.flink.kubernetes.operator.validation.FlinkResourceValidator;
import com.fasterxml.jackson.databind.ObjectMapper;
-import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
+import io.fabric8.kubernetes.api.model.GroupVersionKind;
+import io.fabric8.kubernetes.api.model.KubernetesResource;
+import io.fabric8.kubernetes.client.DefaultKubernetesClient;
+import io.fabric8.kubernetes.client.KubernetesClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.Optional;
import java.util.Set;
/** Validator for FlinkDeployment creation and updates. */
-public class FlinkValidator implements Validator<GenericKubernetesResource> {
+public class FlinkValidator implements Validator<KubernetesResource> {
Review Comment:
When I test, I found that the Object passed here may not always be `GenericKubernetesResource`. It may be the `CustomResource` or `GenericKubernetesResource`. The object retrieved from the AdmissionRequest is the `KubernetesResource` . So I think we do not have to assume that is `GenericKubernetesResource`. Please correct me if I'm wrong
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] morhidi commented on a diff in pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#discussion_r866288584
##########
helm/flink-kubernetes-operator/templates/webhook.yaml:
##########
@@ -83,7 +83,7 @@ metadata:
cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/flink-operator-serving-cert
name: flink-operator-{{ .Release.Namespace }}-webhook-configuration
webhooks:
-- name: vflinkdeployments.flink.apache.org
+- name: vflinkoperator.flink.apache.org
Review Comment:
nit: Why the name starts with a 'v'? :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] morhidi commented on a diff in pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#discussion_r866469562
##########
helm/flink-kubernetes-operator/templates/webhook.yaml:
##########
@@ -83,7 +83,7 @@ metadata:
cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/flink-operator-serving-cert
name: flink-operator-{{ .Release.Namespace }}-webhook-configuration
webhooks:
-- name: vflinkdeployments.flink.apache.org
+- name: vflinkoperator.flink.apache.org
Review Comment:
I guess it's just a typo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] morhidi commented on a diff in pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#discussion_r866301194
##########
flink-kubernetes-webhook/src/main/java/org/apache/flink/kubernetes/operator/admission/FlinkValidator.java:
##########
@@ -20,41 +20,74 @@
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.NotAllowedException;
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.Operation;
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.validation.Validator;
+import org.apache.flink.kubernetes.operator.crd.CrdConstants;
import org.apache.flink.kubernetes.operator.crd.FlinkDeployment;
+import org.apache.flink.kubernetes.operator.crd.FlinkSessionJob;
import org.apache.flink.kubernetes.operator.validation.FlinkResourceValidator;
import com.fasterxml.jackson.databind.ObjectMapper;
-import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
+import io.fabric8.kubernetes.api.model.GroupVersionKind;
+import io.fabric8.kubernetes.api.model.KubernetesResource;
+import io.fabric8.kubernetes.client.DefaultKubernetesClient;
+import io.fabric8.kubernetes.client.KubernetesClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.Optional;
import java.util.Set;
/** Validator for FlinkDeployment creation and updates. */
-public class FlinkValidator implements Validator<GenericKubernetesResource> {
+public class FlinkValidator implements Validator<KubernetesResource> {
Review Comment:
Why do we need the change GenericKubernetesResource -> KubernetesResource???
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#discussion_r866477291
##########
helm/flink-kubernetes-operator/templates/webhook.yaml:
##########
@@ -83,7 +83,7 @@ metadata:
cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/flink-operator-serving-cert
name: flink-operator-{{ .Release.Namespace }}-webhook-configuration
webhooks:
-- name: vflinkdeployments.flink.apache.org
+- name: vflinkoperator.flink.apache.org
Review Comment:
OK, will remove
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora merged pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
gyfora merged PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#discussion_r866478338
##########
flink-kubernetes-webhook/src/main/java/org/apache/flink/kubernetes/operator/admission/FlinkValidator.java:
##########
@@ -20,41 +20,74 @@
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.NotAllowedException;
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.Operation;
import org.apache.flink.kubernetes.operator.admission.admissioncontroller.validation.Validator;
+import org.apache.flink.kubernetes.operator.crd.CrdConstants;
import org.apache.flink.kubernetes.operator.crd.FlinkDeployment;
+import org.apache.flink.kubernetes.operator.crd.FlinkSessionJob;
import org.apache.flink.kubernetes.operator.validation.FlinkResourceValidator;
import com.fasterxml.jackson.databind.ObjectMapper;
-import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
+import io.fabric8.kubernetes.api.model.GroupVersionKind;
+import io.fabric8.kubernetes.api.model.KubernetesResource;
+import io.fabric8.kubernetes.client.DefaultKubernetesClient;
+import io.fabric8.kubernetes.client.KubernetesClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.Optional;
import java.util.Set;
/** Validator for FlinkDeployment creation and updates. */
-public class FlinkValidator implements Validator<GenericKubernetesResource> {
+public class FlinkValidator implements Validator<KubernetesResource> {
Review Comment:
But I'm not sure what's the actual type of the `AdmissionRequest#object` . It's only defined as `KubernetesResource`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
gyfora commented on PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#issuecomment-1118540176
@Aitozi is this still a draft?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#discussion_r866643546
##########
helm/flink-kubernetes-operator/templates/webhook.yaml:
##########
@@ -83,7 +83,7 @@ metadata:
cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/flink-operator-serving-cert
name: flink-operator-{{ .Release.Namespace }}-webhook-configuration
webhooks:
-- name: vflinkdeployments.flink.apache.org
+- name: flinkoperator.flink.apache.org
Review Comment:
Oh, the wehook name are enforced by the rule, reverted
```
Error: INSTALLATION FAILED: ValidatingWebhookConfiguration.admissionregistration.k8s.io "flink-operator-flink-webhook-configuration" is invalid: webhooks[0].name: Invalid value: "flinkoperator-webhook": should be a domain with at least three segments separated by dots
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#discussion_r866016282
##########
examples/basic-session-job.yaml:
##########
@@ -19,16 +19,10 @@
apiVersion: flink.apache.org/v1beta1
kind: FlinkDeployment
metadata:
- name: basic-session-cluster-with-ha
+ name: basic-session-cluster
spec:
image: flink:1.14
flinkVersion: v1_14
- flinkConfiguration:
Review Comment:
I remove the unnecessary option in this example to keep it simple as a 'basic' example
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] Aitozi commented on pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
Aitozi commented on PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#issuecomment-1118671688
@gyfora It's ready for review now, It do not do the force requirements for the existence of the FlinkDeployment which different from the discussion in the ticket. Because I found it will become less flexible if we enforce that when I testing it. Please let me know if you have better suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-kubernetes-operator] Aitozi commented on a diff in pull request #184: [FLINK-27262] Enrich validator for FlinkSessionJob
Posted by GitBox <gi...@apache.org>.
Aitozi commented on code in PR #184:
URL: https://github.com/apache/flink-kubernetes-operator/pull/184#discussion_r866477291
##########
helm/flink-kubernetes-operator/templates/webhook.yaml:
##########
@@ -83,7 +83,7 @@ metadata:
cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/flink-operator-serving-cert
name: flink-operator-{{ .Release.Namespace }}-webhook-configuration
webhooks:
-- name: vflinkdeployments.flink.apache.org
+- name: vflinkoperator.flink.apache.org
Review Comment:
Removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org