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

Support config yaml #848

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ctrlaltdilj
Copy link

Support Flink 1.19 config.yaml configuration

Flink 1.19 introduced new config.yaml format. There are some dependencies on this new format in 1.19 that prevent old flink-conf.yaml from being able to configure properties.

This PR, adds support to be able to set a config.yaml file which is able to be consumed by the operator as well as deployments.

Brief change log

  • added support to be able to set config.yaml by replacing flink-conf.yaml in the defaultConfiguration

Verifying this change

This change added tests and can be verified as follows:

  • Build image
  • create a values.yaml similar to
  • Extended integration test for recovery after master (JobManager) failure
  • Manually verified the change by running a 4 node cluster with 2 JobManagers and 4 TaskManagers, a stateful streaming program, and killing one JobManager and two TaskManagers during the execution, verifying that recovery happens correctly.

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

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

Documentation

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

@ctrlaltdilj
Copy link
Author

I do think this would be better supported via a flag on configuration on FlinkDeployment, as it enables operator update independently from FlinkDeployments

I am happy to make that change and would prefer that. Any thoughts?

@mateczagany
Copy link
Contributor

Hi, first of all, thank you for the contribution, but for better observability please try to always create a JIRA before opening a PR.
Unless trying to push a fix for a release candidate, please use the main branch for your PRs. Flink version is already 1.19.1 on the main branch.

@ctrlaltdilj
Copy link
Author

Thanks @mateczagany for all the info! I have requested a JIRA account, and will file a ticket

@ctrlaltdilj
Copy link
Author

@mateczagany
Created following ticket: https://issues.apache.org/jira/browse/FLINK-35744

@ctrlaltdilj ctrlaltdilj marked this pull request as draft July 2, 2024 14:57
@ctrlaltdilj ctrlaltdilj marked this pull request as draft July 2, 2024 14:57
@ctrlaltdilj ctrlaltdilj marked this pull request as draft July 2, 2024 14:57
@@ -154,6 +154,9 @@ defaultConfiguration:
# If set to false, loads just the overrides as in (2).
# This option has not effect, if create is equal to false.
append: true
# If set to true, then will support YAML 1.2 syntax through use of `config.yaml` file.
# If set to false, will make use of deprecated flink-conf.yaml filen.
standardYaml: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great if this would be "automatic" based on whether the user specified flink-conf.yaml vs conf.yaml. Do you think that's possible?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this approach does take care of this

@gyfora
Copy link
Contributor

gyfora commented Oct 3, 2024

Hey @ctrlaltdilj !
Are you still working on this PR?

@ctrlaltdilj
Copy link
Author

ctrlaltdilj commented Oct 3, 2024

@gyfora Sorry for the late response, Yes I can make the changes you mentioned, should be able to make it automatic based on the flink version being requested during deployments

Should have time to update this draft sometime later today

@myskaludek
Copy link

Hi , i made changes to helm template and in values set

defaultConfiguration:
  standardYaml: true

The operator dont start pod with errror

Exception in thread "main" org.apache.flink.configuration.IllegalConfigurationException: The Flink config file '/opt/flink/conf/flink-conf.yaml' (/opt/flink/conf/flink-conf.yaml) does not exist.

@hugovideira-ppb
Copy link

Hi @ctrlaltdilj any updates on this? Is there a chance of this moving forward? It would be a great addition!

@ctrlaltdilj
Copy link
Author

Hi @hugovideira-ppb , yes, apologize got busy, I'll try to knock this out

@ctrlaltdilj ctrlaltdilj force-pushed the support-config-yaml branch 2 times, most recently from 9eb3c38 to 670bfcf Compare March 8, 2025 15:44
@ctrlaltdilj ctrlaltdilj changed the base branch from release-1.9 to main March 8, 2025 15:46
@ctrlaltdilj
Copy link
Author

ctrlaltdilj commented Mar 8, 2025

@hugovideira-ppb , I have updated the helm chart, if this approach looks good, can I proceed with some documentation changes and adding some tests?

in the values.yaml you can replace, to test locally:

flink-conf.yaml: |+

to

config.yaml: |+

@ctrlaltdilj ctrlaltdilj requested review from gyfora and ljankows March 8, 2025 15:56
@ctrlaltdilj
Copy link
Author

@gyfora does this approach look better?

@ctrlaltdilj
Copy link
Author

@gyfora @ljankows whenever you get a chance to take a look, if the approach makes sense, I can proceed with some documentation changes and adding some tests?

@ctrlaltdilj ctrlaltdilj marked this pull request as ready for review March 15, 2025 03:44
@gyfora
Copy link
Contributor

gyfora commented Mar 19, 2025

I think this will require some more testing and some e2es to make sure that this type of configuration on the operator side still works with Flink versions.

Can we extend the e2e logic to cover this somehow?

@ctrlaltdilj
Copy link
Author

@gyfora that should be possible, let me take a look

@ctrlaltdilj ctrlaltdilj force-pushed the support-config-yaml branch from aabde8f to 40a1872 Compare April 6, 2025 20:09
@ctrlaltdilj
Copy link
Author

@gyfora added, let me know what you think, or if you have any feedback

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.

6 participants