-
Notifications
You must be signed in to change notification settings - Fork 359
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.
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
Remove Docker Support for Antrea Windows #6019
Conversation
ec59bb1
to
ab12903
Compare
c0a0630
to
3e3921e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Containerd runtime, the major difference from Docker include,
- we may have only one call on the sandbox container when creating Pods, so we don't have the need to check if the request is infra container or not, then the related functions and their calls are supposed to be removed in this function;
- OVS port/interface creation is async with Containerd runtime, but it is sync call with Docker runtime. So we shall have some changes on the OVS port/interface maintanance logic.
if err != nil { | ||
return nil, err | ||
} | ||
// Support for Docker runtime has been deprecated in Antrea 2.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you have a check in server.CmdAdd/CmdDel/CmdCheck to refuse the request with Docker runtime, we shall not hit this condition in the current function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
f31d1e3
to
77501ba
Compare
for _, existingEP := range attachedEpIds { | ||
if existingEP == hcnEp.Id { | ||
attached = true | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I feel the variable
attached
is useless now after Docker is removed. - Need to refine the lines between 245 and 259 as there are some stale logic only for docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@@ -227,26 +226,19 @@ func attachContainerLink(ep *hcsshim.HNSEndpoint, containerID, sandbox, containe | |||
var attached bool | |||
var err error | |||
var hcnEp *hcn.HostComputeEndpoint | |||
if isDockerContainer(sandbox) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need to keep functions isDockerContainer
, getInfraContainer
and the calls on them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isDockerContainer is still needed to validate docker runtime.
1f7460c
to
2392483
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you forget to check function configureContainerLink
?
@@ -56,7 +56,6 @@ const ( | |||
|
|||
const ( | |||
defaultOVSInterfaceType int = iota //nolint suppress deadcode check for windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove defaultOVSInterfaceType
as it is not used any more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
900be3c
to
c9f9776
Compare
c9f9776
to
0fc0465
Compare
/test-windows-containerd-e2e |
da04500
to
b13bf49
Compare
attachedEpIds, err := getNamespaceEndpointIDsFunc(sandbox) | ||
if err != nil { | ||
return nil, err | ||
} | ||
for _, existingEP := range attachedEpIds { | ||
if existingEP == hcnEp.Id { | ||
attached = true | ||
break | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on some earlier review comments, I see that removing this code was intentional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we don't need the attached
variable because there are no multiple cni calls in containerd.
// getOVSInterfaceType returns "internal". Windows uses internal OVS interface for container vNIC. | ||
func getOVSInterfaceType(ovsPortName string) int { | ||
ifaceName := fmt.Sprintf("vEthernet (%s)", ovsPortName) | ||
if !hostInterfaceExistsFunc(ifaceName) { | ||
return defaultOVSInterfaceType | ||
} | ||
return internalOVSInterfaceType | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was specific to Docker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was introduced to be consistent with Docker. For containerd use case, we have two stages for OVS port configurations: 1) create system interface to use OVSDB to store the Pod configurations; 2) modify the interface type as internal to connect to the vNIC which container is actually using. So the OVS interface type in each stage is fixed, so we don't need any switches any more.
// - For Docker runtime, the container interface is created after antrea-agent attaches the HNSEndpoint to the | ||
// sandbox container, so we create OVS port synchronously. | ||
// - Here antrea-agent determines the way of OVS port creation by checking if container interface is yet created. | ||
// If one day containerd runtime changes the behavior and container interface can be created when attaching |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If one day containerd runtime changes ...
With this change, we are losing this "compatibility guarantee" in case of a change of behavior for containerd. Do we think this is the best approach? Is there some new information available that makes us think that this code should be removed? cc @wenyingd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Till now, we didn't find containerd change its behaviors. So we have to use async solutions to ensure OVS is attaching the vNIC (Interface) which container actually uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the need for the async solution, as that was discussed at length in the past. However, it seems that at the time this code was written, someone found value in ensuring the code would keep working if containerd ever changed its implementation (unlikely). What I'm asking is: should we be cautious and preserve this behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is necessary to keep docker-like behavior as we didn't get any clue to show that containerd would modify its current workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll trust you on that and we can remove that logic entirely
04d769a
to
a0b79ef
Compare
/test-windows-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM, couple of questions about the test script (mostly unrelated to this PR) and a few nits
pkg/agent/cniserver/server.go
Outdated
if !validateRuntime(netNS) { | ||
// Support for Docker runtime on Windows has been removed since Antrea 2.0. | ||
return nil, fmt.Errorf("failed to process CmdAdd request because Docker runtime is not supported after Antrea 2.0") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this may be more elegant:
if err := validateRuntime(netNS); err != nil {
return nil, fmt.Errorf("failed to validate container runtime for CmdAdd request: %w")
}
with (for Windows):
func validateRuntime(netNS string) bool {
if isDockerContainer(netNS) {
return fmt.Errorf("Docker runtime is not supported after Antrea 2.0 for Windows Nodes")
}
return nil
}
This way we don't have to add any Windows-specific comment to server.go.
I would say the code is self-explanatory in this case, and no extra comment is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, thanks.
1c0d244
to
9d0a62a
Compare
/test-windows-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a minor comment. Please address it but no need to re-run Windows CI tests afterwards if this is the only change.
This patch removes the support for Docker from the Antrea Windows Agent. The specific changes made in this commit include modifying the CNI call to return an error when the runtime is identified as Docker on Windows and deleting related functions. Signed-off-by: Shuyang Xin <gavinx@vmware.com>
9d0a62a
to
190c51f
Compare
@antoninbas Sure, PR updated, PTAL. |
Only change was a comment update, no need to run all tests again |
/skip-all |
This patch removes the support for Docker from the Antrea Windows Agent.
The specific changes made in this commit include modifying the CNI call to return an error when the runtime is identified as Docker on Windows and deleting related functions.