Skip to content
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-33803] Remove deprecated lastReconciledSpec meta generation in favour of observedGeneration #834

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

angelachenn
Copy link
Contributor

What is the purpose of the change

Followup to https://issues.apache.org/jira/browse/FLINK-33803 (#755)

The observedGeneration field in the FlinkDeployment CRD status is used to persist the generation that was last acted on by the operator. At the end of a successful reconciliation, the operator sets the observedGeneration field to the current generation number (from the CRD metadata).

This PR removes internal usages of the lastReconciledSpec metadata (and generation) that were kept in the initial PR to avoid downstream breakage. observedGeneration replaces this semantically. Removing the deprecated fields is to end up with a cleaner codebase.

Note: the removed fields were nested within the lastReconciledSpec String, thus there is no documentation to be updated (please advise if I missed something).

Brief change log

  • There is no more ObjectMeta metadata attribute (containing generation) within the ReconciliationMetadata class
    • that field will not exist to the operator within FlinkDeploymentStatus w.r.t. reconciliation
  • removed the metadata field (from the CRD metadata) during deserialization before passing the content into a ReconciliationMetadata instance such that its constructor does not mutate the resources
    • if not, Jackson throws a JsonMappingException: Unrecognized field 'metadata' (class org.apache.flink.kubernetes.operator.api.reconciler.ReconciliationMetadata), not marked as ignorable, causing pods to fail.

Verifying this change

This change added tests and can be verified as follows:

  • Modified existing tests that checked for specWithMeta.getMeta().getMetadata().getGeneration() to assert against status.getObservedGeneration() instead (tests pass)
  • Manually verified the change by running a cluster with a FlinkDeployment custom resource that depends on the operator (with 2 JobManagers & 2 TaskManagers), and checking its CRD, verifying that fields are mapped accordingly.
Logs

Before:

k get flinkdeployment flink --context=<context> -n <ns>  -o yaml | yq .status.reconciliationStatus.lastReconciledSpec | jq .resource_metadata
{
  "apiVersion": "flink.apache.org/v1beta1",
  "metadata": {
    "generation": 5
  },
  "firstDeployment": false
}
k get flinkdeployment flink --context=<context> -n <ns>  -o yaml | yq .status.observedGeneration
5

After:

k get flinkdeployment flink --context=<context> -n <ns>  -o yaml | yq .status.reconciliationStatus.lastReconciledSpec | jq .resource_metadata                                                                                                                                                      
{
  "apiVersion": "flink.apache.org/v1beta1",
  "firstDeployment": true
}
k get flinkdeployment flink --context=<context> -n <ns>  -o yaml | yq .status.observedGeneration
5

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changes to the CustomResourceDescriptors: yes
  • Core observer or reconciler logic that is regularly executed: yes

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable

@gyfora
Copy link
Contributor

gyfora commented Aug 6, 2024

hey @angelachenn , are you still working on this change?

@angelachenn
Copy link
Contributor Author

hey @gyfora, thanks for checking in, and apologies for the lack of updates. I had to shift my focus to some higher-priority tasks and have also been on vacation. I do intend to keep working on this change and will be back in office later next week.

@gyfora
Copy link
Contributor

gyfora commented Aug 9, 2024

@angelachenn no hurries I was just curious if you were planning to continue at some point :)
Thank you!

@angelachenn angelachenn force-pushed the FLINK-33803-cleanup branch 3 times, most recently from 8ba93c4 to c7065fb Compare August 22, 2024 13:13
@angelachenn
Copy link
Contributor Author

hey @gyfora, my updated PR is ready for review, I've addressed your comment, tweaked some small things and also added a test for backwards compatibility. All checks pass on my forked branch & I've also tested the changes locally, although please advise if I've missed anything. thank you!
Screenshot 2024-08-22 at 10 31 58 AM

@gyfora gyfora merged commit a10fb45 into apache:main Aug 23, 2024
169 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants