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

Adding Integ Tests for Granular Stop Timeout #1887

Merged
merged 3 commits into from
Feb 27, 2019
Merged

Adding Integ Tests for Granular Stop Timeout #1887

merged 3 commits into from
Feb 27, 2019

Conversation

ubhattacharjya
Copy link
Contributor

@ubhattacharjya ubhattacharjya commented Feb 26, 2019 •
edited
Loading

TraceLog Events

2019-02-26T01:22:44Z [INFO] Managed task [testDependencyHealthyTimeout]: sent container change event [dependency]: testDependencyHealthyTimeout dependency -> STOPPED, Exit 0, , Known Sent: NONE
2019-02-26T01:22:44Z [INFO] Task engine [testDependencyHealthyTimeout]: stopping container [parent1]
2019-02-26T01:22:44Z [INFO] Task engine [testDependencyHealthyTimeout]: stopping container [parent2]
2019-02-26T01:22:49Z [WARN] Managed task [testDependencyHealthyTimeout]: failed to write container [parent1] change event to tcs event stream: Event stream is closed
2019-02-26T01:22:49Z [INFO] Managed task [testDependencyHealthyTimeout]: sending container change event [parent1]: testDependencyHealthyTimeout parent1 -> STOPPED, Exit 137, , Known Sent: NONE
2019-02-26T01:22:49Z [INFO] Managed task [testDependencyHealthyTimeout]: sent container change event [parent1]: testDependencyHealthyTimeout parent1 -> STOPPED, Exit 137, , Known Sent: NONE
2019-02-26T01:22:49Z [INFO] Managed task [testDependencyHealthyTimeout]: redundant container state change. parent1 to STOPPED, but already STOPPED
2019-02-26T01:22:49Z [INFO] Managed task [testDependencyHealthyTimeout]: redundant container state change. parent1 to STOPPED, but already STOPPED
2019-02-26T01:23:15Z [WARN] Managed task [testDependencyHealthyTimeout]: failed to write container [parent2] change event to tcs event stream: Event stream is closed
2019-02-26T01:23:15Z [INFO] Managed task [testDependencyHealthyTimeout]: sending container change event [parent2]: testDependencyHealthyTimeout parent2 -> STOPPED, Exit 0, , Known Sent: NONE
2019-02-26T01:23:15Z [INFO] Managed task [testDependencyHealthyTimeout]: sent container change event [parent2]: testDependencyHealthyTimeout parent2 -> STOPPED, Exit 0, , Known Sent: NONE
2019-02-26T01:23:15Z [INFO] Managed task [testDependencyHealthyTimeout]: sending task change event [testDependencyHealthyTimeout -> STOPPED, Known Sent: NONE, PullStartedAt: 2019-02-26 01:22:13.449581324 +0000 UTC m=+0.041705235, PullStoppedAt: 2019-02-26 01:22:14.147765508 +0000 UTC m=+0.739889420, ExecutionStoppedAt: 2019-02-26 01:22:44.374414138 +0000 UTC m=+30.966538059]
--- PASS: TestGranularStopTimeout (61.84s)
	ordering_integ_test.go:411: Finished successfully.
2019-02-26T01:23:15Z [INFO] Managed task [testDependencyHealthyTimeout]: sent task change event [testDependencyHealthyTimeout -> STOPPED, Known Sent: NONE, PullStartedAt: 2019-02-26 01:22:13.449581324 +0000 UTC m=+0.041705235, PullStoppedAt: 2019-02-26 01:22:14.147765508 +0000 UTC m=+0.739889420, ExecutionStoppedAt: 2019-02-26 01:22:44.374414138 +0000 UTC m=+30.966538059]
PASS

Summary

Implementation details

Testing

New tests cover the changes:

Description for the changelog

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ubhattacharjya ubhattacharjya requested review from petderek and a team February 26, 2019 01:38
petderek
petderek reviewed Feb 26, 2019
View reviewed changes
agent/engine/ordering_integ_test.go Outdated

stateChangeEvents := taskEngine.StateChangeEvents()

