Added the Windows DaemonManager and ManagedDaemon by saurabhc123 · Pull Request #2 · saurabhc123/amazon-ecs-agent

saurabhc123 · 2024-06-04T00:47:26Z

Summary

This PR adds the DaemonManager and ManagedDaemon components to support EBS-TaskAttach for Windows.

Implementation details

We refactored the existing DaemonManager and ManagedDaemon modules for linux and repurposed the high-level workflow. The platform specific details were moved to the respective windows/linux modules.

Testing

We used the existing linux tests and refactored them so that all of them can be repurposed for Windows.

New tests cover the changes: Yes

Description for the changelog

Add support EBS-TaskAttach for Windows.

Licensing

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

rawahars · 2024-06-04T17:29:11Z

 	return errors.New("task with FSx for Windows File Server volumes is only supported on Windows container instance")
 }

+func (task *Task) getTaskUser(container *container.Container) string {


This code will return empty user string for Linux when the conditions are not met.
That is not how it is in presently for Linux.

Maybe return container.user itself

I got this verified by the Linux team. They don't care about what user it is. I also checked with them on the change of the sequence of operation too. They agreed with the change.

Technically it is correct as the struct is declared in the method and therefore would be initialised with empty string. But think from method construction viewpoint, the name is getTaskUser and it returns empty unless specific conditions are met. It sets an incorrect precedent for the future where folks would be confused about the reason for setting it as empty and not returning the actual user.

I would strongly suggest that this be rectified.

rawahars · 2024-06-04T17:32:09Z

+
+const (
+	csiDriverSocketAddress = "C:\\ProgramData\\Amazon\\ECS\\ebs-csi-driver\\csi-driver.sock"
+	hostMountDir           = "C:\\var\\lib\\kubelet"


why do we have kubelet here?

CSIProxt won't allow to mount in any folder other than something under this. This needs to be investigated further for the CSIDriver work.

rawahars · 2024-06-04T17:36:31Z

-			logger.Fields{
-				field.Image: loadedImageRef,
-			})
-		mountPoint := dockermount.Mount{


Why mounts were skipped in the common code?

No mounts were skipped in the common code. The previous code was hardcoding the mount type to bind and we want some of our mounts to be npipe which we are passing from upstream and processing accordingly.

https://github.com/saurabhc123/amazon-ecs-agent/pull/2/files#diff-c27a802d01286c3a45f84d314f4bf1feb2448370cba4991ebf67d9afdc433d5aR116-R131

rawahars · 2024-06-04T17:43:12Z

 	fsTypeBlockName = "block"
 )

+func DefaultSocketFilePath() string {


Please add comments

rawahars · 2024-06-04T17:44:57Z

+)
+
+const (
+	imageTarPath                          = "C:\\ProgramData\\Amazon\\ECS\\data\\"


Why do we need a separate file with the same method.
Can we just convert the values as constants declared in platform files and then keep the method common?

rawahars · 2024-06-04T17:45:24Z

+	imageTarPath                          = "C:\\ProgramData\\Amazon\\ECS\\data\\"
+	imageTagDefault                       = "latest"
+	defaultAgentCommunicationPathHostRoot = "C:\\ProgramData\\Amazon\\ECS"
+	defaultApplicationLogPathHostRoot     = "C:\\ProgramData\\Amazon\\ECS\\log"


Again, please do not hardcode ProgramData directly as on ECS EC2, customers can set their own path for the same.

rawahars · 2024-06-04T17:46:04Z

+)
+
+const (
+	imageTarPath                          = "C:\\ProgramData\\Amazon\\ECS\\data\\"


Same comments as earlier. I don't think we need to copy and fork the method.

This is only method that needs forking. Reason being, there are different mount points for Windows and especially because of the additional ones for the CSIProxy. In addition to this, the configuration of ADMIN and the KernelCapabilities (windows doesn't need it) varies between linux and windows. There is very little common between them.

rawahars · 2024-06-06T11:09:29Z


+// For Windows task run via the docker DaemonManager, we will run them as ContainerAdministrator
+func (task *Task) getTaskUser(container *container.Container) string {
+	return config.ContainerAdminUser


Similarly, we need to check the code paths from where the docker config method is called. Setting user to containerAdministrator in all container configs can have unintended impact. Take example that a customer specifies a lower user to run the container but we set it to Admin.

Correct approach would be to not pass the user for Windows. Keep it empty and then let the container take user as defined in the image.

rawahars · 2024-06-06T11:11:15Z

+	// containerAdminUser is the admin username for any container on Windows.
+	ContainerAdminUser = "ContainerAdministrator"
+	// This is the path that will be used to store the local named pipe for CSI Proxy
+	ManagedDaemonSocketPathHostRoot = "C:\\ProgramData\\Amazon\\ECS\\ebs-csi-driver"


Same as comment from previous review.

Please do not hardcode ProgramData path. It can change dynamically on ECS EC2.
Correct approach is to construct it via ProgramData Env as has been done in other parts of the agent.

rawahars · 2024-06-06T11:15:05Z

While the review is not completely ready to be shipped, there are not grave concerns except a few regarding setting of container user. Please create a formal PR in the aws repo and I can provide other review comments there. That would help Linux team have context and can chime in if needed about the practises they use in the agent.