Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-31765][WEBUI][test-maven] Upgrade HtmlUnit >= 2.37.0 #28585

Closed
wants to merge 6 commits into from

Conversation

sarutak
Copy link
Member

@sarutak sarutak commented May 20, 2020

What changes were proposed in this pull request?

This PR upgrades HtmlUnit.
Selenium and Jetty also upgraded because of dependency.

Why are the changes needed?

Recently, a security issue which affects HtmlUnit is reported.
https://nvd.nist.gov/vuln/detail/CVE-2020-5529
According to the report, arbitrary code can be run by malicious users.
HtmlUnit is used for test so the impact might not be large but it's better to upgrade it just in case.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing testcases.

// QueuedThreadPoolExecutor#setIdleTimeout is changed and IllegalStateException
// will be thrown if we try to set idle timeout after the server has started.
// But this workaround works for Jetty 9.4.28 by ignoring the exception.
Try(pool.setIdleTimeout(0))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that as of Jetty 9.4.21, the implementation of QueuedThreadPool.
When QueuedThreadPool#setIdleTimeout is called, QueuedThreadPool#_idleTimeout is set to the new timeout as well as ReservedThreadExecutor#_idleTimeout.

If ReservedThreadExecutor#_idleTImeout is tried to be set after Jetty has started, IllegalStateException will be thrown.

But QueuedThreadPool#_idleTimeout will be set so this workaround will still work by ignoring the exception.

@SparkQA
Copy link

SparkQA commented May 20, 2020

Test build #122866 has finished for PR 28585 at commit f53735e.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@sarutak
Copy link
Member Author

sarutak commented May 20, 2020

Ah, I remember that some tests will fail because the latest HtmlUnit can't properly interpret JavaScript code used in those tests.
So PR should follow #28578 .

@srowen
Copy link
Member

srowen commented May 20, 2020

Looks OK pending tests

@HyukjinKwon
Copy link
Member

cc @gengliangwang too FYI. seems okay to me too

@@ -784,6 +784,7 @@ class UISeleniumSuite extends SparkFunSuite with WebBrowser with Matchers with B

