Skip to content

[SPARK-35501][SQL][TESTS] Add a feature for removing pulled container image for docker integration tests #32652

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.

Sign up for GitHub

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

Closed
sarutak wants to merge 4 commits into from
Closed

[SPARK-35501][SQL][TESTS] Add a feature for removing pulled container image for docker integration tests #32652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7377ed4
Add pulled image removal feature.
sarutak May 24, 2021
60ea4fd
Add the property to pom.xml.
sarutak May 24, 2021
1594657
Merge branch 'master' of https://github.com/apache/spark into docker-…
sarutak May 26, 2021
da66bb1
Enabled the property by default.
sarutak May 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...tegration-tests/src/test/scala/org/apache/spark/sql/jdbc/DockerJDBCIntegrationSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ abstract class DockerJDBCIntegrationSuite extends SharedSparkSession with Eventu
val connectionTimeout = timeout(5.minutes)
val keepContainer =
sys.props.getOrElse("spark.test.docker.keepContainer", "false").toBoolean
val removePulledImage =
sys.props.getOrElse("spark.test.docker.removePulledImage", "true").toBoolean
Copy link
Member

@dongjoon-hyun dongjoon-hyun May 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If this causes image downloading every time, this is a regression for the non-GA users, isn't it?
I'd keep the default as false and enable this true only at GitHub Action environment if that is trying to resolve GA issue.

I investigated the reason and I found it's short of the storage capacity of the host on GA.

WDYT, @sarutak and @HyukjinKwon ?

Copy link
Member Author

@sarutak sarutak May 29, 2021

Choose a reason for hiding this comment

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

To avoid behavior change, it would be better to keep it false but keeping container images in the local repository is side effect and done implicitly before this change. So I thought it is better do keep the images only if users can explicitly set the property to false.
Any thoughts? @dongjoon-hyun @HyukjinKwon

Copy link
Member

@dongjoon-hyun dongjoon-hyun May 30, 2021

Choose a reason for hiding this comment

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

Got it. If you think so, never mind. Since it's already merged, let's leave this AS-IS, @sarutak .

Copy link
Member

@HyukjinKwon HyukjinKwon May 31, 2021

Choose a reason for hiding this comment

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

I am okay either way. Please feel free to change if you guys have a preference 👍


private var docker: DockerClient = _
// Configure networking (necessary for boot2docker / Docker Machine)
Expand All @@ -109,6 +111,7 @@ abstract class DockerJDBCIntegrationSuite extends SharedSparkSession with Eventu
port
}
private var containerId: String = _
private var pulled: Boolean = false
protected var jdbcUrl: String = _

override def beforeAll(): Unit = {
Expand All @@ -130,6 +133,7 @@ abstract class DockerJDBCIntegrationSuite extends SharedSparkSession with Eventu
case e: ImageNotFoundException =>
log.warn(s"Docker image ${db.imageName} not found; pulling image from registry")
docker.pull(db.imageName)
pulled = true
}
val hostConfigBuilder = HostConfig.builder()
.privileged(db.privileged)
Expand Down Expand Up @@ -215,6 +219,9 @@ abstract class DockerJDBCIntegrationSuite extends SharedSparkSession with Eventu
}
} finally {
docker.removeContainer(containerId)
if (removePulledImage && pulled) {
docker.removeImage(db.imageName)
}
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@
<spark.test.home>${session.executionRootDirectory}</spark.test.home>
<spark.test.webdriver.chrome.driver></spark.test.webdriver.chrome.driver>
<spark.test.docker.keepContainer>false</spark.test.docker.keepContainer>
<spark.test.docker.removePulledImage>true</spark.test.docker.removePulledImage>
Copy link
Member

@dongjoon-hyun dongjoon-hyun May 29, 2021

Choose a reason for hiding this comment

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

ditto.


<CodeCacheSize>1g</CodeCacheSize>
<!-- Needed for consistent times -->
Expand Down Expand Up @@ -2728,6 +2729,7 @@
<spark.unsafe.exceptionOnMemoryLeak>true</spark.unsafe.exceptionOnMemoryLeak>
<spark.test.webdriver.chrome.driver>${spark.test.webdriver.chrome.driver}</spark.test.webdriver.chrome.driver>
<spark.test.docker.keepContainer>${spark.test.docker.keepContainer}</spark.test.docker.keepContainer>
<spark.test.docker.removePulledImage>${spark.test.docker.removePulledImage}</spark.test.docker.removePulledImage>
<!-- Needed by sql/hive tests. -->
<test.src.tables>__not_used__</test.src.tables>
</systemProperties>
Expand Down