windows: changed method for retrieving Windows network statistics in case of awsvpc network mode #3425

rawahars · 2022-10-10T18:28:55Z

Summary

Previously, we were running a Powershell method (Get-NetAdapterStatistics) to retrieve the network statistics when the task is using awsvpc network mode. However, this can cause CPU spikes on smaller instance types.

Another method to obtain the network statistics would be access them using Win32 API. This method is significantly more performant than using Powershell cmdlet. Therefore, we are switching the agent implementation to use native Win32 API for retrieving the same.

Implementation

When we initialise the stats module for a given task, we will query and store the interface LUID of the task ENI. During the subsequent calls to retrieve the network statistics, we will query the statistics specific to the interface using it's LUID.

In order to accomplish the same, we are introducing two new APIs in NetworkUtils interface would be the preferred way to access Windows networking APIs in the agent. The two APIs are-

ConvertInterfaceAliasToLUID: Converts the interface alias into the corresponding LUID.
- Internally invokes ConvertInterfaceAliasToLuid Win32 API to convert interface alias into LUID.
GetMIBIfEntryFromLUID: Returns the MIB_IF_ROW2 for the interface with the given LUID.
- Internally invokes GetIfEntry2Ex Win32 API to obtain the specific interface which has given LUID.

Reference:

Note: As part of this PR, refactoring has been done to improve the code quality of existing Windows workflow.

Removed Setter method from networkUtils package
Removed Setter method from watcher package
Introduced mocks for networkUtils package and used the same in watcher tests instead of object injection.

Testing

Tested the new agent using a custom AMI.
Tested it out on smaller instance types for any periodic spikes.

New tests cover the changes:
Yes

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.

ofiliz · 2022-10-10T18:40:54Z

agent/stats/statsretriever/stats_retriever_windows.go

+
+	// We would be using GetIfTable2Ex and FreeMibTable Win32 APIs from IP Helper.
+	moduleIPHelper := windows.NewLazySystemDLL("iphlpapi.dll")
+	procGetIfTable2Ex := moduleIPHelper.NewProc("GetIfTable2Ex")


Why are we retrieving the entire table and not the specific interface row?
https://learn.microsoft.com/en-us/windows/win32/api/netioapi/nf-netioapi-getifentry2ex?

Ah we have been searching for this particular method all along. Unfortunately, we were not aware about the same.
I'll make the changes to use GetIfEntry2Ex API.

jterry75 · 2022-10-14T16:50:54Z

agent/stats/task_windows.go

+			cancel()
+			return nil, errors.Wrapf(err, "failed to initialise stats task container")
+		}
+		ifaceLUID[index] = ifEntry.InterfaceLUID


I still dont quite understand. Cant we just use ConvertInterfaceIndexToLuid for this and then be done? We dont need to do an entire mib table and filter it right?

So the thing is that we do not store interface index for the task network adapters. There can be two cases here-

We query the interface index first using net.Interfaces and then use ConvertInterfaceIndexToLuid in the stats module itself. This leads to two Win32 calls. Now, net.Interfaces would in turn call GetAdaptersAddresses which fetches all the addresses associated with the adapters. I feel that a single call to GetIfTable2Ex might be more performant than these two calls.

We store the ENI's interface index along with Link name during the initial task attachment workflow. Now, the issue here is that if the interface index becomes stale for some reason say, instance reboot, we would not have any mechanism to identify or refresh the same.

jterry75 · 2022-10-14T16:53:11Z

agent/eni/watcher/watcher_windows_test.go