eventually(timeout(10.seconds), interval(50.milliseconds)) {
goToUi(sc, "/jobs")

Copy link
Member Author

@sarutak sarutak May 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extra empty line is wrong change but after #28578 is merged, this testcase will be moved.

Copy link
Member

@gengliangwang gengliangwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the work!

@gengliangwang
Copy link
Member

merging to master

@sarutak
Copy link
Member Author

sarutak commented May 21, 2020

@gengliangwang This PR doesn't pass test.
As I mentioned here, #28578 should be merged first.

@gengliangwang
Copy link
Member

@sarutak sorry about that, let me do the revert

@SparkQA
Copy link

SparkQA commented May 22, 2020

Test build #122950 has finished for PR 28585 at commit f53735e.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented May 22, 2020

I merged #28578 ; this may need a rebase

@SparkQA
Copy link

SparkQA commented May 22, 2020

Test build #123015 has finished for PR 28585 at commit d950b42.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@sarutak
Copy link
Member Author

sarutak commented May 23, 2020

According to the last failure, I noticed that real headless browser support is needed for HistoryServer tests too.
I've open a PR for that.
#28622

srowen pushed a commit that referenced this pull request Jun 1, 2020
…ver tests

### What changes were proposed in this pull request?

This PR adds two things.

Real headless browser support for HistoryServer tests.
A test suite using headless Chrome as one instance of those browsers.

### Why are the changes needed?

The motivation is same as #28578 .
In the current master, there is a testcase for HistoryServer which uses Ajax so we need the support for HistoryServer tests.

Also this change is necessary to upgrade HtmlUnit (See #28585)

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

I tested with following patterns. Both Chrome and Chrome driver should be installed to test.
1. sbt / with default excluded tags (ChromeUIHistoryServerSuite is expected to be skipped and SQLQueryTestSuite is expected to succeed)
`build/sbt -Dspark.test.webdriver.chrome.driver=/path/to/chromedriver "testOnly org.apache.spark.deploy.history.ChromeUIHistoryServerSuite org.apache.spark.sql.SQLQueryTestSuite"

2. sbt / overwrite default excluded tags as empty string (Both suites are expected to succeed)
`build/sbt -Dtest.default.exclude.tags= -Dspark.test.webdriver.chrome.driver=/path/to/chromedriver "testOnly org.apache.spark.deploy.history.ChromeUIHistoryServerSuite org.apache.spark.sql.SQLQueryTestSuite"

3. sbt / set `test.exclude.tags` to `org.apache.spark.tags.ExtendedSQLTest` (Both suites are expected to be skipped)
`build/sbt -Dtest.exclude.tags=org.apache.spark.tags.ExtendedSQLTest -Dspark.test.webdriver.chrome.driver=/path/to/chromedriver "testOnly org.apache.spark.deploy.history.ChromeUIHistoryServerSuite org.apache.spark.sql.SQLQueryTestSuite"

4. Maven / with default excluded tags (ChromeUIHistoryServerSuite is expected to be skipped and SQLQueryTestSuite is expected to succeed)
`build/mvn -Dspark.test.webdriver.chrome.driver=/path/to/chromedriver -Dtest=none -DwildcardSuites=org.apache.spark.deploy.history.ChromeUIHistoryServerSuite,org.apache.spark.sql.SQLQueryTestSuite test`

5. Maven / overwrite default excluded tags as empty string (Both suites are expected to succeed)
`build/mvn -Dtest.default.exclude.tags= -Dspark.test.webdriver.chrome.driver=/path/to/chromedriver -Dtest=none -DwildcardSuites=org.apache.spark.deploy.history.ChromeUIHistoryServerSuite,org.apache.spark.sql.SQLQueryTestSuite test`

6. Maven / set `test.exclude.tags` to `org.apache.spark.tags.ExtendedSQLTest` (Both suites are expected to be skipped)
`build/mvn -Dtest.exclude.tags=org.apache.spark.tags.ExtendedSQLTest  -Dspark.test.webdriver.chrome.driver=/path/to/chromedriver -Dtest=none -DwildcardSuites=org.apache.spark.deploy.history.ChromeUIHistoryServerSuite,org.apache.spark.sql.SQLQueryTestSuite test`

Closes #28622 from sarutak/headless-browser-support-for-historyserver.

Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com>
Signed-off-by: Sean Owen <srowen@gmail.com>
@HyukjinKwon
Copy link
Member

Thanks @dongjoon-hyun for monitoring this.

@sarutak
Copy link
Member Author

sarutak commented Jun 3, 2020

Thanks @dongjoon-hyun , @HyukjinKwon .
I don't know why sbt passes the tests but I'll investigate the reason anyway.

@dongjoon-hyun
Copy link
Member

Thanks, @sarutak .

@sarutak sarutak reopened this Jun 3, 2020
@sarutak sarutak changed the title [SPARK-31765][WEBUI] Upgrade HtmlUnit >= 2.37.0 [SPARK-31765][WEBUI][test-maven] Upgrade HtmlUnit >= 2.37.0 Jun 3, 2020
@SparkQA
Copy link

SparkQA commented Jun 3, 2020

Test build #123462 has finished for PR 28585 at commit d950b42.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 3, 2020

Test build #123469 has started for PR 28585 at commit 955a147.

@sarutak
Copy link
Member Author

sarutak commented Jun 3, 2020

@dongjoon-hyun @HyukjinKwon The reason of the failure seems that bytebuddy which upgraded-selenium depends on is much older than mockito depends on.
I've excluded the older one.
Let's wait the result of CI.

@dongjoon-hyun
Copy link
Member

Sounds reasonable. Thanks!

@sarutak
Copy link
Member Author

sarutak commented Jun 3, 2020

retest this please.

@SparkQA
Copy link

SparkQA commented Jun 3, 2020

Test build #123501 has started for PR 28585 at commit 955a147.

@sarutak
Copy link
Member Author

sarutak commented Jun 4, 2020

retest this please.

@SparkQA
Copy link

SparkQA commented Jun 4, 2020

Test build #123518 has finished for PR 28585 at commit 955a147.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@sarutak
Copy link
Member Author

sarutak commented Jun 4, 2020

retest this please.

@SparkQA
Copy link

SparkQA commented Jun 4, 2020

Test build #123522 has finished for PR 28585 at commit 955a147.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@sarutak
Copy link
Member Author

sarutak commented Jun 4, 2020

By excluding old version of bytebuddy , Maven tests seem to pass.

@srowen
Copy link
Member

srowen commented Jun 6, 2020

Heh, do we think it's safe to merge again?
An exclusion is probably fine, but now it depends on something it needs only transitively. We could also manage the version explicitly. But, this maybe fine now.

@sarutak
Copy link
Member Author

sarutak commented Jun 7, 2020

The errors were reproduced in PR builder and they are fixed by excluding the old bytebuddy so I believe it's safe to merge.
What do you think, @dongjoon-hyun, @HyukjinKwon ?

@srowen
Copy link
Member

srowen commented Jun 11, 2020

I'll merge today if there are no objections

@srowen
Copy link
Member

srowen commented Jun 11, 2020

Trying again - merged to master

@dongjoon-hyun
Copy link
Member

According to @sarutak , this broke org.apache.spark.deploy.history.ChromeUIHistoryServerSuite due to Guava dependency.

@srowen
Copy link
Member

srowen commented Sep 23, 2020

Shoot. If this is non-essential, I think we can just revert it.
Or .. just lose this test? let me comment on the other PR.

@dongjoon-hyun
Copy link
Member

Thanks, @srowen ~

@sarutak sarutak deleted the upgrade-htmlunit branch June 4, 2021 20:43
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.

6 participants