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.

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

[Windows] Remove HNSEndpoint only if infra container fails to create #2976

Merged

Conversation

lzhecheng
Copy link
Contributor

For non-infra containers, there's no need to remove HNSEndpoint if failure since it will be done when the Pod is removed.

Signed-off-by: Zhecheng Li lzhecheng@vmware.com

@lzhecheng lzhecheng requested a review from tnqn November 4, 2021 07:01
@lzhecheng lzhecheng added the area/OS/windows Issues or PRs related to the Windows operating system. label Nov 4, 2021
@lzhecheng lzhecheng requested a review from wenyingd November 4, 2021 07:01
@@ -140,7 +140,9 @@ func (ic *ifConfigurator) configureContainerLink(
containerIface, err := attachContainerLink(endpoint, containerID, containerNetNS, containerIFDev)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have check outside the func attachContainerLink, maybe we could remove this check: https://github.com/antrea-io/antrea/blob/main/pkg/agent/cniserver/interface_configuration_windows.go#L302

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2021

Codecov Report

Merging #2976 (a135fb5) into main (0feed23) will increase coverage by 20.63%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #2976       +/-   ##
===========================================
+ Coverage   40.23%   60.86%   +20.63%     
===========================================
  Files         166      289      +123     
  Lines       20693    24540     +3847     
===========================================
+ Hits         8326    14937     +6611     
+ Misses      11558     7974     -3584     
- Partials      809     1629      +820     
Flag Coverage Δ
kind-e2e-tests 47.93% <ø> (?)
unit-tests 40.20% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/apiserver/handlers/webhook/convert_crd.go 2.56% <0.00%> (ø)
pkg/apiserver/handlers/webhook/mutation_labels.go 24.71% <0.00%> (ø)
...s/externalversions/core/v1alpha2/externalentity.go 64.28% <0.00%> (ø)
...g/agent/apiserver/handlers/addressgroup/handler.go 40.00% <0.00%> (ø)
pkg/agent/apiserver/handlers/agentinfo/handler.go 38.23% <0.00%> (ø)
...gacyclient/listers/core/v1alpha2/externalentity.go 6.25% <0.00%> (ø)
pkg/legacyapis/core/v1alpha2/register.go 80.00% <0.00%> (ø)
pkg/apis/stats/install/install.go 100.00% <0.00%> (ø)
pkg/legacyapis/stats/install/install.go 100.00% <0.00%> (ø)
pkg/agent/interfacestore/interface_cache.go 81.37% <0.00%> (ø)
... and 230 more

@lzhecheng lzhecheng force-pushed the win-remove-hnsendpoint-if-infra-container branch from 533a0fe to 4ec8d8e Compare November 4, 2021 08:57
@lzhecheng
Copy link
Contributor Author

/test-all /test-windows-all

Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

In the commit message:

there's no need to remove HNSEndpoint if failure

"if failure" -> "at failure" or "if fails to attach the HNS Endpoint"

For non-infra containers, there's no need to remove HNSEndpoint at
failure since it will be done when the Pod is removed.

Signed-off-by: Zhecheng Li <lzhecheng@vmware.com>
@lzhecheng lzhecheng force-pushed the win-remove-hnsendpoint-if-infra-container branch from 4ec8d8e to a135fb5 Compare November 5, 2021 00:24
@lzhecheng
Copy link
Contributor Author

@jianjuns Done.

@lzhecheng
Copy link
Contributor Author

/test-all /test-windows-all

@lzhecheng
Copy link
Contributor Author

/test-windows-e2e /test-e2e /test-windows-networkpolicy

@lzhecheng
Copy link
Contributor Author

@tnqn @jianjuns could you please help merge the PR? Thanks.

@jianjuns jianjuns merged commit ff17f09 into antrea-io:main Nov 11, 2021
@jianjuns
Copy link
Contributor

@tnqn @jianjuns could you please help merge the PR? Thanks.

Done.

@lzhecheng lzhecheng deleted the win-remove-hnsendpoint-if-infra-container branch November 12, 2021 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/OS/windows Issues or PRs related to the Windows operating system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants