Issue/jperf 445 sudo dind #5

dagguh · 2019-04-02T15:07:44Z

No description provided.

Augment the vast `docker-java` lib rather than hiding it under our layers. Drop `testcontainers` in favor of the underlying `docker-java` API. Use `com.atlassian.performance.tools:ssh` for more interoperability. This new API is already tested in `virtual-users` (merged for weeks) and locally in `infrastructure` (tested tens of times today).

.editorconfig

wyrzyk · 2019-04-03T11:53:42Z

build.gradle.kts

@@ -29,14 +27,14 @@ configurations.all {
 }

 dependencies {
+    api("com.atlassian.performance.tools:ssh:[2.0.0,3.0.0)")


Do we have to add an ssh client to the ssh-ubuntu lib?

Pros:

the consumer can immediately interact with the Ubuntu via SSH, no need to translate APIs

the consumer doesn't have to search for and decide on an SSH client library

❗️ we can preinstall sudo as a part of API

Cons:

bigger dependency tree - more conflicts to resolve (unless we start using ranges)

doesn't let users choose their own SSH client lib

We could avoid the cons and keep pros by introducing another module.

wyrzyk · 2019-04-03T11:55:19Z

src/main/kotlin/com/atlassian/performance/tools/sshubuntu/api/SshUbuntuContainer.kt

+ * @param [ports] Binds container ports to host address space.
+ * @param [peerIp] Addresses the Ubuntu within the Docker network.
+ */
+class SshUbuntuContainer internal constructor(


Internal constructor means nothing to the java consumer of the library.

True. Hmm, so we make it public and wait and see if we need to add fields later?

We were experimenting with private constructor + factory method. The function will be visible from java, but the user will be warned.

We can also make it public, I'm not insisting.

wyrzyk · 2019-04-03T11:56:55Z

src/main/kotlin/com/atlassian/performance/tools/sshubuntu/api/SshUbuntuImage.kt

@@ -0,0 +1,7 @@
+package com.atlassian.performance.tools.sshubuntu.api
+
+interface SshUbuntuImage {


Is it an API or SPI? Can I use own implementations as a user of the library?

Initially there was no interface at all. I added it to mimic the original ssh-ubuntu design.
Perhaps it's unnecessary. TBH I'm not sure about the value here.

wyrzyk · 2019-04-03T11:57:50Z

src/main/kotlin/com/atlassian/performance/tools/sshubuntu/api/SudoSshUbuntuImage.kt

+            .exec(PullImageResultCallback())
+            .awaitCompletion()
+        return docker
+            .createContainerCmd("rastasheep/ubuntu-sshd:16.04")


What license does the image use?

Default one, good catch
rastasheep/ubuntu-sshd#25

Wait a minute...
https://github.com/atlassian/ssh-ubuntu/blob/release-0.1.0/docker/Dockerfile
Currently we're using the same base

wyrzyk · 2019-04-03T12:05:53Z

src/test/kotlin/com/atlassian/performance/tools/sshubuntu/api/SudoSshUbuntuImageIT.kt

+    fun shouldStartUbuntu() {
+        val docker = DockerClientBuilder.getInstance().build()
+        val osName = docker
+            .createNetworkCmd()


Is the low-level knowledge about docker networks needed to use the library?

In this form it is. Adding network optionality complicated the implementation.

Pros:

simpler, non-branching implementation

exposes the consumer to the real complexity of networks: internal/external addresses/ports - necessary context when dealing with browsers connecting to servers (VU tests) or servers polling themselves locally (infra upgrade REST polling)

Cons:

it's extra complexity for the users who do not care about connecting multiple containers

We definitely have to leave the option for networks, but we might invest into making the usage easier for simpler use cases. I'm not sure if it's worth it. I used this code in multiple tests already and the network creation was easily hidden by a local reused method. Perhaps we should wait and see if it's actually a problem.

src/main/kotlin/com/atlassian/performance/tools/sshubuntu/api/docker/ConnectedContainer.kt

dagguh · 2019-04-02T15:17:29Z

build.gradle.kts

    implementation("org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlinVersion")
-    implementation("org.testcontainers:testcontainers:1.10.5")


I can try to shrink the scope if it's possible to implement DIND and sudo via testcontainers. I could leave out the autocloseables as well and stick to the existing coarse-grained cleanup mechanisms (leaving leftovers if there's an exception somewhere in the middle of ssh-ubuntu.

The upshot is: this is already tested in VUs and has less external dependencies (like Docker publishing).
Locally I rewrote infra for this (basically a find and replace) and it's all green and still fast.

dagguh · 2019-04-04T08:56:18Z

build.gradle.kts

@@ -29,14 +27,14 @@ configurations.all {
 }

 dependencies {
+    api("com.atlassian.performance.tools:ssh:[2.0.0,3.0.0)")


Pros:

the consumer can immediately interact with the Ubuntu via SSH, no need to translate APIs

the consumer doesn't have to search for and decide on an SSH client library

❗️ we can preinstall sudo as a part of API

Cons:

bigger dependency tree - more conflicts to resolve (unless we start using ranges)

doesn't let users choose their own SSH client lib

dagguh · 2019-04-04T13:16:38Z

Perhaps this PR is overkill for the purposes of sudo and Docker in Docker. I opened this, because I already tested it in other places, didn't want my source to go to waste and didn't want to think too hard about it, because I was in a rush. But that reasoning is suboptimal for the community.

Here's an alternative, which is not a shotgun surgery: #6
It changes fewer things and yields a more powerful API.

wyrzyk

The PR changes API significantly and adds more requirements that we had before. If you'd like to push the change forward, please take care to upgrade it in modules that already depend on it. For ssh module the new API will make the lib unusable.

dagguh · 2019-04-05T08:47:09Z

This PR is closed, because #6 has won the competition and got merged

Use IDE to help with trailing blank lines

c689dab

dagguh requested a review from wyrzyk April 2, 2019 15:07

Remove the unused Bitbucket Pipelines config

8ddc9fd

dagguh force-pushed the issue/JPERF-445-sudo-dind branch from d3b4a6b to e7bfbd8 Compare April 2, 2019 15:25

dagguh added 2 commits April 2, 2019 18:50

JPERF-444: Avoid degradation-prone central Ubuntu servers

146079f

dagguh force-pushed the issue/JPERF-445-sudo-dind branch from e7bfbd8 to 146079f Compare April 2, 2019 16:50

dagguh mentioned this pull request Apr 2, 2019

Issue/jperf 273 transplant ssh jira nodes atlassian/infrastructure#7

Draft

sebapawlak approved these changes Apr 3, 2019

View reviewed changes

.editorconfig Show resolved Hide resolved

wyrzyk reviewed Apr 3, 2019

View reviewed changes

dagguh commented Apr 4, 2019

View reviewed changes

dagguh mentioned this pull request Apr 4, 2019

Issue/jperf 445 customize container #6

Merged

wyrzyk reviewed Apr 5, 2019

View reviewed changes

dagguh closed this Apr 5, 2019

dagguh deleted the issue/JPERF-445-sudo-dind branch April 5, 2019 08:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/jperf 445 sudo dind #5

Issue/jperf 445 sudo dind #5

dagguh commented Apr 2, 2019

wyrzyk Apr 3, 2019

dagguh Apr 4, 2019 •

edited

Loading

wyrzyk Apr 5, 2019

wyrzyk Apr 3, 2019

dagguh Apr 4, 2019

wyrzyk Apr 5, 2019 •

edited

Loading

wyrzyk Apr 3, 2019

dagguh Apr 4, 2019

wyrzyk Apr 3, 2019

dagguh Apr 4, 2019

dagguh Apr 4, 2019

wyrzyk Apr 3, 2019

dagguh Apr 4, 2019

dagguh Apr 4, 2019

dagguh Apr 2, 2019

dagguh Apr 2, 2019

dagguh Apr 4, 2019 •

edited

Loading

dagguh commented Apr 4, 2019 •

edited

Loading

wyrzyk left a comment

dagguh commented Apr 5, 2019

		@@ -0,0 +1,7 @@
		package com.atlassian.performance.tools.sshubuntu.api

		interface SshUbuntuImage {

		implementation("org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlinVersion")
		implementation("org.testcontainers:testcontainers:1.10.5")

Issue/jperf 445 sudo dind #5

Issue/jperf 445 sudo dind #5

Conversation

dagguh commented Apr 2, 2019

Choose a reason for hiding this comment

dagguh Apr 4, 2019 • edited Loading

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

wyrzyk Apr 5, 2019 • edited Loading

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

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

Choose a reason for hiding this comment

dagguh Apr 4, 2019 • edited Loading

Choose a reason for hiding this comment

dagguh commented Apr 4, 2019 • edited Loading

wyrzyk left a comment

Choose a reason for hiding this comment

dagguh commented Apr 5, 2019

dagguh Apr 4, 2019 •

edited

Loading

wyrzyk Apr 5, 2019 •

edited

Loading

dagguh Apr 4, 2019 •

edited

Loading

dagguh commented Apr 4, 2019 •

edited

Loading