Implement clientmode

[sudiptob2]

closes G-Research/spark#148
I attempted to implement client mode on top of the latest changes. Thanks for the initial POC by @GeorgeJahad

How to run

config.sh

export IMAGE_NAME="spark:armada"
export ARMADA_MASTER="local://armada://localhost:30002"
export ARMADA_QUEUE="test"
export ARMADA_LOOKOUT_URL="http://localhost:30000"
export INCLUDE_PYTHON=true
export USE_KIND=true
export SPARK_DRIVER_HOST="172.18.0.1"
export SPARK_DRIVER_PORT="7078"

1. Cluster mode + Dynamic allocation
   ./scripts/submitArmadaSpark.sh -M cluster -A dynamic 100

2. Cluster mode + Static allocation
   ./scripts/submitArmadaSpark.sh -M cluster -A static 100

3. Client mode + Dynamic allocation
   ./scripts/submitArmadaSpark.sh -M client -A dynamic 100

4. Client mode + Static allocation
   ./scripts/submitArmadaSpark.sh -M client -A static 100

I added an extra condition to the check based on ARMADA_JOB_SET_ID to determine clustermode, since this variable is only set in the driver’s environment in cluster mode. This is somewhat hacky, but it prevents extra executor requests and the E2E tests are now passing. Root cause of this problem still needs to be figured out and should be fixes when we implement E2E test for client mode.

[GeorgeJahad]

When I refactored, I created this trait to manage all the combinations deployment and allocation modes: https://github.com/armadaproject/armada-spark/blob/master/src/main/scala/org/apache/spark/deploy/armada/DeploymentModeHelper.scala#L22

That way we don't have to litter the code with if statements testing client/dyamic etc. Can you take a look and see if you can take more advantage of that approach?

[GeorgeJahad]

I can't remember the reason why these are different between client and cluster mode. Is there a reason why we shouldn't just make them both come from the config/appId?

[sudiptob2]

Not sure about the specific reason, but in a general sense, I don't see any reason for them to handle differently based on modes. Should come from config regardless of modes

[GeorgeJahad]

I agree. Please make it so.

[GeorgeJahad]

Instead of adding a new parameter here, can't we just have the mode helper return the driver hostname?

[GeorgeJahad]

I'd really prefer the rand to happen inside our scala code. The default should be random. If the user wants to override for some reason, he can, but I don't want us to have to do this here.

[sudiptob2]

Made some changes to fix this.

[sudiptob2]

The previous if check intended to ensure client mode, was not sufficient. As a result, additional executors were being requested even in cluster mode, which caused the E2E failures.

I added an extra condition to the check based on ARMADA_JOB_SET_ID, since this variable is only set in the driver’s environment in cluster mode. This is somewhat hacky, but it prevents extra executor requests and the E2E tests are now passing.

Please let me know if there’s a better way to reliably determine the mode without relying on ARMADA_JOB_SET_ID.

[GeorgeJahad]

lgtm, thanks @sudiptob2!