-
Notifications
You must be signed in to change notification settings - Fork 393
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
[FLINK-35265] Implement FlinkStateSnapshot custom resource #821
Conversation
b7c7644
to
6dbb319
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started reviewing this, added some initial comments.
In the PR description you mentioned that with the new mode the manual trigger nonce doesn't work anymore. Would it make sense to keep it working for backward compatibility (when the nonce changes simply create a snapshot resource)
.../apache/flink/kubernetes/operator/reconciler/deployment/AbstractFlinkResourceReconciler.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkStateSnapshotStatus.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/flink/kubernetes/operator/crd/CustomResourceDefinitionWatcherImpl.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/flink/kubernetes/operator/controller/FlinkStateSnapshotController.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/flink/kubernetes/operator/controller/FlinkStateSnapshotController.java
Outdated
Show resolved
Hide resolved
5c78025
to
0f094f3
Compare
...operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/spec/CheckpointSpec.java
Outdated
Show resolved
Hide resolved
...-api/src/main/java/org/apache/flink/kubernetes/operator/api/spec/FlinkStateSnapshotSpec.java
Outdated
Show resolved
Hide resolved
...operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/CommonStatus.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkStateSnapshotStatus.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/flink/kubernetes/operator/config/KubernetesOperatorConfigOptions.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/flink/kubernetes/operator/observer/snapshot/StateSnapshotObserver.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/flink/kubernetes/operator/observer/snapshot/StateSnapshotObserver.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/flink/kubernetes/operator/observer/snapshot/StateSnapshotObserver.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/flink/kubernetes/operator/observer/snapshot/StateSnapshotObserver.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/flink/kubernetes/operator/observer/snapshot/StateSnapshotObserver.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need some extra logic to handle savepoint job submissions and snapshot info during upgrades.
There is a bit of implicit logic currently in why the lastSavepoint is updated every time we submit a job from savepoint, and we need some new logic to capture that. (see my other long comment)
.../main/java/org/apache/flink/kubernetes/operator/observer/snapshot/StateSnapshotObserver.java
Outdated
Show resolved
Hide resolved
.../apache/flink/kubernetes/operator/reconciler/deployment/AbstractFlinkResourceReconciler.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/flink/kubernetes/operator/reconciler/deployment/AbstractJobReconciler.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/flink/kubernetes/operator/reconciler/deployment/AbstractJobReconciler.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/flink/kubernetes/operator/reconciler/deployment/ApplicationReconciler.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/flink/kubernetes/operator/reconciler/snapshot/StateSnapshotReconciler.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/flink/kubernetes/operator/reconciler/snapshot/StateSnapshotReconciler.java
Outdated
Show resolved
Hide resolved
A small addition to the previous comments. The SnapshotObserver currently also observes and updates the lastSavepointInfo for terminal jobs. This is a key mechanism to be able to handle failures during stateful upgrades. So we have to update that logic as well so that instead of record the lastSavepoint status we use a new shared logic with the snapshot custom resource mechanism. This mechanism is covered by some tests currently, enabling the snapshot CR for those tests would probably help repro the problem. For example:
I believe with the current implementation the savpoint info would be lost |
.../main/java/org/apache/flink/kubernetes/operator/controller/FlinkStateSnapshotController.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some testing myself as well, I think this is getting close to a good and mergable shape! Did not checked through the code, but added 2 local comments. In general, did some testing with the StateMachineExample
tutorial:
- Deployed a fresh job
- Created checkpoint via CRD (1 specific comment about
INCREMENTAL
) - Created savepoint via CRD
- Restored job from savepoint
So far the main functionality looks good, I plan to go though the files in the next couple days.
...rnetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/spec/JobSpec.java
Outdated
Show resolved
Hide resolved
...operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/spec/CheckpointSpec.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second part of my review, hopefully I will have some time tomorrow to finish and check the rest of the classes.
...i/src/main/java/org/apache/flink/kubernetes/operator/api/status/FlinkStateSnapshotState.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/apache/flink/kubernetes/operator/config/KubernetesOperatorConfigOptions.java
Show resolved
Hide resolved
...c/main/java/org/apache/flink/kubernetes/operator/config/KubernetesOperatorConfigOptions.java
Outdated
Show resolved
Hide resolved
...perator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/SavepointInfo.java
Show resolved
Hide resolved
...src/main/java/org/apache/flink/kubernetes/operator/controller/FlinkStateSnapshotContext.java
Outdated
Show resolved
Hide resolved
...ernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/listener/AuditUtils.java
Outdated
Show resolved
Hide resolved
...or/src/main/java/org/apache/flink/kubernetes/operator/metrics/FlinkStateSnapshotMetrics.java
Outdated
Show resolved
Hide resolved
...s-operator/src/main/java/org/apache/flink/kubernetes/operator/observer/SnapshotObserver.java
Show resolved
Hide resolved
.../main/java/org/apache/flink/kubernetes/operator/observer/snapshot/StateSnapshotObserver.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/flink/kubernetes/operator/reconciler/deployment/ApplicationReconciler.java
Outdated
Show resolved
Hide resolved
...perator/src/main/java/org/apache/flink/kubernetes/operator/service/AbstractFlinkService.java
Outdated
Show resolved
Hide resolved
Thanks for contributing this super useful feature @mateczagany 👍 !.
|
Thanks for your feedback @anupamaggarwal, really appreciate it!
|
...src/main/java/org/apache/flink/kubernetes/operator/controller/FlinkStateSnapshotContext.java
Outdated
Show resolved
Hide resolved
@@ -72,21 +74,26 @@ public class FlinkConfigManager { | |||
|
|||
private volatile Configuration defaultConfig; | |||
private volatile FlinkOperatorConfiguration defaultOperatorConfiguration; | |||
private final boolean snapshotCrdInstalled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed offline, the whole snapshot CRD install check is planned for 1 release to give more meaningful errors for users the first time they upgrade to an operator version that includes this feature, but will be useless after that. So I think it would make sense to mark this with a TODO to remove it in the 1.11 release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the note, thank you. I am going to also note here so others can see:
This field will be determined in FlinkOperator
when the operator starts, by checking if FlinkStateSnapshot
CRD is installed on the Kubernetes cluster.
When generating the default configuration for the operator, kubernetes.operator.snapshot.resource.enabled
will be set to false in this class if snapshotCrdInstalled
is false, to allow users to continue using the operator if they upgraded from a previous version but did not install the new FlinkStateSnapshot
CRD.
This check and field is planned to be removed in the release after this FLIP gets released (so hopefully 1.11 if this PR is released in 1.10).
...rnetes-operator/src/main/java/org/apache/flink/kubernetes/operator/service/FlinkService.java
Outdated
Show resolved
Hide resolved
...perator/src/main/java/org/apache/flink/kubernetes/operator/service/AbstractFlinkService.java
Outdated
Show resolved
Hide resolved
...perator/src/main/java/org/apache/flink/kubernetes/operator/service/AbstractFlinkService.java
Outdated
Show resolved
Hide resolved
...erator/src/main/java/org/apache/flink/kubernetes/operator/utils/FlinkStateSnapshotUtils.java
Outdated
Show resolved
Hide resolved
...erator/src/main/java/org/apache/flink/kubernetes/operator/utils/FlinkStateSnapshotUtils.java
Outdated
Show resolved
Hide resolved
...erator/src/main/java/org/apache/flink/kubernetes/operator/utils/FlinkStateSnapshotUtils.java
Outdated
Show resolved
Hide resolved
...erator/src/main/java/org/apache/flink/kubernetes/operator/utils/FlinkStateSnapshotUtils.java
Outdated
Show resolved
Hide resolved
...bhook/src/main/java/org/apache/flink/kubernetes/operator/admission/mutator/FlinkMutator.java
Outdated
Show resolved
Hide resolved
...erator/src/main/java/org/apache/flink/kubernetes/operator/utils/FlinkStateSnapshotUtils.java
Outdated
Show resolved
Hide resolved
.rescheduleAfter(ctx.getOperatorConfig().getReconcileInterval().toMillis()); | ||
case COMPLETED: | ||
var flinkDeployment = getFlinkDeployment(ctx); | ||
return handleSnapshotCleanup(resource, flinkDeployment, ctx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying out the periodic snapshot feature today. Does it make sense to introduce a feature to auto-cleanup the flinkstatesnapshots.flink.apache.org
CRs and only retain upto some max retention limit (we could still retain the savepoint dir?) IIUC disposeOnDelete
for periodic savepoints is set to false by default (so it should be relatively safe to do so).
If the user sets the savepoint interval kubernetes.operator.periodic.savepoint.interval
to a low value, and there are multiple jobs on the cluster it would lead to creation of many snapshot CRs. I am not familiar with K8s internals but could this increase the load on the API server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely! I would not feel comfortable releasing a new operator version that does not have auto-cleanup of FlinkStateSnapshot CRs. For the now-deprecated way of taking snapshots this could be configured using kubernetes.operator.savepoint.history.max.age
and kubernetes.operator.savepoint.history.max.count
.
You are right, disposeOnDelete
is false by default, and it can be controlled by the config kubernetes.operator.savepoint.dispose-on-delete
.
I am not sure of the exact approach I will take to handle cleanup, but it will be implemented under FLINK-35493 in another PR. That's a feature I felt we could easily separate to another PR, as this one is already growing quite big :D
With a low periodic interval, there won't be any more load on the API server and etcd than tracking snapshots in the Flink resource CR (as the operator does currently). Periodically cleaning them up might affect the load on API server/etcd depending on what solution we will go with. But that will be done under FLINK-35493.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done some investigation, and I think I will be able to easily implement this in the FlinkDeployment and FlinkSessionJob controller observe phase, just like the operator currently cleans up savepoints. I could just add a new InformerEventSource
so we would be able to list FlinkStateSnapshot
resources as secondary resources when we reconcile FlinkDeployment/FlinkSessionJob resources without querying the Kubernetes API server.
Once this PR is merged, I will create a new PR for this functionality.
051d796
to
566caf9
Compare
I rebased the branch on top of main, but I also pushed commit 566caf9 |
@anupamaggarwal @ferenc-csaky unless there are any concerns I would like to merge this. Afterwards we can focus on the follow up items and even get a chance to paralelnize this work |
Thank you @gyfora , I have done a lot of manual testing and I am quite happy with the current state of the PR. Once this is merged, I will start working on the other tickets in this FLIP, and create new PRs hopefully this week for the following:
I will be unavailable for ~3 weeks starting from Aug 23, I hope to have the remaining PRs ready and in good state until then. |
566caf9
to
56a30ff
Compare
What is the purpose of the change
Implement FlinkStateSnapshot as according to FLIP-446. This PR does not include the e2e-tests and documentation.
Brief change log
FlinkStateSnapshot
and all its dependent classes toflink-kubernetes-operator-api
FlinkDeployment
/FlinkSessionJob
as accepted in the FLIPFlinkService
to extract the logic of saving snapshot path to other classesFlinkConfigManager
class to check if the CRFlinkStateSnapshot
can be created on the current Kubernetes server during runtime. This is intended to be temporary to ensure a smooth upgrade process.Verifying this change
Does this pull request potentially affect one of the following parts:
CustomResourceDescriptors
: yesDocumentation
Other implementation details
FlinkStateSnapshot
CRs, and the snapshot will be taken when that resource is reconciled.FlinkStateSnapshot
CR, marking it withalreadyExists
.COMPLETED
, but with empty path.kubernetes.operator.savepoint.dispose-on-delete
) was also added that was not specified in the FLIP.jobReference
will contain an optional fieldnamespace
. If null, this will default to the namespace of the snapshot.CheckpointSpec#type
is removed as the only supported manual checkpoint type from Flink 1.19 isFULL
.Other PRs