Add uses bulkdata argument to paasta spark run #3995
Conversation
There was a problem hiding this comment.
lgtm (and i think it's perfectly fine to not check if there's a spark config for mounting /nail/bulkdata since it looks like that's not something anyone is currently doing - and i doubt any of our spark users would add such a mount un-prompted)
that said i'll let someone for ml compute ship since they own this file in its entirety :)
| "--uses-bulkdata", | ||
| help="Mount /nail/bulkdata in the container", | ||
| action="store_true", | ||
| default=False, |
There was a problem hiding this comment.
Should we not set the default to true for now, then roll out my change to add the flag everywhere, and then set the default to false?
There was a problem hiding this comment.
good point - I thought by default we are getting the /nail/bulkdata mount from the host, but I cant actually see where that is happening - these changes are to configure_and_run_docker_container - which calls run_docker_container which then calls os.execlpe("paasta_docker_wrapper", *docker_run_cmd, merged_env) which is defined here but none of these seem to include system_volumes from /etc/paasta/volumes.json so i'm actually unsure why spark-run is mounting the bulkdata volume at all currently
@nemacysts / @chi-yelp do you have any ideas where the /nail/bulkdata mount is happening? Or is there any way I can test this like I did with #3893 in the paasta playground
There was a problem hiding this comment.
hmm I checked using the following command and it seems /nail/bulkdata isn't mounted:
./.tox/py38-linux/bin/paasta spark-run --aws-profile=dev --cmd bashThere was a problem hiding this comment.
so volumes are found from the instance config here which is then passed to spark conf here and then later referenced here
Meaning it is important that I check if /nail/bulkdata is in the list of volumes before adding it. And the command @chi-yelp shared is not mounting bulkdata because its not specifying a service / instance that uses bulkdata - @chi-yelp was that command run on my branch?
There was a problem hiding this comment.
Sorry for the late reply, yes I ran the command on the branch of this PR after creating the virtualenv by make dev.
The mount paths will be deduplicated here by a dict, but I think it's also good for checking the input anyway
This makes the change to paasta spark run so that https://github.yelpcorp.com/sysgit/yelpsoa-configs/pull/52010 will work as expected I'm not checking here if the /nail/bulkdata volume is specified in the spark config, e.g `spark.kubernetes.executor.volumes.hostPath.0.mount.path=/nail/bulkdata` - doing this and setting uses_bulkdata set to True would result in multiple docker volumes being set which would cause a failure. This follows on from [this conversation in slack](https://yelp.slack.com/archives/CA8BWU65D/p1729768030212919) and will allow us to complete [this project](https://yelpwiki.yelpcorp.com/display/PRODENG/Project+Incredible+Bulk)
timmow
force-pushed
the
u/tmower/uses-bulkdata-spark-run-PERES-5194
branch
from
0b43710 to
c9de580
Compare
This makes the change to paasta spark run so that https://github.yelpcorp.com/sysgit/yelpsoa-configs/pull/52010 will work as expected. This works by adding the uses_bulkdata key to the intsance config if the spark job has the key present and set to true. I have added this arg to the tests so that they pass, however we're not explicitly testing that this functionality works. See #3995 for more informa tion about why we're doing this.
This makes the change to paasta spark run so that https://github.yelpcorp.com/sysgit/yelpsoa-configs/pull/52010 will work as expected. This works by adding the uses_bulkdata key to the intsance config if the spark job has the key present and set to true. I have added this arg to the tests so that they pass, and also created a test so that we can check all the different ways that uses_bulkdata can be set, either on paasta spark-run as an argument, or in the instance config. See #3995 for more information about why we're doing this.
|
closing in favor of #4005 |
This makes the change to paasta spark run so that
https://github.yelpcorp.com/sysgit/yelpsoa-configs/pull/52010 will work as expected
I'm not checking here if the /nail/bulkdata volume is specified in the spark config, e.g
spark.kubernetes.executor.volumes.hostPath.0.mount.path=/nail/bulkdataThis follows on from this conversation in
slack and will allow us to complete this
project