taskArn := "testDependencyHealthyTimeout"
Copy link
Contributor

Choose a reason for hiding this comment

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

change name

agent/engine/ordering_integ_test.go Outdated
taskArn := "testDependencyHealthyTimeout"
testTask := createTestTask(taskArn)

dependency := createTestContainerWithImageAndName(baseImageForOS, "dependency")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd do this test the other way around -- 1 parent, 2 dependencies. You can get rid of the sleep (and do exit 0 instead) since this will guarantee that the other containers have started.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense.

agent/engine/ordering_integ_test.go Outdated

verifyContainerStoppedStateChange(t, taskEngine)
verifyContainerStoppedStateChange(t, taskEngine)
verifyContainerStoppedStateChange(t, taskEngine)
Copy link
Contributor

Choose a reason for hiding this comment

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

This test will succeed regardless of if the timeouts are applied or not. We need to validate the exit codes of the containers.

@ubhattacharjya ubhattacharjya force-pushed the StopTimeoutTest branch 2 times, most recently from a0a5437 to 399f151 Compare February 26, 2019 03:23
@ubhattacharjya ubhattacharjya requested review from petderek and a team February 26, 2019 03:26
petderek
petderek approved these changes Feb 26, 2019
View reviewed changes
agent/engine/ordering_integ_test.go Outdated
}

dependency1.EntryPoint = &entryPointForOS
dependency1.Command = []string{"trap 'echo caught' SIGTERM; while true; do echo hello; sleep 1s; done;"}
Copy link
Contributor

Choose a reason for hiding this comment

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

can this Command be the same as the other one? Just so we can be confident that the timeout is the only changing parameter?

This doesn't change the outcome of the test, it just more explicitly demonstrates what we have built

petderek
petderek approved these changes Feb 27, 2019
View reviewed changes
Copy link
Contributor

@petderek petderek left a comment

Choose a reason for hiding this comment

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

lgtm, but the test won't work on windows. move it to the linux-specific file.

agent/engine/ordering_integ_test.go Outdated
@@ -339,6 +340,74 @@ func TestDependencyHealthyTimeout(t *testing.T) {
waitFinished(t, finished, orderingTimeout)
}


func TestGranularStopTimeout(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This uses functions that don't exist in powershell. I'd move this to the _integ_linux_test.go file

@ubhattacharjya ubhattacharjya added this to the 1.26.0 milestone Feb 27, 2019
@ubhattacharjya ubhattacharjya force-pushed the StopTimeoutTest branch 3 times, most recently from 74911e6 to 464ff08 Compare February 27, 2019 18:46
@petderek
integ tests: allocate more cpu per container
c3b8514 
The default helper function will now allocate 1024 cpu shares or 100%
cpu-percent on windows. This will enable the windows based tests to
finish in more predictable ways. When Windows tests were constrained,
simple tasks liek "sleep 10" were taking much longer than the expected
10 seconds.
@ubhattacharjya
Copy link
Contributor Author

#1869 - The windows functional test issues

@petderek
Copy link
Contributor

We've validated the linux tests manually.

I believe we are getting errors in the windows functional tests because we set the pull behavior to 'prefer cached'. The error message we are receiving is that PullImageStart time is not set.

@petderek
Copy link
Contributor

We've validated the linux tests manually.

I believe we are getting errors in the windows functional tests because we set the pull behavior to 'prefer cached'. The error message we are receiving is that PullImageStart time is not set.

I'll validate with haikuo if this is the expected behavior. If not, we will need to fix this bug.

@ubhattacharjya ubhattacharjya merged commit 95de818 into aws:container-ordering-feature Feb 27, 2019
@ubhattacharjya ubhattacharjya deleted the StopTimeoutTest branch April 8, 2019 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yhlee-aws yhlee-aws yhlee-aws approved these changes

@petderek petderek petderek approved these changes

Assignees
No one assigned
Labels
bot/test
Projects
None yet
Milestone
1.26.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ubhattacharjya @petderek @yhlee-aws