[Windows] Fix NetNeighbor Powershell error handling #2905
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2905 +/- ##
==========================================
- Coverage 60.32% 52.67% -7.66%
==========================================
Files 283 283
Lines 23603 23644 +41
==========================================
- Hits 14239 12454 -1785
- Misses 7845 9803 +1958
+ Partials 1519 1387 -132
Flags with carried forward coverage won't be shown. Click here to find out more.
|
pkg/agent/util/net_windows.go
Outdated
| } | ||
| // If no such neighbor exists, exit code will be 1 and RunCommand() returns error. | ||
| // Since Get-NetNeighbor ignores error, it should be ignored. | ||
| neighborsStr, _ := ps.RunCommand(cmd) |
There was a problem hiding this comment.
I don't understand the comment. The first sentence says it will return error, but the second one says the error should be ignored?
There was a problem hiding this comment.
If neighbor doesn't exist, it will return error but I don't think no such neighbor means error. So I suggest ignoring error. What do you think? And @wenyingd please share your idea.
There was a problem hiding this comment.
Discussed with Wenying, now the code doesn't ignore error and checks if error contains "No matching MSFT_NetNeighbor objects"
lzhecheng
force-pushed
the
win-fix-antreaproxy
branch
2 times, most recently
from
d6cc337
to
eb5c441
Compare
|
/test-windows-proxyall-e2e |
|
/test-all /test-windows-all |
|
/test-conformance |
|
@tnqn The tests are successful so please help merge the PR. |
| return err | ||
| } | ||
| return nil | ||
| return NewNetNeighbor(neighbor) |
There was a problem hiding this comment.
question: if the neighbor for the IP exists but the MAC or the stat doesn't match, can NewNetNeighbor override it?
There was a problem hiding this comment.
You're right. No, it says Instance MSFT_NetNeighbor already exist. I added a RemoveNetNeighbor before it.
lzhecheng
force-pushed
the
win-fix-antreaproxy
branch
from
eb5c441
to
dfab715
Compare
pkg/agent/util/net_windows.go
Outdated
| if n.LinkLayerAddress.String() == neighbor.LinkLayerAddress.String() { | ||
| found = true | ||
| break | ||
| if n.LinkLayerAddress.String() == neighbor.LinkLayerAddress.String() && n.State == "Permanent" { |
There was a problem hiding this comment.
As a generic util, I think this should be:
| if n.LinkLayerAddress.String() == neighbor.LinkLayerAddress.String() && n.State == "Permanent" { | |
| if n.LinkLayerAddress.String() == neighbor.LinkLayerAddress.String() && n.State == neighbor.State { |
And the callers should set the desired State
There was a problem hiding this comment.
Good idea. Updated.
Stop ignoring Get-NetNeighbor's error and check if the error message includes "No matching MSFT_NetNeighbor objects" Signed-off-by: Zhecheng Li <[email protected]>
lzhecheng
force-pushed
the
win-fix-antreaproxy
branch
from
dfab715
to
97c6454
Compare
|
/test-all |
Signed-off-by: Zhecheng Li [email protected]