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/06/03 11:01:02 UTC
[GitHub] [flink-kubernetes-operator] morhidi opened a new pull request, #254: Flink 27812
morhidi opened a new pull request, #254:
URL: https://github.com/apache/flink-kubernetes-operator/pull/254
This PR enables handling namespace changes via Dynamically Operator Configuration through the config option: `kubernetes.operator.watched.namespaces`. Watched namespaces originally were defined using environment variables, thus namespaces changes required a POD restart always. With the current feature Helm upgrades won't trigger an automatic POD restart on namespace changes anymore.
Suggestions for testing:
```
helm install flink-kubernetes-operator helm/flink-kubernetes-operator --set "watchNamespaces={default}"
helm upgrade flink-kubernetes-operator helm/flink-kubernetes-operator --set "watchNamespaces={default,flink}"
```
The upgrade will update the config map, but it takes some time until the new configs are propagated in the operator. Worth setting the `kubernetes.operator.dynamic.config.check.interval: 10 s` for local testing.
I would be happy to get your feedback on this @wangyang0918 @Aitozi @gyfora @tweise
--
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 #254: [FLINK-27812] Support Dynamic Change of Watched Namespaces
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #254:
URL: https://github.com/apache/flink-kubernetes-operator/pull/254#discussion_r900053365
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/KubernetesOperatorConfigOptions.java:
##########
@@ -187,4 +188,18 @@ public class KubernetesOperatorConfigOptions {
.withDescription(
"Interval at which periodic savepoints will be triggered. "
+ "The triggering schedule is not guaranteed, savepoints will be triggered as part of the regular reconcile loop.");
+
+ public static final ConfigOption<String> OPERATOR_WATCHED_NAMESPACES =
+ ConfigOptions.key("kubernetes.operator.watched.namespaces")
+ .stringType()
+ .defaultValue(Constants.WATCH_ALL_NAMESPACES)
+ .withDescription(
+ "Comma separated list of namespaces the operator monitors for custom resources. Defaults to "
+ + Constants.WATCH_ALL_NAMESPACES);
+
+ public static final ConfigOption<Boolean> OPERATOR_DYNAMIC_NAMESPACES_ENABLED =
+ ConfigOptions.key("kubernetes.operator.dynamic.namespaces.enabled")
+ .booleanType()
+ .defaultValue(false)
+ .withDescription("Enables dynamic change of namespaces. Defaults to false");
Review Comment:
makes sense, will change it, thanks.
--
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 #254: [FLINK-27812] Support Dynamic Change of Watched Namespaces
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #254:
URL: https://github.com/apache/flink-kubernetes-operator/pull/254#discussion_r900053365
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/KubernetesOperatorConfigOptions.java:
##########
@@ -187,4 +188,18 @@ public class KubernetesOperatorConfigOptions {
.withDescription(
"Interval at which periodic savepoints will be triggered. "
+ "The triggering schedule is not guaranteed, savepoints will be triggered as part of the regular reconcile loop.");
+
+ public static final ConfigOption<String> OPERATOR_WATCHED_NAMESPACES =
+ ConfigOptions.key("kubernetes.operator.watched.namespaces")
+ .stringType()
+ .defaultValue(Constants.WATCH_ALL_NAMESPACES)
+ .withDescription(
+ "Comma separated list of namespaces the operator monitors for custom resources. Defaults to "
+ + Constants.WATCH_ALL_NAMESPACES);
+
+ public static final ConfigOption<Boolean> OPERATOR_DYNAMIC_NAMESPACES_ENABLED =
+ ConfigOptions.key("kubernetes.operator.dynamic.namespaces.enabled")
+ .booleanType()
+ .defaultValue(false)
+ .withDescription("Enables dynamic change of namespaces. Defaults to false");
Review Comment:
makes sense
--
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 #254: [FLINK-27812] Support Dynamic Change of Watched Namespaces
Posted by GitBox <gi...@apache.org>.
gyfora merged PR #254:
URL: https://github.com/apache/flink-kubernetes-operator/pull/254
--
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 #254: [FLINK-27812] Support Dynamic Change of Watched Namespaces
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #254:
URL: https://github.com/apache/flink-kubernetes-operator/pull/254#discussion_r900054810
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/FlinkConfigManager.java:
##########
@@ -109,14 +117,19 @@ public Configuration getDefaultConfig() {
@VisibleForTesting
public void updateDefaultConfig(Configuration newConf) {
- if (newConf.equals(defaultConfig)) {
+ if (defaultConfig != null
+ && newConf != null
+ && defaultConfig.toMap().equals(newConf.toMap())) {
LOG.info("Default configuration did not change, nothing to do...");
return;
}
LOG.info("Updating default configuration to {}", newConf);
- this.operatorConfiguration =
- FlinkOperatorConfiguration.fromConfiguration(newConf, namespaces);
+ this.operatorConfiguration = FlinkOperatorConfiguration.fromConfiguration(newConf);
+
+ if (this.operatorConfiguration.getDynamicNamespacesEnabled()) {
Review Comment:
Sure, I'll add an extra check 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] morhidi commented on a diff in pull request #254: [FLINK-27812] Support Dynamic Change of Watched Namespaces
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #254:
URL: https://github.com/apache/flink-kubernetes-operator/pull/254#discussion_r900053993
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/FlinkConfigManager.java:
##########
@@ -109,14 +117,19 @@ public Configuration getDefaultConfig() {
@VisibleForTesting
public void updateDefaultConfig(Configuration newConf) {
- if (newConf.equals(defaultConfig)) {
+ if (defaultConfig != null
+ && newConf != null
Review Comment:
Yes, this is needed
--
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 pull request #254: [FLINK-27812] Support Dynamic Change of Watched Namespaces
Posted by GitBox <gi...@apache.org>.
morhidi commented on PR #254:
URL: https://github.com/apache/flink-kubernetes-operator/pull/254#issuecomment-1158931070
Thank you for your review @gyfora I've addressed your comments.
--
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 #254: [FLINK-27812] Support Dynamic Change of Watched Namespaces
Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #254:
URL: https://github.com/apache/flink-kubernetes-operator/pull/254#discussion_r900031472
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/KubernetesOperatorConfigOptions.java:
##########
@@ -187,4 +188,18 @@ public class KubernetesOperatorConfigOptions {
.withDescription(
"Interval at which periodic savepoints will be triggered. "
+ "The triggering schedule is not guaranteed, savepoints will be triggered as part of the regular reconcile loop.");
+
+ public static final ConfigOption<String> OPERATOR_WATCHED_NAMESPACES =
+ ConfigOptions.key("kubernetes.operator.watched.namespaces")
+ .stringType()
+ .defaultValue(Constants.WATCH_ALL_NAMESPACES)
+ .withDescription(
+ "Comma separated list of namespaces the operator monitors for custom resources. Defaults to "
+ + Constants.WATCH_ALL_NAMESPACES);
+
+ public static final ConfigOption<Boolean> OPERATOR_DYNAMIC_NAMESPACES_ENABLED =
+ ConfigOptions.key("kubernetes.operator.dynamic.namespaces.enabled")
+ .booleanType()
+ .defaultValue(false)
+ .withDescription("Enables dynamic change of namespaces. Defaults to false");
Review Comment:
nit: `Enables dynamic change of watched/monitored namespaces`
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/FlinkConfigManager.java:
##########
@@ -109,14 +117,19 @@ public Configuration getDefaultConfig() {
@VisibleForTesting
public void updateDefaultConfig(Configuration newConf) {
- if (newConf.equals(defaultConfig)) {
+ if (defaultConfig != null
+ && newConf != null
+ && defaultConfig.toMap().equals(newConf.toMap())) {
LOG.info("Default configuration did not change, nothing to do...");
return;
}
LOG.info("Updating default configuration to {}", newConf);
- this.operatorConfiguration =
- FlinkOperatorConfiguration.fromConfiguration(newConf, namespaces);
+ this.operatorConfiguration = FlinkOperatorConfiguration.fromConfiguration(newConf);
+
+ if (this.operatorConfiguration.getDynamicNamespacesEnabled()) {
Review Comment:
should we check if thhe watched namespaces actually changed?
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/FlinkConfigManager.java:
##########
@@ -109,14 +117,19 @@ public Configuration getDefaultConfig() {
@VisibleForTesting
public void updateDefaultConfig(Configuration newConf) {
- if (newConf.equals(defaultConfig)) {
+ if (defaultConfig != null
+ && newConf != null
Review Comment:
Do we need these nullchecks here? Why would they ever be null?
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/informer/InformerManager.java:
##########
@@ -104,4 +100,24 @@ Map<String, SharedIndexInformer<CR>> createRunnableInformer(
return informers;
}
}
+
+ public void setNameSpaces(Set<String> watchedNamespaces) {
Review Comment:
nit: `setNameSpaces` -> `setNamespaces`
--
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 #254: [FLINK-27812] Support Dynamic Change of Watched Namespaces
Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #254:
URL: https://github.com/apache/flink-kubernetes-operator/pull/254#discussion_r900053993
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/config/FlinkConfigManager.java:
##########
@@ -109,14 +117,19 @@ public Configuration getDefaultConfig() {
@VisibleForTesting
public void updateDefaultConfig(Configuration newConf) {
- if (newConf.equals(defaultConfig)) {
+ if (defaultConfig != null
+ && newConf != null
Review Comment:
Yes, this is needed. During initialization the defaultConfig is null. The other null check is just an added extra.
--
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