Spark dcos CLI breaks with multiple spaces #193

skonto · 2017-10-16T10:35:11Z

Spaces between arguments break the spark cli:
https://github.com/mesosphere/spark-build/blob/master/cli/dcos-spark/submit_builder.go#L302

This will fail when the job is run:

dcos spark run --verbose --submit-args="--conf spark.streaming.fileStream.minRememberDuration=1200s__--driver-cores 1 --conf spark.cores.max=4 --driver-memory 1024M --class com.lightbend.fdp.spark.test.FileStreamWithCheckpointing https://s3-eu-west-1.amazonaws.com/fdp-stavros-test/checkpointing-test-assembly-1.0-SNAPSHOT.jar --checkpointDirectory /checkpoint --outputDirectory /output --inputDirectory /watched"dcos spark run --verbose --submit-args="--conf spark.streaming.fileStream.minRememberDuration=1200s --driver-cores 1 --conf spark.cores.max=4 --driver-memory 1024M --class com.lightbend.fdp.spark.test.FileStreamWithCheckpointing https://s3-eu-west-1.amazonaws.com/fdp-stavros-test/checkpointing-test-assembly-1.0-SNAPSHOT.jar --checkpointDirectory /checkpoint --outputDirectory /output --inputDirectory /watched"

Error is not informative on mesos:
I1016 10:31:50.852361 7563 fetcher.cpp:285] Fetching directly into the sandbox directory
I1016 10:31:50.852404 7563 fetcher.cpp:222] Fetching URI ''
E1016 10:31:50.852429 7563 fetcher.cpp:579] EXIT with status 1: Failed to fetch '': A relative path was passed for the resource but the Mesos framework home was not specified. Please either provide this config option or avoid using a relative path

There are two spaces between those args: --conf spark.streaming.fileStream.minRememberDuration=1200s --driver-cores 1 --conf. I am using underscore to show that.

Another case is when we have:
--conf spark.app.name=kerberosStreaming
Two spaces again.
This will make dcos cli fail immediately:
2017/10/16 13:33:40 Error when parsing --submit-args: expected KEY=VALUE got ''

skonto · 2017-10-16T10:37:57Z

@susanxhuynh @ArtRand pls review.

elezar · 2017-10-16T11:12:57Z

cli/dcos-spark/submit_builder.go

 	// clean up any instances of shell-style escaped newlines: "arg1\\narg2" => "arg1 arg2"
-	argsCleaned := strings.TrimSpace(backslashNewlinePattern.ReplaceAllLiteralString(argsStr, " "))
+	argsCleaned := strings.TrimSpace(backslashNewlinePattern.ReplaceAllLiteralString(argsCompacted, " "))
 	// HACK: spark-submit uses '--arg val' by convention, while kingpin only supports '--arg=val'.
 	//       translate the former into the latter for kingpin to parse.
 	args := strings.Split(argsCleaned, " ")


Another option would be to split argsCleaned using a Regexp.Split instead of strings.Split:

package main import ( "fmt" "regexp" ) func main() { argString := "one two three four" splitter := regexp.MustCompile(`\s+`) for _, s := range splitter.Split(argString, -1) { fmt.Println(s) } }

It would also be nice to add submit_builder_test.go with some unit tests for the various cases here.

thnx @elezar .Yes there are multiple options to fix this I guess. I agree we need tests since this is not the only issue found so far (probably will need to address it in a separate PR). @susanxhuynh are you running any tests for the cli internally?

As far as I know, there are no additional tests for this kind of behaviour, and adding a simple unit tests here would add great value. If you don't get to it, I will see if I can add something this weekend.

@elezar ok I will check what tests I could add here. As I said though missing tests could be addressed with another PR... @susanxhuynh are there any cli tests for this part, should we add tests here?

The CLI is "tested" by the integration tests, they will fail fast if it's not working. Simple unit tests are fine, but IMO opinion we could just add to the CI tests given that the unit tests would be very simple (akin to getter/setter basically).

skonto · 2017-10-18T13:07:54Z

@susanxhuynh @elezar I added a basic unit test. @susanxhuynh Let me know if I need to trigger testing in more places. Is the setup ok for jenkins?

skonto · 2017-10-18T13:09:30Z

cli/bin/test.sh

+
+cd "${BIN_DIR}/.."
+
+if [ -z "$GOPATH" -o -z "$(which go)" ]; then


