[FLINK-9762] CoreOptions.TMP_DIRS ('io.tmp.dirs') wrongly managed on Yarn #6284

JTaky · 2018-07-09T09:55:12Z

What is the purpose of the change

Currently TMP_DIRS (io.tmp.dirs) option has a bug in case of default behaviour on Yarn platform:
If not value has been set via parameter on Yarn platform the env value 'LOCAL_DIRS' is used on Application Master.
When task manager is started this 'LOCAL_DIRS' is passed to the TaskManager, so TaskManager do not understand that he needs to have a default behaviour.
The proposed ~~hack~~ fix is to remove TMP_DIRS value in case if default behaviour is required before starting TaskManager.

Brief change log

Extract method which set io.tmp.dirs configuration value
Fix configuration pass to TaskManager (I am not sure what is the best way to implement this):
** Add a new flag, saying io.tmp.dirs is value has been overridden
** If io.tmp.dirs has not been overridden we remove io.tmp.dirs remove config value, by setting the empty value (should we remove? clone without a given key?)
Correction of the documentation

Verifying this change

This change is already covered by existing tests, such as MiniClusterResource for Mesos part.

I do not see a way to test launching of the TaskManager with a correct configuration.
A unit test documenting specific logic of TMP_DIRS managing is added: BootstrapToolsTest#testSetTmpDirectoriesConfig

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 changed class annotated with @Public(Evolving): no
The serializers: no
The runtime per-record code paths (performance sensitive): no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: no
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? no

yanghua · 2018-07-10T11:40:46Z

find related test error, please recheck~

tillrohrmann

Thanks for your contribution @JTaky. I had some comments how we could streamline it a bit more.

Did you test the changes on a cluster? I think the Yarn part should not have worked since we passed the cloned config as the wrong argument to Utils#createTaskExecutorContext.

tillrohrmann · 2018-07-12T15:18:51Z

flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java

@@ -181,12 +181,17 @@
 	 * The config parameter defining the directories for temporary files, separated by
 	 * ",", "|", or the system's {@link java.io.File#pathSeparator}.
 	 */
-	@Documentation.OverrideDefault("System.getProperty(\"java.io.tmpdir\")")
+	@Documentation.OverrideDefault("'LOCAL_DIRS' on Yarn and '_FLINK_TMP_DIR' on Mesos.")


We should also keep saying that in the standalone mode we use System.getProperty("java.io.tmpdir")

tillrohrmann · 2018-07-12T15:39:05Z

flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java

+	public static void setTmpDirectoriesConfig(Configuration configuration, String defaultDirs){
+		if (configuration.contains(CoreOptions.TMP_DIRS) && !configuration.getString(CoreOptions.TMP_DIRS).isEmpty()) {
+			LOG.info("Overriding Fink's temporary file directories with those " +
+				"specified in the Flink config: " + configuration.getValue(CoreOptions.TMP_DIRS));


Should use logging placeholder {}

tillrohrmann · 2018-07-12T15:43:47Z

flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java

+	 * @param configuration flink config to patch
+	 * @param defaultDirs in case no tmp directories is set, next directories will be applied
+	 */
+	public static void setTmpDirectoriesConfig(Configuration configuration, String defaultDirs){


Should we maybe call it updateTmpDirectoriesInConfiguration?

tillrohrmann · 2018-07-12T15:48:38Z

flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java

+	/**
+	 * String key, which says if variable `java.io.tmpdir` has been overridden for the cluster.
+	 */
+	public static final String TMP_DIRS_OVERRIDDEN = "io.tmp.dirs.overridden";


Let's make this a proper ConfigOption<Boolean> with name USE_LOCAL_DEFAULT_TMP_DIRS key name internal.io.tmp.dirs.use-local-default

tillrohrmann · 2018-07-12T15:52:49Z

flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java

@@ -250,6 +250,10 @@ public static Configuration generateTaskManagerConfiguration(
 			cfg.setInteger(TaskManagerOptions.NUM_TASK_SLOTS, numSlots);
 		}

+		if (!baseConfig.containsKey(CoreOptions.TMP_DIRS_OVERRIDDEN)){
+			cfg.setString(CoreOptions.TMP_DIRS, "");	// HACK: emulate removal for the given key
+		}


I would suggest to add a BootstratpTools#cloneConfiguration(Configuration) method which clones the given Configuration and resets all node specific fields. One of these fields is CoreOptions#TMP_DIRS. This means if USE_LOCAL_DEFAULT_TMP_DIRS is true, then we clear the TMP_DIRS option. We actually need for that a Configuration#clear(ConfigOption) method which removes the option entry from Configuration#confData. Otherwise we might risk that we don't use the TMP_DIRS default value but instead "" if there are no local default tmp dirs.

Agree, magic values are the dirtiest way.
Will go with a 'clear' (probably remove, in order to mimic java collection API) method.

Didn't get the point with clone method. Is it in order to factorize this 4 lines or do you want to make it generic and use for all custom settings? In such case we should extract list of predicates per each configuration which looks quite complex as an API

tillrohrmann · 2018-07-12T15:53:42Z

flink-yarn/src/main/java/org/apache/flink/yarn/YarnResourceManager.java

+		Configuration taskManagerConfig = flinkConfig.clone();
+		if (!flinkConfig.containsKey(CoreOptions.TMP_DIRS_OVERRIDDEN)){
+			taskManagerConfig.setString(CoreOptions.TMP_DIRS, "");	// HACK: emulate removal for the given key
+		}


Replace with BootstratpTools#cloneConfiguration(Configuration) in order to reduce code duplication.

same question as before :( didn't get the idea

tillrohrmann · 2018-07-12T15:54:00Z

flink-yarn/src/main/java/org/apache/flink/yarn/YarnResourceManager.java

@@ -473,8 +474,13 @@ private ContainerLaunchContext createTaskExecutorLaunchContext(Resource resource

 		log.debug("TaskManager configuration: {}", flinkConfig);


log the flinkConfig after cloning/resetting partial values.

tillrohrmann · 2018-07-12T16:02:43Z

flink-yarn/src/main/java/org/apache/flink/yarn/YarnResourceManager.java

 		ContainerLaunchContext taskExecutorLaunchContext = Utils.createTaskExecutorContext(
-			flinkConfig,


here we should keep the flinkConfig and instead replace the 5th argument with taskManagerConfig because it is this Configuration which is given to the new TaskManager container.

done. Thank you very much!
Since you seem fine with this hacky approach I have tested and made stable the last PR on our Yarn cluster.

tillrohrmann

Thanks for the update @JTaky. Merging this PR now.

This closes apache#6284.

This closes #6284.

This closes apache#6284.

JTaky force-pushed the tmpDirs branch 2 times, most recently from a4b15dc to 106e25e Compare July 10, 2018 08:07

JTaky force-pushed the tmpDirs branch 4 times, most recently from 2cbd8e3 to 8048552 Compare July 12, 2018 10:48

tillrohrmann requested changes Jul 12, 2018

View reviewed changes

JTaky force-pushed the tmpDirs branch 2 times, most recently from 85c34ba to 684aa97 Compare July 16, 2018 19:40

Oleksandr Nitavskyi added 3 commits July 17, 2018 07:49

[FLINK-9762][yarn] Extract method: updateTmpDirectoriesInConfiguration

99589e6

[FLINK-9762] fix documenatation for 'io.tmp.dirs' config

aad059a

[FLINK-9762][yarn] Fix TMP_DIRS configuration passing

ea4508d

JTaky force-pushed the tmpDirs branch from 684aa97 to ea4508d Compare July 17, 2018 05:59

tillrohrmann approved these changes Jul 18, 2018

View reviewed changes

tillrohrmann pushed a commit to tillrohrmann/flink that referenced this pull request Jul 18, 2018

[FLINK-9762][yarn] Use local default tmp directories on Yarn and Mesos

a603258

This closes apache#6284.

tillrohrmann pushed a commit to tillrohrmann/flink that referenced this pull request Jul 19, 2018

[FLINK-9762][yarn] Use local default tmp directories on Yarn and Mesos

1e1a8ff

This closes apache#6284.

asfgit pushed a commit that referenced this pull request Jul 19, 2018

[FLINK-9762][yarn] Use local default tmp directories on Yarn and Mesos

877cd7e

This closes #6284.

asfgit closed this in ec28f92 Jul 19, 2018

tillrohrmann pushed a commit to tillrohrmann/flink that referenced this pull request Jul 19, 2018

[FLINK-9762][yarn] Use local default tmp directories on Yarn and Mesos

b505e52

This closes apache#6284.

sampathBhat pushed a commit to sampathBhat/flink that referenced this pull request Jul 26, 2018

[FLINK-9762][yarn] Use local default tmp directories on Yarn and Mesos

950fb89

This closes apache#6284.

rmetzger added the component=Deployment/YARN label Mar 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-9762] CoreOptions.TMP_DIRS ('io.tmp.dirs') wrongly managed on Yarn #6284

[FLINK-9762] CoreOptions.TMP_DIRS ('io.tmp.dirs') wrongly managed on Yarn #6284

JTaky commented Jul 9, 2018

yanghua commented Jul 10, 2018

tillrohrmann left a comment

tillrohrmann Jul 12, 2018

JTaky Jul 13, 2018

tillrohrmann Jul 12, 2018

JTaky Jul 13, 2018

tillrohrmann Jul 12, 2018

JTaky Jul 13, 2018

tillrohrmann Jul 12, 2018

JTaky Jul 16, 2018

tillrohrmann Jul 12, 2018

JTaky Jul 16, 2018

tillrohrmann Jul 12, 2018

JTaky Jul 16, 2018

tillrohrmann Jul 12, 2018

JTaky Jul 16, 2018

tillrohrmann Jul 12, 2018

JTaky Jul 16, 2018

tillrohrmann left a comment

		@@ -473,8 +474,13 @@ private ContainerLaunchContext createTaskExecutorLaunchContext(Resource resource

		log.debug("TaskManager configuration: {}", flinkConfig);

		ContainerLaunchContext taskExecutorLaunchContext = Utils.createTaskExecutorContext(
		flinkConfig,

[FLINK-9762] CoreOptions.TMP_DIRS ('io.tmp.dirs') wrongly managed on Yarn #6284

[FLINK-9762] CoreOptions.TMP_DIRS ('io.tmp.dirs') wrongly managed on Yarn #6284

Conversation

JTaky commented Jul 9, 2018

What is the purpose of the change

Brief change log

Verifying this change

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

Documentation

yanghua commented Jul 10, 2018

tillrohrmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillrohrmann left a comment

Choose a reason for hiding this comment