@@ -87,6 +86,7 @@ func newTestWatcher(ctx context.Context, primaryMAC string, state dockerstate.Ta
 	notificationChannel := make(chan int)
 	err := eniMonitor.Start(notificationChannel)
 	if err != nil {
+		cancel()


This is fine but we dont even use ctx until the &ENIWatcher below .If we did not early alloc it we would avoid this case entirely

Basically move ln 84 to 92

jterry75 · 2022-10-14T16:58:43Z

agent/eni/networkutils/utils_windows.go

+// GetMIBIfEntryFromDeviceName return the MIB_IF_ROW2 object for the queried device.
+// This method would internally invoke the GetIfTable2Ex Windows API to get complete interface list
+// from which the queried device would be filtered.
+func (utils *networkUtils) GetMIBIfEntryFromDeviceName(device string) (*MibIfRow2, error) {


Look at my other comment using the LUID. I think we can remove this entirely.

jterry75 · 2022-10-14T17:01:17Z

agent/eni/networkutils/utils_windows.go

+}
+
+// getIfEntry2Ex invokes GetIfEntry2Ex Win32 API to retrieve the MIB_IF_ROW2 for the interface with given LUID.
+func getIfEntry2Ex(ifRow *MibIfRow2) (*MibIfRow2, error) {


Did we add this extra function just for unit testing?

Yes we did :)

jterry75 · 2022-10-14T17:03:02Z

agent/eni/networkutils/utils_windows_test.go

+}
+
+// TestGetMIBIfEntryFromLUID tests the GetMIBIfEntryFromLUID method.
+func TestGetMIBIfEntryFromLUID(t *testing.T) {


Since we went to all the trouble here lets add the negative test if we return error from the call too.

jterry75 · 2022-10-14T17:03:56Z

agent/eni/watcher/watcher_windows.go

@@ -65,6 +65,7 @@ func newWatcher(ctx context.Context,
 	notificationChannel := make(chan int)
 	err := eniMonitor.Start(notificationChannel)
 	if err != nil {
+		cancel()


Same, move ln 62 to 72 and then we dont have to handle this case.

jterry75 · 2022-10-14T17:05:06Z

agent/stats/task_windows.go

 	devices := make([]string, len(taskENIs))
+	ifaceLUID := make([]uint64, len(taskENIs))


You can have multiple eni's per task?

That is the case with trunk ENI I think.

jterry75 · 2022-10-14T17:09:45Z

agent/stats/task_windows_test.go

@@ -108,14 +85,16 @@ func TestTaskStatsCollection(t *testing.T) {
 			TaskMetadata: &TaskMetadata{
 				TaskArn:          taskId,
 				ContainerPID:     containerPID,
-				DeviceName:       []string{"Ethernet 3"},
+				DeviceName:       []string{deviceName},


Why are we using DeviceName? Isnt the ACS api based on mac and then we find index? Once we find Index we can get LUID. And from there we are good to go for basically all win32 api's. Why use name?

Well that is the current workflow which is shared with the Linux. We store the stats corresponding to each device name. Reference:

amazon-ecs-agent/agent/stats/task.go

Line 151 in 67240c1

networkStats, err := taskStat.retrieveNetworkStatistics()

Regarding fetching interface index, I have mentioned the same in earlier comment.

Presently, we have setter methods present in the networkutils and watcher package instead of having mock methods for the same. This is inconsistent with rest of the agent package and goes against Go guidelines.

Currently, we are running a Powershell method (Get-NetAdapterStatistics) to retrieve the network statistics in case of awsvpc network mode. However, this can cause CPU spikes on smaller instance types. Therefore, we are switching the method to use native Win32 API for retrieving the same. As part of this change, we have added two new APIs to existing NetworkUtils interface applicable for Windows. - ConvertInterfaceAliasToLUID: Returns the LUID corresponding to the given interface alias. - GetMIBIfEntryFromLUID: Returns the MIB_IF_ROW for the specified LUID. Windows API reference: https://learn.microsoft.com/en-us/windows/win32/api/netioapi/nf-netioapi-convertinterfacealiastoluid https://learn.microsoft.com/en-us/windows/win32/api/netioapi/nf-netioapi-getifentry2ex

jterry75

LGTM!

rawahars requested a review from a team as a code owner October 10, 2022 18:28

rawahars requested review from jterry75 and a team October 10, 2022 18:29

ofiliz reviewed Oct 10, 2022

View reviewed changes

rawahars force-pushed the dev branch from e77c0a6 to 13adf28 Compare October 12, 2022 06:53

rawahars marked this pull request as draft October 12, 2022 07:15

rawahars force-pushed the dev branch from 13adf28 to 0270f37 Compare October 14, 2022 11:54

rawahars marked this pull request as ready for review October 14, 2022 12:53

jterry75 reviewed Oct 14, 2022

View reviewed changes

Harsh Rawat added 2 commits October 15, 2022 01:32

refactored the networkutils and windows watcher package

4cdc8bc

Presently, we have setter methods present in the networkutils and watcher package instead of having mock methods for the same. This is inconsistent with rest of the agent package and goes against Go guidelines.

rawahars force-pushed the dev branch from 0270f37 to bc51c59 Compare October 14, 2022 20:03

singholt added the bot/test label Oct 14, 2022

amazon-ecs-bot removed the bot/test label Oct 14, 2022

jterry75 approved these changes Oct 17, 2022

View reviewed changes

sparrc approved these changes Oct 17, 2022

View reviewed changes

singholt approved these changes Oct 18, 2022

View reviewed changes

singholt merged commit 8680732 into aws:dev Oct 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

windows: changed method for retrieving Windows network statistics in case of awsvpc network mode #3425

windows: changed method for retrieving Windows network statistics in case of awsvpc network mode #3425

rawahars commented Oct 10, 2022 •

edited

Loading

ofiliz Oct 10, 2022

rawahars Oct 12, 2022

jterry75 Oct 14, 2022

rawahars Oct 14, 2022

jterry75 Oct 14, 2022

jterry75 Oct 14, 2022

jterry75 Oct 14, 2022

jterry75 Oct 14, 2022

rawahars Oct 14, 2022

jterry75 Oct 14, 2022

jterry75 Oct 14, 2022

jterry75 Oct 14, 2022

rawahars Oct 14, 2022

jterry75 Oct 14, 2022

rawahars Oct 14, 2022

jterry75 left a comment

		devices := make([]string, len(taskENIs))
		ifaceLUID := make([]uint64, len(taskENIs))

windows: changed method for retrieving Windows network statistics in case of awsvpc network mode #3425

windows: changed method for retrieving Windows network statistics in case of awsvpc network mode #3425

Conversation

rawahars commented Oct 10, 2022 • edited Loading

Summary

Implementation

Testing

Description for the changelog

Licensing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jterry75 left a comment

Choose a reason for hiding this comment

rawahars commented Oct 10, 2022 •

edited

Loading