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

Move periodic timeout implementation to wsclient library. #3883

Merged
merged 45 commits into from
Sep 5, 2023
Merged

Move periodic timeout implementation to wsclient library. #3883

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
243d65b
wsclient: periodic timeout functionality addeed to wsclient
Jun 16, 2023
2ae064c
wsclient: edited comments
Jun 16, 2023
68b8a86
Fix static check failures
Jul 6, 2023
4520971
wsclient: new mock client and client_factory
Jul 8, 2023
fd249e9
wsclient: update tests using new mock wsclient and client_factory
Jul 9, 2023
c5695c1
wsclient: update the tcs handler in ecs-agent
Jul 10, 2023
ecdcded
Pass metricsfactory from handler
Jul 10, 2023
63401f8
Update vendor folder
Jul 10, 2023
61ceddb
Fix goimports
Jul 10, 2023
47a93e3
Move heartbeathandler back to acs,tcs
Jul 13, 2023
bfd7250
Updated mocks
Jul 13, 2023
034bb44
Updated unit tests.
Jul 13, 2023
a1f4839
Pass metricsFactory from acs session
Jul 13, 2023
2280263
Updated acs_handler tests to send new parameter to newsession function.
Jul 13, 2023
c29347a
Fix goimports
Jul 13, 2023
7c9ecb7
Add parameter to Do() in mock as Connect() is updated with parameter …
Jul 15, 2023
7deb4ad
Update test files with new parameters
Jul 15, 2023
924173f
Updated tests for wsclient, tcs, acs handlers
Jul 18, 2023
2ca9f02
Update metricsFactory.Done
Jul 18, 2023
2e6f077
Fixing goimports
Jul 18, 2023
0de2bb3
Fix acs handler test fail when passed timer as nil in mockwsclient
Jul 18, 2023
b5f5222
Fix acs client tests
Jul 19, 2023
31ed62a
Fix acs client tests
Jul 19, 2023
fadb396
Fix import order
Jul 19, 2023
f883a51
Add new tests
Jul 19, 2023
f5524b4
Fix import order
Jul 19, 2023
a15cc01
Fix go imports
Jul 19, 2023
8f91476
Fic acs handler tests
Jul 19, 2023
dee616b
Fic acs handler tests
Jul 19, 2023
2763c0c
Remove startTime from acs handler
Aug 18, 2023
36e6eca
Update acs tests
Aug 18, 2023
ff6de35
Fix static check failiures
Aug 18, 2023
7cdcb68
Fix unit test acs
Aug 18, 2023
60c9a5f
Update venfor directory
Aug 29, 2023
a304b44
Remove unnecessary metricsFactory
Aug 30, 2023
f1a60e8
Update error message from connection closure due to writeclosemessage
Aug 30, 2023
7e417d2
Update test messages
Aug 30, 2023
1d0cc8a
Update tests
Aug 31, 2023
ad67133
Return disconnect time in unit tests
Aug 31, 2023
d28b190
Handle review comments
Aug 31, 2023
69e60a9
Handle race condition in the TestPeriodicDisconnect
Aug 31, 2023
e9e4e3b
Minor changes to handle review comments
Sep 1, 2023
46f5aa5
Update vendor dir
Sep 1, 2023
4f57ed3
Goimports
Sep 1, 2023
90bf651
Fix test
Sep 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/wsclient/client.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions ecs-agent/tcs/handler/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,11 @@ import (
"testing"
"time"

"github.com/aws/amazon-ecs-agent/ecs-agent/metrics"

"context"

"github.com/aws/amazon-ecs-agent/ecs-agent/doctor"
"github.com/aws/amazon-ecs-agent/ecs-agent/eventstream"
"github.com/aws/amazon-ecs-agent/ecs-agent/metrics"
tcsclient "github.com/aws/amazon-ecs-agent/ecs-agent/tcs/client"
"github.com/aws/amazon-ecs-agent/ecs-agent/tcs/model/ecstcs"
"github.com/aws/amazon-ecs-agent/ecs-agent/wsclient"
Expand Down Expand Up @@ -441,9 +440,6 @@ func TestClientReconnectsAfterInactiveTimeout(t *testing.T) {
// it would continue to reconnect and test will be in forever loop.
assert.False(t, websocket.IsCloseError(err, websocket.CloseAbnormalClosure),
"Read from closed connection should produce an io.EOF error")

assert.Equal(t, err.Error(), context.DeadlineExceeded.Error(), "Context deadline exceeded error expected.")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit (non-blocking): Just curious, is there a specific reason why this assert statement was removed from this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the test was flaky. The return code differs based on when the ctx is cancelled. If the context is cancelled when we are in client.consumeMessages, it results in non-nil error. But when the context is cancelled in starttelemetrysession(), it results in a nil error. Another way to test would be to test for either of the condition to be true. Let me try that out and i can re-add both the conditions in a later PR.


}

func getPayloadFromRequest(request string) (string, error) {
Expand Down
14 changes: 11 additions & 3 deletions ecs-agent/wsclient/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ import (
"sync"
"time"

"github.com/aws/amazon-ecs-agent/ecs-agent/logger/field"

"github.com/aws/amazon-ecs-agent/ecs-agent/logger"
"github.com/aws/amazon-ecs-agent/ecs-agent/metrics"
"github.com/aws/amazon-ecs-agent/ecs-agent/utils"
Expand Down Expand Up @@ -287,7 +289,8 @@ func (cs *ClientServerImpl) Connect(disconnectMetricName string,
return nil, cs.NewError(possibleError)
}
}
logger.Warn(fmt.Sprintf("Error creating a websocket client: %v", err))
logger.Warn(fmt.Sprintf(
"Error creating a websocket client: %v", err))
return nil, errors.Wrapf(err, "websocket client: unable to dial %s response: %s",
parsedURL.Host, string(resp))
}
Expand Down Expand Up @@ -621,8 +624,13 @@ func (cs *ClientServerImpl) CloseClient(startTime time.Time, timeoutDuration tim
})
err := cs.WriteCloseMessage()
if err != nil {
danehlim marked this conversation as resolved.
Show resolved Hide resolved
logger.Warn(fmt.Sprintf("Error disconnecting client with url: %s, err: %s", cs.URL, err))
logger.Warn(("Error disconnecting client."), logger.Fields{
"URL": cs.URL,
field.Error: err,
})
}
logger.Info("Disconnected from server.")
logger.Info(("Disconnected from server."), logger.Fields{
"URL": cs.URL,
})
return err
}
5 changes: 2 additions & 3 deletions ecs-agent/wsclient/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,9 +474,8 @@ func TestPeriodicDisconnect(t *testing.T) {
messageError <- cs.ConsumeMessages(ctx)
endTime := time.Now()
timeDiff := endTime.Sub(startTime)
assert.True(t, timeDiff > disconnectTimeout,
"ConsumeMessages should be alive for more than disconnectTimeout.")
cs.Close()
assert.True(t, timeDiff >= disconnectTimeout,
"ConsumeMessages should be not be closed before disconnectTimeout has elapsed.")
}()

RichaGangwar marked this conversation as resolved.
Show resolved Hide resolved
// Assert that the connection is closed on the server side as expected
Expand Down