@susanxhuynh This part is also found in build-go.sh and it is repeated here. I don't like bash include files but maybe we should add it in one. Let's verify that this works for the jenkins stuff first.

@skonto How about remove this file, and just add go test to the build script, build-go.sh? (https://github.com/mesosphere/spark-build/blob/master/cli/bin/build-go.sh) That's how we have it in dcos-commons: we run unit tests inside the build script: https://github.com/mesosphere/dcos-commons/blob/master/tools/build_go_exe.sh#L88

I would almost prefer that we don't add it to build-go.sh so that you can build the CLI without invoking the tests (or commenting out lines then forgetting to uncomment them). FWIW I usually build the CLI then just copy it to my .dcos/ dir for testing the CLI.

skonto · 2017-10-18T17:04:32Z

@susanxhuynh could you assist with failures and how to integrate with the jenkins setup.
Btw I also tried:

go test -cover -coverprofile=c.out  
go tool cover -html=c.out -o coverage.html

Test coverage for the submit code (submit_builder.go) is 23.9% which is misleading because although that part of the code was triggered not many conditions were checked in the single unit test anyway.

susanxhuynh · 2017-10-19T14:12:21Z

Yeah, I'll check why the jenkins build failed, and figure out how to run the test in CI.

susanxhuynh · 2017-10-19T17:26:41Z

retest this please

susanxhuynh

I'm still debugging the Jenkins failure ... seems like an unrelated failure with Hadoop S3 library.

susanxhuynh · 2017-10-19T15:36:41Z

cli/dcos-spark/cli_test.go

+
+import "testing"
+
+// verify kingpin gets --arg=val


This comment doesn't seem to match. How about mentioning testing the multiple spaces?

would it also be hard to add a test for scopt args?

Ok will add a test sure.

susanxhuynh · 2017-10-19T15:58:19Z

cli/bin/test.sh

+
+cd "${BIN_DIR}/.."
+
+if [ -z "$GOPATH" -o -z "$(which go)" ]; then


@skonto How about remove this file, and just add go test to the build script, build-go.sh? (https://github.com/mesosphere/spark-build/blob/master/cli/bin/build-go.sh) That's how we have it in dcos-commons: we run unit tests inside the build script: https://github.com/mesosphere/dcos-commons/blob/master/tools/build_go_exe.sh#L88

ArtRand

I'd leave it to a casual vote, CLI unit tests or add to the CI tests (by adding edge cases to the commands invoked). Either way, this change is good, just add the appropriate test.

ArtRand · 2017-10-22T13:25:01Z

cli/bin/test.sh

+
+cd "${BIN_DIR}/.."
+
+if [ -z "$GOPATH" -o -z "$(which go)" ]; then


I would almost prefer that we don't add it to build-go.sh so that you can build the CLI without invoking the tests (or commenting out lines then forgetting to uncomment them). FWIW I usually build the CLI then just copy it to my .dcos/ dir for testing the CLI.

ArtRand · 2017-10-22T13:25:48Z

cli/dcos-spark/cli_test.go

+
+import "testing"
+
+// verify kingpin gets --arg=val


would it also be hard to add a test for scopt args?

ArtRand · 2017-10-22T13:28:24Z

cli/dcos-spark/submit_builder.go

 	// clean up any instances of shell-style escaped newlines: "arg1\\narg2" => "arg1 arg2"
-	argsCleaned := strings.TrimSpace(backslashNewlinePattern.ReplaceAllLiteralString(argsStr, " "))
+	argsCleaned := strings.TrimSpace(backslashNewlinePattern.ReplaceAllLiteralString(argsCompacted, " "))
 	// HACK: spark-submit uses '--arg val' by convention, while kingpin only supports '--arg=val'.
 	//       translate the former into the latter for kingpin to parse.
 	args := strings.Split(argsCleaned, " ")


The CLI is "tested" by the integration tests, they will fail fast if it's not working. Simple unit tests are fine, but IMO opinion we could just add to the CI tests given that the unit tests would be very simple (akin to getter/setter basically).

skonto · 2017-10-23T20:11:44Z

@susanxhuynh I updated the PR.

susanxhuynh

@skonto I just merged a fix to the CI tests (#195). Would you mind rebasing? Also, could you move the unit tests a little earlier, see comment:

susanxhuynh · 2017-10-23T20:29:36Z

cli/bin/build-go.sh

@@ -57,3 +57,5 @@ case "$OSTYPE" in
    linux*) strip "${EXE_NAME}${SUFFIX}"
 esac
 print_file "${EXE_NAME}${SUFFIX}"
+
+go test


@skonto Could you add this earlier, like around L. 37? So the logic would be: if the unit tests do not pass, we do not bother building the (Windows, OSX, Linux) executables.

Ok I thought you build first and then test, sure.

@susanxhuynh I dont need rebasing I think, I touched different files.

@skonto My recent fix is needed to get one of the CI tests to pass (something changed in hdfs and I had to add a workaround). So, I think you have to include that fix in order for the Jenkins tests to pass for this PR.

susanxhuynh · 2017-10-24T00:19:47Z

@skonto Only thing missing is rebasing to pull in my CI fix. Otherwise, CI test ("test:shakedown") will not pass in this PR.

skonto · 2017-10-24T20:26:28Z

@susanxhuynh done

elezar reviewed Oct 16, 2017

View reviewed changes

skonto force-pushed the fix_spaces branch from 0497430 to 60b4b83 Compare October 18, 2017 13:06

skonto commented Oct 18, 2017

View reviewed changes

susanxhuynh reviewed Oct 19, 2017

View reviewed changes

ArtRand suggested changes Oct 22, 2017

View reviewed changes

susanxhuynh reviewed Oct 23, 2017

View reviewed changes

skonto force-pushed the fix_spaces branch from 6f22e86 to 61259b0 Compare October 24, 2017 20:20

fix spaces

19abd2a

skonto force-pushed the fix_spaces branch from 61259b0 to 19abd2a Compare October 24, 2017 20:23

susanxhuynh merged commit 17e2483 into d2iq-archive:master Oct 24, 2017

susanxhuynh mentioned this pull request Oct 25, 2017

Added integration test for CLI multiple spaces bug #197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spark dcos CLI breaks with multiple spaces #193

Spark dcos CLI breaks with multiple spaces #193

skonto commented Oct 16, 2017 •

edited

Loading

skonto commented Oct 16, 2017

elezar Oct 16, 2017

skonto Oct 16, 2017 •

edited

Loading

elezar Oct 17, 2017

skonto Oct 17, 2017 •

edited

Loading

ArtRand Oct 22, 2017

skonto commented Oct 18, 2017

skonto Oct 18, 2017 •

edited

Loading

susanxhuynh Oct 19, 2017

ArtRand Oct 22, 2017

skonto commented Oct 18, 2017 •

edited

Loading

susanxhuynh commented Oct 19, 2017

susanxhuynh commented Oct 19, 2017

susanxhuynh left a comment

susanxhuynh Oct 19, 2017

ArtRand Oct 22, 2017

skonto Oct 23, 2017

susanxhuynh Oct 19, 2017

ArtRand left a comment

ArtRand Oct 22, 2017

ArtRand Oct 22, 2017

ArtRand Oct 22, 2017

skonto commented Oct 23, 2017

susanxhuynh left a comment

susanxhuynh Oct 23, 2017

skonto Oct 23, 2017

skonto Oct 23, 2017

skonto Oct 23, 2017 •

edited

Loading

susanxhuynh Oct 23, 2017

susanxhuynh commented Oct 24, 2017

skonto commented Oct 24, 2017


		cd "${BIN_DIR}/.."

		if [ -z "$GOPATH" -o -z "$(which go)" ]; then

Spark dcos CLI breaks with multiple spaces #193

Spark dcos CLI breaks with multiple spaces #193

Conversation

skonto commented Oct 16, 2017 • edited Loading

skonto commented Oct 16, 2017

Choose a reason for hiding this comment

skonto Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto commented Oct 18, 2017

skonto Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto commented Oct 18, 2017 • edited Loading

susanxhuynh commented Oct 19, 2017

susanxhuynh commented Oct 19, 2017

susanxhuynh 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

ArtRand 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

skonto commented Oct 23, 2017

susanxhuynh 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

skonto Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

susanxhuynh commented Oct 24, 2017

skonto commented Oct 24, 2017

skonto commented Oct 16, 2017 •

edited

Loading

skonto Oct 16, 2017 •

edited

Loading

skonto Oct 17, 2017 •

edited

Loading

skonto Oct 18, 2017 •

edited

Loading

skonto commented Oct 18, 2017 •

edited

Loading

skonto Oct 23, 2017 •

edited

Loading