Skip to content

Conversation

@jlpedrosa
Copy link
Contributor

What changes were proposed in this pull request?

This PR makes possible disable the negative dns caching in JVM by adding a variable.

How was this patch tested?

I could not see a place to run this tests automatically, so I run them manually by executing
docker run:

josemsmac:spark jopedros$ docker run --rm -it --env "DISABLE_DNS_NEGATIVE_CACHING=1" spark:latest executor
++ id -u
+ myuid=185
++ id -g
+ mygid=0
+ set +e
++ getent passwd 185
+ uidentry=
+ set -e
+ '[' -z '' ']'
+ '[' -w /etc/passwd ']'
+ echo '185:x:185:0:anonymous uid:/opt/spark:/bin/false'
+ SPARK_CLASSPATH=':/opt/spark/jars/*'
+ env
+ grep SPARK_JAVA_OPT_
+ sort -t_ -k4 -n
+ sed 's/[^=]*=\(.*\)/\1/g'
+ readarray -t SPARK_EXECUTOR_JAVA_OPTS
+ '[' -n '' ']'
+ '[' '' == 2 ']'
+ '[' '' == 3 ']'
+ '[' -z ']'
+ case "$1" in
+ shift 1
+ CMD=(${JAVA_HOME}/bin/java "${SPARK_EXECUTOR_JAVA_OPTS[@]}" -Xms$SPARK_EXECUTOR_MEMORY -Xmx$SPARK_EXECUTOR_MEMORY -cp "$SPARK_CLASSPATH")
+ '[' -z x ']'
+ MOD_OPTS_PATH=/tmp/jvm.java.security
+ sed -e s/networkaddress.cache.negative.ttl=10/networkaddress.cache.negative.ttl=0/g /usr/lib/jvm/java-1.8-openjdk/jre/lib/security/java.security
+ CMD+=(-Djava.security.properties=${MOD_OPTS_PATH})
+ CMD+=(org.apache.spark.executor.CoarseGrainedExecutorBackend --driver-url $SPARK_DRIVER_URL --executor-id $SPARK_EXECUTOR_ID --cores $SPARK_EXECUTOR_CORES --app-id $SPARK_APPLICATION_ID --hostname $SPARK_EXECUTOR_POD_IP)
+ exec /sbin/tini -s -- /usr/lib/jvm/java-1.8-openjdk/bin/java -Xms -Xmx -cp ':/opt/spark/jars/*' -Djava.security.properties=/tmp/jvm.java.security org.apache.spark.executor.CoarseGrainedExecutorBackend --driver-url --executor-id --cores --app-id --hostname

@jlpedrosa
Copy link
Contributor Author

Hi @srowen

This is the other part, affecting only to k8s.

@erikerlandson
Copy link
Contributor

This change has some hard-coded paths and sed code that will cause problems. @skonto proposed modifying the java.security using a config-map, which seems both safer and more flexible to user environment particulars.

@jlpedrosa
Copy link
Contributor Author

Hi @erikerlandson, @skonto

I've been reviewing the discussion we had in #24702. We clarified that we can't set individual via java args, only the full file via properties. Then @skonto sugested this aproach:

@jlpedrosa @srowen I still find it better to be able to load your own file eg. via java.security.properties. I dont really agree with this being hardcoded, at I least I would add it to the entrypoint script and do the sed there so it is configurable.

Which is exactly what I've implemented. But I am open to implement it in any other way.
When you say "config-map" not sure if you mean a

  • k8s config map created by spark-sbmit and then inject that map in the drivers and executors? (all java/scala code)

  • Add a argument to the spark submit as a variable and then alter in java code the java security?

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-28149][Kubernetes] Added variable to disable negative DNS caching [SPARK-28149][K8S] Added variable to disable negative DNS caching Jul 6, 2019
@jlpedrosa
Copy link
Contributor Author

Hi
@erikerlandson @skonto Any feedback?

@erikerlandson
Copy link
Contributor

@jlpedrosa I think the idiomatic way to do this (going forward) will be to:

  1. specify java security modifications via a file, mounted as a configmap.
  2. set the -Djava.security.properties via --driver-java-options

Config map support is slated to be supported via user supplied YAML, see:
https://issues.apache.org/jira/browse/SPARK-24434

This feature will land in 3.0, so I'd propose verifying that this can be done without new code starting with 3.0

@jlpedrosa
Copy link
Contributor Author

Hi @erikerlandson

Looking at the docs of master, I see that it si not possible to mount a config map as a file. The only types of mounts supported are:

hostPath: mounts a file or directory from the host node’s filesystem into a pod.
emptyDir: an initially empty volume created when a pod is assigned to a node.
persistentVolumeClaim: used to mount a PersistentVolume into a pod.

Do you want me to open a separate ticket to add that feature? "Capability to mount config maps"
Do you preffer to change the approach? Do you want me to implement that feature in this ticket?

@erikerlandson
Copy link
Contributor

@jlpedrosa I'm guessing no specific doc was added because config-files were intended to be just one of an arbitrary number of things that could be managed via templates, starting with #22146

