Skip to content

[SPARK-24551][K8S] Add integration tests for secrets - #21652

Closed
skonto wants to merge 1 commit into
apache:masterfrom
skonto:add-secret-its
Closed

[SPARK-24551][K8S] Add integration tests for secrets#21652
skonto wants to merge 1 commit into
apache:masterfrom
skonto:add-secret-its

Conversation

@skonto

@skontoskonto commented Jun 28, 2018

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

  • Adds integration tests for env and mount secrets.

How was this patch tested?

Manually by checking that secrets were added to the containers and by tuning the tests.

image

@skonto
skontoforce-pushed the add-secret-its branch 2 times, most recently from f0d59cc to 7602dbcCompareJune 28, 2018 03:59
@skonto

skonto commented Jun 28, 2018

Copy link
Copy Markdown
ContributorAuthor

@foxish@liyinan926@erikerlandson pls review.

@skontoskonto changed the title [SPARK-24551][K8s] Add integration tests for secrets[SPARK-24551][K8S] Add integration tests for secretsJun 28, 2018
@SparkQA

Copy link
Copy Markdown

@SparkQA

Copy link
Copy Markdown

@SparkQA

Copy link
Copy Markdown

Test build #92408 has finished for PR 21652 at commit f0d59cc.

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

@SparkQA

Copy link
Copy Markdown

@SparkQA

Copy link
Copy Markdown

Test build #92409 has finished for PR 21652 at commit 7602dbc.

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

@SparkQA

Copy link
Copy Markdown

@skonto

skonto commented Jun 28, 2018

Copy link
Copy Markdown
ContributorAuthor

@foxish@erikerlandson I got this failure: "namespaces "59da2249348347a39391b4389ab418ab" not found."
I don't think it relates to the PR, I haven't seen anything locally. Secret is created in the namespace defined in the tests, I don't touch it.

@skonto

skonto commented Jun 29, 2018

Copy link
Copy Markdown
ContributorAuthor

@felixcheung@liyinan926@ssuchter review pls.

@foxish

Copy link
Copy Markdown
Contributor

jenkins, retest this please

@foxishfoxish left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like a great start. I like that we're now getting the output from pod exec and validating it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should add these tests to a separate class? This file is growing in size and we can separate them out a bit for better code understanding.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

You are right probably we should. Ok let me try do this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: indentation

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

will fix.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a pretty quick timeout. Is there some way to have a longer timeout but to return instantly if the watch command exits? E.g. some kind of "join" method call on the watch instance that itself has a timeout?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ok will have a look. It does work because I guess its minikube and it is fast, I didn't see any issues. But might be a problem don't know.

@skontoskontoJul 4, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@ssuchter I tried the approach here: https://github.com/fabric8io/kubernetes-client/blob/master/kubernetes-examples/src/main/java/io/fabric8/kubernetes/examples/ExecLoopExample.java
and also mentioned here:
https://github.com/fabric8io/kubernetes-client/blob/master/kubernetes-examples/src/main/java/io/fabric8/kubernetes/examples/ExecPipesExample.java#L60
Latches were not useful to me. The examples there use the timeout approach (actually I recall that I looked at the examples before writing the code here).
The ideal approach would be to check continuously on the watch output on a future (eg. look what this class does: https://github.com/fabric8io/kubernetes-client/blob/9b2128ca5f2362991c99640d8a0325d60741644c/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/InputStreamPumper.java) and wait until it completes with some time out. But that didnt work well so far for me.
So I guess that sleep is the most easy solution.

@ssuchterssuchter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This overall looks pretty nice, thanks! I had just a couple questions - please take a look.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be done before every test, or just the one that is using the secrets?

@skontoskontoJul 4, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I guess we can add it before the specific test. My rationale was that this is something I require before the tests are run.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same question about every test versus just one of them.

@SparkQA

Copy link
Copy Markdown

@SparkQA

Copy link
Copy Markdown

@SparkQA

Copy link
Copy Markdown

Test build #92538 has finished for PR 21652 at commit 7602dbc.

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

@skontoskontoJul 4, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@ssuchter moved the secret creation in here. I had the idea of using before and after and checking for a flag to identify the current test (scalatest with FunSuite does not give you that AFAIK) but I think this is cleaner.

@SparkQA

Copy link
Copy Markdown

@SparkQA

Copy link
Copy Markdown

Test build #92610 has finished for PR 21652 at commit 2bda40c.

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

@SparkQA

Copy link
Copy Markdown

@skonto
skontoforce-pushed the add-secret-its branch 2 times, most recently from 9bd17b3 to 9de4a7aCompareJuly 4, 2018 15:26
@skonto

skonto commented Jul 4, 2018

Copy link
Copy Markdown
ContributorAuthor

@foxish@ssuchter I refactored the tests in different classes. There are some clear benefits now.
KubernetesSuite should only provide the env setup now. If we want real separation in the future we can just create real Suites, now the tests are included as traits.
Pls check my comment above about the timeout issue.

@SparkQA

Copy link
Copy Markdown

@shaneknapp

Copy link
Copy Markdown
Contributor

test this please

@SparkQA

Copy link
Copy Markdown

@SparkQA

Copy link
Copy Markdown

@SparkQA

Copy link
Copy Markdown

Test build #93204 has finished for PR 21652 at commit f843638.

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

@shaneknapp

Copy link
Copy Markdown
Contributor

test this please (i tied this build to amp-jenkins-staging-worker-02 to unblock things)

@SparkQA

Copy link
Copy Markdown

@SparkQA

Copy link
Copy Markdown

@shaneknapp

Copy link
Copy Markdown
Contributor

argh fixing 02 again and will retrigger then once it's done.

i'm really sorry that things have been so flaky recently.

@SparkQA

Copy link
Copy Markdown

Test build #93205 has finished for PR 21652 at commit f843638.

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

@shaneknapp

Copy link
Copy Markdown
Contributor

test this please

@SparkQA

Copy link
Copy Markdown

@SparkQA

Copy link
Copy Markdown

Test build #93206 has finished for PR 21652 at commit f843638.

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

@SparkQA

Copy link
Copy Markdown

@skonto

Copy link
Copy Markdown
ContributorAuthor

@shaneknapp thanks, @srowen@foxish gentle ping.

@skontoskontoJul 18, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I removed the package objects and replaced them with normal ones. The package statement was not ok and their naming convention was not following scala rules.

@SparkQA

Copy link
Copy Markdown

@SparkQA

Copy link
Copy Markdown

Test build #93228 has finished for PR 21652 at commit 5265129.

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

@SparkQA

Copy link
Copy Markdown

@skonto

skonto commented Jul 19, 2018

Copy link
Copy Markdown
ContributorAuthor

@foxish@srowen I rebased again could you review and merge pls. It is ready now.

@SparkQA

Copy link
Copy Markdown

@SparkQA

Copy link
Copy Markdown

NOCACHEARG=
BUILD_PARAMS=
while getopts f:mr:t:n:b: option
while getopts f:p:mr:t:n:b: option

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@debasishg spotted this.

@SparkQA

Copy link
Copy Markdown

Test build #93298 has finished for PR 21652 at commit 67df340.

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

@skonto

skonto commented Jul 20, 2018

Copy link
Copy Markdown
ContributorAuthor

@felixcheung@srowen may I have a merge pls?

@srowen

Copy link
Copy Markdown
Member

Merged to master

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@skonto@SparkQA@foxish@liyinan926@shaneknapp@ifilonenko@srowen@ssuchter