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

Fixed jenkins verifyservice e2e tests #23581

Conversation

waveywaves
Copy link
Contributor

Fixed as containers were going to crashloopbackoff.
The image loaded before was an s2i image without the source.
In this iteration we are using the redis-epemeral template with reduced memory which also provides us with the basic service to test and in the template jenkins-verifyservice-pipeline we are creating the headless service and the buildconfig with the jenkins file to test the feature.

cc @akram @gabemontero

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 9, 2019
@akram
Copy link
Contributor

akram commented Aug 9, 2019

/assign @akram
/assign @gabemontero

@waveywaves
Copy link
Contributor Author

/assign @adambkaplan

Copy link
Contributor

@akram akram left a comment

Choose a reason for hiding this comment

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

@waveywaves , can you make the required changes?

test/extended/builds/pipeline_origin_bld.go Outdated Show resolved Hide resolved
test/extended/builds/pipeline_origin_bld.go Outdated Show resolved Hide resolved
test/extended/builds/pipeline_origin_bld.go Outdated Show resolved Hide resolved
test/extended/builds/pipeline_origin_bld.go Outdated Show resolved Hide resolved
@waveywaves waveywaves force-pushed the e2etests/jenkins-client-plugin/verifyservice branch from b4220c2 to 1c845e9 Compare August 9, 2019 14:17
o.Expect(err).NotTo(o.HaveOccurred())
err = oc.AsAdmin().Run("delete").Args("all", "-l", "app=nodejs-example").Execute()
err = oc.AsAdmin().Run("delete").Args("all", "-l", fmt.Sprintf("app=%v", redisTemplate)).Execute()
Copy link
Contributor

Choose a reason for hiding this comment

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

this one should be redisAppName not redisTemplate.

- apiVersion: v1
kind: BuildConfig
metadata:
name: jenkins-verifyservice-pipeline
app: redis-ephemeral
Copy link
Contributor

Choose a reason for hiding this comment

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

All the other components will be create with app=redis and not app=redis-ephemeral

@gabemontero
Copy link
Contributor

/approve

@gabemontero
Copy link
Contributor

yeah adding the test data will require @adambkaplan (perhaps another candidate for OWNERS update)

@gabemontero
Copy link
Contributor

@waveywaves there is an error with your changes in the latest e2e-aws-builds run ... I'll refrain from highlighting it for now ... see if you can sort it out.

@akram
Copy link
Contributor

akram commented Aug 9, 2019

/hold

still other issues that needs to be fixed, I pinged @waveywaves

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 9, 2019
o.Expect(err).NotTo(o.HaveOccurred())
redisTemplate := "redis-ephemeral"
redisAppName := "redis"
newAppRedisEphemeralArgs := []string{"--template", fmt.Sprintf("--name=%v", redisAppName), redisTemplate, "-p", "MEMORY_LIMIT=128Mi"}
Copy link
Contributor

Choose a reason for hiding this comment

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

The syntax is not correct, it should be:

newAppRedisEphemeralArgs := []string{ fmt.Sprintf("--template=%v", redisTemplate),  fmt.Sprintf("--name=%v", redisAppName), "-p", "MEMORY_LIMIT=128Mi"}

or simplier:

newAppRedisEphemeralArgs := []string{ redisTemplate, "--name", redisAppName), "-p", "MEMORY_LIMIT=128Mi"}

@waveywaves waveywaves force-pushed the e2etests/jenkins-client-plugin/verifyservice branch from 1c845e9 to 8b5986d Compare August 9, 2019 20:43
@akram
Copy link
Contributor

akram commented Aug 9, 2019

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 9, 2019
@waveywaves
Copy link
Contributor Author

/test e2e-cmd
/test e2e-aws-serial
/test e2e-aws-upgrade

@akram
Copy link
Contributor

akram commented Aug 10, 2019

/test e2e-aws-serial

@akram
Copy link
Contributor

akram commented Aug 10, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2019
@akram
Copy link
Contributor

akram commented Aug 10, 2019

/approve

@akram
Copy link
Contributor

akram commented Aug 10, 2019

/assign @adambkaplan

@akram
Copy link
Contributor

akram commented Aug 10, 2019

/assign @bparees

As of @adambkaplan is on PTO, can you approve this one ?
I think it was breaking origin e2e tests, so the sooner the
Thanks

@bparees
Copy link
Contributor

bparees commented Aug 10, 2019

/approve

but i would like to understand this:

Fixed as containers were going to crashloopbackoff.
The image loaded before was an s2i image without the source.

why was the original nodejs image crashlooping? was the nodejs image changed such that it can no longer be run without source code being added?

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akram, bparees, gabemontero, waveywaves

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2019
@openshift-merge-robot openshift-merge-robot merged commit 0e3af06 into openshift:master Aug 10, 2019
@waveywaves
Copy link
Contributor Author

waveywaves commented Aug 11, 2019

/approve

but i would like to understand this:

Fixed as containers were going to crashloopbackoff.
The image loaded before was an s2i image without the source.

why was the original nodejs image crashlooping? was the nodejs image changed such that it can no longer be run without source code being added?

It is an S2I image with no source code and nothing defined as the entrypoint. To make the container run with the image, commands need ot be given in the Deployment. Hence we had to fall bakc to using a predefined template.

@bparees
Copy link
Contributor

bparees commented Aug 11, 2019

It is an S2I image with no source code and nothing defined as the entrypoint. To make the container run with the image, commands need ot be given in the Deployment. Hence we had to fall bakc to using a predefined template.

yes but presumably this test was working at some point, so why did it start failing?

@waveywaves
Copy link
Contributor Author

waveywaves commented Aug 11, 2019

yes but presumably this test was working at some point, so why did it start failing?

After testing out the deployment config seperately, it did fail with a crashloopback off. When the changes were pushed to github, the tests which ran did not fail (keep in mind these are not the new ones) and seems like changes in these tests weren't picked up by the CI unlike travisCI where the changes in the PR are applied to the test env.

After the last PR with the changes in origin/test/extended/... was merged, only after that the new tests started to run ad we were able to confirm that there is something wrong with the tests themselves after which we fixed it.

@bparees
Copy link
Contributor

bparees commented Aug 11, 2019

hm. i'm still a little puzzled but it sounds like this test basically wasn't being run normally so we didn't notice it was broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants