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.

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

Jump to bottom

deploy: check the dc conditions instead of relying on deployer logs #14395

Merged
merged 1 commit into from
May 30, 2017
Merged

deploy: check the dc conditions instead of relying on deployer logs #14395

merged 1 commit into from
May 30, 2017

Conversation

mfojtik
Copy link
Contributor

@mfojtik mfojtik commented May 29, 2017

Unblock: #13980

Basically check that the new RC is available (so the rollout succeded) instead of checking the deployer log messages.

This does not solve the issue where the last log line dissapears, but it is band-aid to unblock the queue.

@mfojtik mfojtik requested a review from smarterclayton May 29, 2017 12:33
@mfojtik
Copy link
Contributor Author

mfojtik commented May 29, 2017

lets [test] this couple times to ensure the flake is gone.

mfojtik
mfojtik commented May 29, 2017
View reviewed changes
test/extended/deployments/deployments.go Outdated
o.Expect(err).NotTo(o.HaveOccurred())
} else {
cond := deployutil.GetDeploymentCondition(dc.Status, deployapi.DeploymentProgressing)
o.Expect(cond.Reason).To(o.BeEquivalentTo(deployapi.NewRcAvailableReason))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kargakis is this enough to keep this test sane without relying on the deployer log? note this is an attempt to unblock the queue not to fix the log problem.

@mfojtik mfojtik mentioned this pull request May 29, 2017
Closed
tnozicka
tnozicka approved these changes May 29, 2017
View reviewed changes
Copy link
Contributor

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

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

lgtm

@mfojtik
Copy link
Contributor Author

mfojtik commented May 29, 2017

[test] flake: #14396

@mfojtik
Copy link
Contributor Author

mfojtik commented May 29, 2017 via email

@mfojtik mfojtik force-pushed the extended-use-conditions branch from 7c16d23 to 8bb6996 Compare May 29, 2017 19:42
@mfojtik
Copy link
Contributor Author

mfojtik commented May 29, 2017

for the record, this just flaked in the other location where we test for --> Success (same test, outside the loop...):

/go/src/github.com/openshift/origin/test/extended/deployments/deployments.go:319
Expected
    <string>: --> pre: Running hook pod ...
    test pre hook executed
    --> pre: Success
    --> Scaling deployment-test-1 to 2
    --> Waiting up to 10m0s for pods in rc deployment-test-1 to become ready
to contain substring
    <string>: --> Success
/go/src/github.com/openshift/origin/test/extended/deployments/deployments.go:257

@smarterclayton @ncdc @Kargakis i believe there is problem with oc logs -f where when the pod is deleted it might loose the last log line from the output. Can't explain this otherwise.

@smarterclayton
Copy link
Contributor

smarterclayton commented May 29, 2017 via email

@mfojtik
Copy link
Contributor Author

mfojtik commented May 30, 2017

@smarterclayton for some reason it got much worse in last days hitting the deployement tests as they rely on the deployer logs...

@mfojtik mfojtik force-pushed the extended-use-conditions branch from 8bb6996 to 509b70c Compare May 30, 2017 07:32
@mfojtik
Copy link
Contributor Author

mfojtik commented May 30, 2017

@Kargakis @tnozicka @smarterclayton in that case i don't think we even need to check the logs for --> Success as we already check that the rollout completed by:

o.Expect(waitForLatestCondition(oc, "deployment-test", deploymentRunTimeout, deploymentReachedCompletion)).NotTo(o.HaveOccurred())

So removing the --> Success checks should unblock the queue and we can put that back once the journald rate limiter is fixed. Thoughts?

@mfojtik mfojtik force-pushed the extended-use-conditions branch 2 times, most recently from c9a2310 to eec5989 Compare May 30, 2017 07:37
@mfojtik
Copy link
Contributor Author

mfojtik commented May 30, 2017

[merge][severity:blocker]

Fixes the P0 flake and ublocks the queue.

tnozicka
tnozicka reviewed May 30, 2017
View reviewed changes
test/extended/deployments/deployments.go Outdated
@@ -496,7 +502,7 @@ var _ = g.Describe("deploymentconfigs", func() {
o.Expect(out).To(o.ContainSubstring("--> Reached 50%"))
o.Expect(out).To(o.ContainSubstring("Halfway"))
o.Expect(out).To(o.ContainSubstring("Finished"))
o.Expect(out).To(o.ContainSubstring("--> Success"))
// o.Expect(out).To(o.ContainSubstring("--> Success"))
Copy link
Contributor

Choose a reason for hiding this comment

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

FIXME?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, lazy to copy the comment ;-)

@tnozicka
Copy link
Contributor

@mfojtik yeah, that seems to be already covered so just commenting it out sgtm until jurnald gets fixed.

If anyone has the link to the jurnald bug it would be nice to have it linked here or in a followup issue.

@mfojtik mfojtik force-pushed the extended-use-conditions branch from eec5989 to 5d3d45c Compare May 30, 2017 09:12
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 5d3d45c

@mfojtik
Copy link
Contributor Author

mfojtik commented May 30, 2017

[merge][severity:blocker]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 5d3d45c

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1836/) (Base Commit: 5a19120)

@openshift-bot
Copy link
Contributor

openshift-bot commented May 30, 2017 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/842/) (Base Commit: 1c38059) (Extended Tests: blocker) (Image: devenv-rhel7_6283)

@openshift-bot openshift-bot merged commit d861384 into openshift:master May 30, 2017
@mfojtik mfojtik deleted the extended-use-conditions branch September 5, 2018 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnozicka tnozicka tnozicka approved these changes

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mfojtik @smarterclayton @tnozicka @openshift-bot