If config-files are not working via pod template files, that needs to be reconciled somehow.

@jlpedrosa
Copy link
Contributor Author

Hi @erikerlandson

I'm not sure if I've explained myself. What I was trying to say is that spark (3.0/master) has capabilities to mount different types of volumes into the pods without temaplates.
Using the args to spark submit:

spark.kubernetes.driver.volumes.[VolumeType].[VolumeName].mount.path 
Add the Kubernetes Volume named VolumeName of the VolumeType type to the driver pod on the path specified in the value. For example,spark.kubernetes.driver.volumes.persistentVolumeClaim.checkpointpvc.mount.path=/checkpoint.

That [VolumeType] can be only of the types I mentioned before (as in the doc).
Then the question is, do you prefer to:

  • Improve args in spark submit so it can mount a configMap?
  • Mount the config map through templates?

If we would extend the capabilities then it would be something like

spark-submit 
--spark.kubernetes.driver.volumes.configMap.myJavaSecOpts.mount.path=/opt/secopt
--driver-java-options  "-Djava.security.properties=/opt/secopt"

@vanzin
Copy link
Contributor

vanzin commented Jul 24, 2019

Parachuting into the conversation: we should not extend the config system to replicate functionality available through pod templates. That was the whole point of pod templates...

@jlpedrosa
Copy link
Contributor Author

jlpedrosa commented Jul 25, 2019

Hi @vanzin @erikerlandson

I've tested the approach suggested and it works, the process is a bit cumbersome for the user. So it is a question of usability,

  1. Create docker spark images (generic ones, in my case I used master)
  2. Create a modified version of the java security file.
  3. Push the config map to k8s
  4. Create a template that contains the volumes and the mounts
  5. Create custom spark images based on 1) including 2) (-java.security.properties can be only mounted from local.)
  6. Run spark submit using the images on 5) and using 4) as a template:
--conf spark.kubernetes.driver.podTemplateFile=....vtemplate.yaml \
--conf 'spark.driver.extraJavaOptions=-Djava.security.properties=/etc/config_mount/jvm.java.security'     

sample template:

spec:
  containers:
    - volumeMounts:
      - name: config-volume
        mountPath: /etc/config_mount
  volumes:
    - name: config-volume
      configMap:
        name: jvm-opts-no-dns-caching

@vanzin
Copy link
Contributor

vanzin commented Jul 25, 2019

Instead of trying to modify the java.security file, how about adding code in Spark to change the property programmatically?

Seems like all you have to do is call:

java.security.Security.setProperty("networkaddress.cache.negative.ttl" , "0");

Just need to add code so that is done during driver and executor startup somehow. Shouldn't be hard.

@erikerlandson
Copy link
Contributor

Hardcoding this behavior seems not quite right to me on general principles. However, the question would be "what if the DNS failure was 'real' instead of just a transitory startup artifact?" And maybe from a security point of view "what if the failure was part of some attack - trying to connect to something hostile?" In the benign case maybe not so bad. I'm less sure about a hypothetical vulnerability case.

Thinking about the original problem I have a more basic question: I am wondering why it is not possible to reconfigure longer timeouts.

@vanzin
Copy link
Contributor

vanzin commented Jul 25, 2019

Nobody suggested hardcoding anything.

@erikerlandson
Copy link
Contributor

@jlpedrosa my working assumption was that the security file could be mounted as a config-map, without having to mess with custom images (steps 1 & 5). If that is truly necessary, then I'd consider it a reasonable case for some kind of code modification.

As @vanzin alluded to above, the motivation for pod templates was to make k8s-related modifications possible without having to re-code the back end to pass through k8s config every time a user wanted to set some new feature. However, there was also the implicit assumption that it should be possible without a great deal of gymnastics from the user.

@jlpedrosa
Copy link
Contributor Author

@erikerlandson I'd say step 1) should be and standard good practice to keep them in your own repos (not even sure if there are in public "official" repos for the spark images, quick google does not seem to found official ones). Point 5) was clearly miss explained by me, k8s best practices would say that if you change your code, you should upload a new version of the image (instead of loading it from hdfs). The file can be mounted just with the template and pushing the config map. Image 5 should contain only the generated jar with the job, not the file.
So an existing image working (made with steps 1) and 5)) can be modified by creating the security file, a map with that file and running spark submit with the right set of options.

As you can see on #24702 Initially I proposed to go the approach @vanzin suggested (which I agree with). Taking aside the "changing existing behaviour", personally I think that default behaviour of java of caching forever DNS resolution failures is a bit undesirable choice by default. I don't think that is done by any other programming language by default.

I'd suggest to add a new config property to spark so it is actionable for all schedulers, and keep the JVM default behaviour (as it was). But I am open to implement it in any way you guys consider, just let me know the approach.

@jlpedrosa
Copy link
Contributor Author

Other benefit I could see to the approach suggested by @vanzin is that it would be easier to be ported to 2.4 branch.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@vanzin
Copy link
Contributor

vanzin commented Dec 10, 2019

I'll close this for now, feel free to update your branch to reopen the PR.

@vanzin vanzin closed this Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants