You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "mbalassi (via GitHub)" <gi...@apache.org> on 2023/03/13 21:37:15 UTC
[GitHub] [flink-kubernetes-operator] mbalassi opened a new pull request, #548: [FLINK-31407] Bump fabric8 version to 6.5.0
mbalassi opened a new pull request, #548:
URL: https://github.com/apache/flink-kubernetes-operator/pull/548
This new version brings critical stability fixes for the informers. I fixed the newly introduced deprecation warnings.
--
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] mbalassi commented on pull request #548: [FLINK-31407] Bump fabric8 version to 6.5.0
Posted by "mbalassi (via GitHub)" <gi...@apache.org>.
mbalassi commented on PR #548:
URL: https://github.com/apache/flink-kubernetes-operator/pull/548#issuecomment-1467803697
The changes broke our tests that rely on the `KubernetesMockClient`, e.g.:
```
Caused by: io.fabric8.kubernetes.client.KubernetesClientException: Failure executing: PATCH at: https://localhost:54301/apis/flink.apache.org/v1beta1/namespaces/flink-operator-test/flinkdeployments/test-cluster?fieldManager=fabric8. Message: Not Found.
at io.fabric8.kubernetes.client.dsl.internal.OperationSupport.requestFailure(OperationSupport.java:701)
```
I will come back to this a bit later, any pointers are welcome. 😏
--
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 #548: [FLINK-31407] Bump fabric8 version to 6.5.0
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on code in PR #548:
URL: https://github.com/apache/flink-kubernetes-operator/pull/548#discussion_r1142249645
##########
flink-kubernetes-standalone/src/main/java/org/apache/flink/kubernetes/operator/kubeclient/Fabric8FlinkStandaloneKubeClient.java:
##########
@@ -57,14 +58,14 @@ public void stopAndCleanupCluster(String clusterId) {
.apps()
.deployments()
.withName(StandaloneKubernetesUtils.getJobManagerDeploymentName(clusterId))
- .cascading(true)
+ .withPropagationPolicy(DeletionPropagation.ORPHAN)
.delete();
this.internalClient
.apps()
.deployments()
.withName(StandaloneKubernetesUtils.getTaskManagerDeploymentName(clusterId))
- .cascading(true)
+ .withPropagationPolicy(DeletionPropagation.ORPHAN)
Review Comment:
Why not FOREGROUND/BACKGROUND?
--
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] gaborgsomogyi commented on pull request #548: [FLINK-31407] Bump fabric8 version to 6.5.0
Posted by "gaborgsomogyi (via GitHub)" <gi...@apache.org>.
gaborgsomogyi commented on PR #548:
URL: https://github.com/apache/flink-kubernetes-operator/pull/548#issuecomment-1471892857
I've just had a deeper look at the test issues and here are my findings:
* The originally used `createOrReplace` has the following semantics
* Create the resource with `POST` operation
* If response is HTTP_CONFLICT (409) then replace the resource with `PUT` operation (replace operation is effectively a forced update with retries)
* The `createOrReplace` doc says the following:
```
/**
* Creates a provided resource in a Kubernetes Cluster. If creation
* fails with a HTTP_CONFLICT, it tries to replace resource.
*
* @return created item returned in kubernetes api response
*
* @deprecated please user {@link ServerSideApplicable#serverSideApply()} or attempt a create and edit/patch operation.
*/
@Deprecated
T createOrReplace();
```
* The doc is cloudy what `serverSideApply` really does so had an in-depth look of the source code.
* In case of custom resource the code is unconditionally patch the resource with `PATCH` operation
My general conclusion is that `createOrReplace` and `serverSideApply` are having totally different semantics if we consider the source code. My proposal is the following:
* Use `create` when new resource is needed
* Use `update` when existing resource need to be upgraded (since `replace` is deprecated too)
* If it's super hard to decide whether create or update is needed then one can re-implement to original `createOrReplace` functionality but I discourage to do it
--
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] mbalassi commented on a diff in pull request #548: [FLINK-31407] Bump fabric8 version to 6.5.0
Posted by "mbalassi (via GitHub)" <gi...@apache.org>.
mbalassi commented on code in PR #548:
URL: https://github.com/apache/flink-kubernetes-operator/pull/548#discussion_r1142261346
##########
flink-kubernetes-standalone/src/main/java/org/apache/flink/kubernetes/operator/kubeclient/Fabric8FlinkStandaloneKubeClient.java:
##########
@@ -57,14 +58,14 @@ public void stopAndCleanupCluster(String clusterId) {
.apps()
.deployments()
.withName(StandaloneKubernetesUtils.getJobManagerDeploymentName(clusterId))
- .cascading(true)
+ .withPropagationPolicy(DeletionPropagation.ORPHAN)
.delete();
this.internalClient
.apps()
.deployments()
.withName(StandaloneKubernetesUtils.getTaskManagerDeploymentName(clusterId))
- .cascading(true)
+ .withPropagationPolicy(DeletionPropagation.ORPHAN)
Review Comment:
Same as https://github.com/apache/flink-kubernetes-operator/pull/548#discussion_r1142261062.
--
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] mbalassi merged pull request #548: [FLINK-31407] Bump fabric8 version to 6.5.0
Posted by "mbalassi (via GitHub)" <gi...@apache.org>.
mbalassi merged PR #548:
URL: https://github.com/apache/flink-kubernetes-operator/pull/548
--
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 #548: [FLINK-31407] Bump fabric8 version to 6.5.0
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on code in PR #548:
URL: https://github.com/apache/flink-kubernetes-operator/pull/548#discussion_r1137077929
##########
flink-kubernetes-standalone/src/main/java/org/apache/flink/kubernetes/operator/kubeclient/Fabric8FlinkStandaloneKubeClient.java:
##########
@@ -57,14 +58,14 @@ public void stopAndCleanupCluster(String clusterId) {
.apps()
.deployments()
.withName(StandaloneKubernetesUtils.getJobManagerDeploymentName(clusterId))
- .cascading(true)
+ .withPropagationPolicy(DeletionPropagation.ORPHAN)
Review Comment:
Why note foreground? Orphan would not delete pods, configmaps etc right?
--
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] mbalassi commented on pull request #548: [FLINK-31407] Bump fabric8 version to 6.5.0
Posted by "mbalassi (via GitHub)" <gi...@apache.org>.
mbalassi commented on PR #548:
URL: https://github.com/apache/flink-kubernetes-operator/pull/548#issuecomment-1479966458
@gaborgsomogyi offered to help me out with finishing this, so we are spitting up the work into 2 portions. One that can be verified and merged quickly and the `createOrReplace` related part that will be split out to a separate PR.
--
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] mbalassi commented on a diff in pull request #548: [FLINK-31407] Bump fabric8 version to 6.5.0
Posted by "mbalassi (via GitHub)" <gi...@apache.org>.
mbalassi commented on code in PR #548:
URL: https://github.com/apache/flink-kubernetes-operator/pull/548#discussion_r1142261062
##########
flink-kubernetes-standalone/src/main/java/org/apache/flink/kubernetes/operator/kubeclient/Fabric8FlinkStandaloneKubeClient.java:
##########
@@ -57,14 +58,14 @@ public void stopAndCleanupCluster(String clusterId) {
.apps()
.deployments()
.withName(StandaloneKubernetesUtils.getJobManagerDeploymentName(clusterId))
- .cascading(true)
+ .withPropagationPolicy(DeletionPropagation.ORPHAN)
Review Comment:
That makes more sense, to me - the javadoc was ambiguous tbh. I prefer foreground unless @usamj or @dannycranmer disagrees.
--
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] mbalassi commented on a diff in pull request #548: [FLINK-31407] Bump fabric8 version to 6.5.0
Posted by "mbalassi (via GitHub)" <gi...@apache.org>.
mbalassi commented on code in PR #548:
URL: https://github.com/apache/flink-kubernetes-operator/pull/548#discussion_r1134642722
##########
flink-kubernetes-standalone/src/main/java/org/apache/flink/kubernetes/operator/kubeclient/Fabric8FlinkStandaloneKubeClient.java:
##########
@@ -57,14 +58,14 @@ public void stopAndCleanupCluster(String clusterId) {
.apps()
.deployments()
.withName(StandaloneKubernetesUtils.getJobManagerDeploymentName(clusterId))
- .cascading(true)
+ .withPropagationPolicy(DeletionPropagation.ORPHAN)
Review Comment:
@usamj could you please confirm that this was the intention here.
##########
flink-kubernetes-standalone/src/main/java/org/apache/flink/kubernetes/operator/kubeclient/Fabric8FlinkStandaloneKubeClient.java:
##########
@@ -57,14 +58,14 @@ public void stopAndCleanupCluster(String clusterId) {
.apps()
.deployments()
.withName(StandaloneKubernetesUtils.getJobManagerDeploymentName(clusterId))
- .cascading(true)
+ .withPropagationPolicy(DeletionPropagation.ORPHAN)
Review Comment:
@usamj could you please confirm that this was the intention here?
--
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] usamj commented on a diff in pull request #548: [FLINK-31407] Bump fabric8 version to 6.5.0
Posted by "usamj (via GitHub)" <gi...@apache.org>.
usamj commented on code in PR #548:
URL: https://github.com/apache/flink-kubernetes-operator/pull/548#discussion_r1143235967
##########
flink-kubernetes-standalone/src/main/java/org/apache/flink/kubernetes/operator/kubeclient/Fabric8FlinkStandaloneKubeClient.java:
##########
@@ -57,14 +58,14 @@ public void stopAndCleanupCluster(String clusterId) {
.apps()
.deployments()
.withName(StandaloneKubernetesUtils.getJobManagerDeploymentName(clusterId))
- .cascading(true)
+ .withPropagationPolicy(DeletionPropagation.ORPHAN)
Review Comment:
I agree, foreground is more in-line with the behaviour that we want.
--
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 #548: [FLINK-31407] Bump fabric8 version to 6.5.0
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on code in PR #548:
URL: https://github.com/apache/flink-kubernetes-operator/pull/548#discussion_r1137077929
##########
flink-kubernetes-standalone/src/main/java/org/apache/flink/kubernetes/operator/kubeclient/Fabric8FlinkStandaloneKubeClient.java:
##########
@@ -57,14 +58,14 @@ public void stopAndCleanupCluster(String clusterId) {
.apps()
.deployments()
.withName(StandaloneKubernetesUtils.getJobManagerDeploymentName(clusterId))
- .cascading(true)
+ .withPropagationPolicy(DeletionPropagation.ORPHAN)
Review Comment:
Why not foreground? Orphan would not delete pods, configmaps etc right?
--
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 #548: [FLINK-31407] Bump fabric8 version to 6.5.0
Posted by "gyfora (via GitHub)" <gi...@apache.org>.
gyfora commented on PR #548:
URL: https://github.com/apache/flink-kubernetes-operator/pull/548#issuecomment-1476388021
> I've just had a deeper look at the test issues and here are my findings:
>
> * The originally used `createOrReplace` has the following semantics
>
> * Create the resource with `POST` operation
> * If response is HTTP_CONFLICT (409) then replace the resource with `PUT` operation (replace operation is effectively a forced update with retries)
> * The `createOrReplace` doc says the following:
> ```
> /**
> * Creates a provided resource in a Kubernetes Cluster. If creation
> * fails with a HTTP_CONFLICT, it tries to replace resource.
> *
> * @return created item returned in kubernetes api response
> *
> * @deprecated please user {@link ServerSideApplicable#serverSideApply()} or attempt a create and edit/patch operation.
> */
> @Deprecated
> T createOrReplace();
> ```
> * The doc is cloudy what `serverSideApply` really does so had an in-depth look of the source code.
> * In case of custom resource the code is unconditionally patch the resource with `PATCH` operation
>
> My general conclusion is that `createOrReplace` and `serverSideApply` are having totally different semantics if we consider the source code. My proposal is the following:
>
> * Use `create` when new resource is needed
> * Use `update` when existing resource need to be upgraded (since `replace` is deprecated too)
> * If it's super hard to decide whether create or update is needed then one can re-implement to original `createOrReplace` functionality but I discourage to do it
I agree with @gaborgsomogyi 's assessment. If we need we can keep the serverSideApply in some tests but we should not use it in the core logic.
--
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