Read network configuration from a file on azure stack #369
Conversation
Codecov Report
@@ Coverage Diff @@
## master #369 +/- ##
==========================================
+ Coverage 40.01% 40.93% +0.91%
==========================================
Files 25 25
Lines 3546 3518 -28
==========================================
+ Hits 1419 1440 +21
+ Misses 1927 1870 -57
- Partials 200 208 +8
Continue to review full report at Codecov.
|
tamilmani1989
left a comment
tamilmani1989
left a comment
There was a problem hiding this comment.
PR looks good except for minor comments. Can we have few unit tests with multi nic configuration (both sdn and local). One will be primary and rest are secondary
ipam/mas.go
Outdated
| } | ||
|
|
||
| func populateAddressSpace(localAddressSpace *addressSpace, sdnInterfaces *NetworkInterfaces, localInterfaces []net.Interface) error { | ||
|
|
| // Minimum time interval between consecutive queries. | ||
| masQueryInterval = 10 * time.Second | ||
| defaultLinuxFilePath = "/etc/kubernetes/interfaces.json" | ||
| defaultWindowsFilePath = `c:\k\interfaces.json` |
There was a problem hiding this comment.
just a small question -> does this json file exists after reboot?
ipam/mas.go
Outdated
| decoder := json.NewDecoder(resp.Body) | ||
| err = decoder.Decode(&obj) | ||
| // Set the local address space as active. | ||
| err = source.sink.setAddressSpace(local) |
There was a problem hiding this comment.
we moved to this standard few months back. If the function returns only error, we can have this std.
if err := source.sink.setAddressSpace(local); err != nil {
return err
}
ipam/mas.go
Outdated
| return err | ||
| } | ||
|
|
||
| err = populateAddressSpace(local, sdnInterfaces, localInterfaces) |
There was a problem hiding this comment.
change this also to below std
ipam/mas.go
Outdated
| } | ||
|
|
||
| interfaces := &NetworkInterfaces{} | ||
| err = json.Unmarshal(data, interfaces) |
| IPAddresses: []IPAddress{ | ||
| {Address: "1.1.1.5", IsPrimary: true}, | ||
| {Address: "1.1.1.6", IsPrimary: false}, | ||
| {Address: "1.1.1.6", IsPrimary: false}, |
There was a problem hiding this comment.
can we have one more valid address and check the length of address records as 2?
ipam/mas.go
Outdated
| if queryInterval == 0 { | ||
| queryInterval = masQueryInterval | ||
| var filePath string | ||
| if runtime.GOOS == "windows" { |
ipam/mas.go
Outdated
| s.sink = nil | ||
| func (source *masSource) stop() { | ||
| source.sink = nil | ||
| return |
@t-chappl Can you add unit tests that I mentioned ? |
|
@tamilmani1989 did you see the new test I pushed this morning? It has multiple hardware and sdn interfaces. Are there other cases you would like me to cover? |
tamilmani1989
left a comment
tamilmani1989
left a comment
There was a problem hiding this comment.
Sorry i missed that. unit tests looks good. just a minor comment. Have you tested in windows also(the scenarios you mentioned in email)?
ipam/mas.go
Outdated
| } | ||
|
|
||
| log.Printf("[ipam] Address space successfully populated from config file") | ||
|
|
|
@tamilmani1989 I initially test the scenarios only on linux but I will try them out on windows right now. Sorry, I should have done it sooner. |
|
@tamilmani1989 I tried the scenarios on windows and they worked fine. |
tamilmani1989
left a comment
tamilmani1989
left a comment
There was a problem hiding this comment.
@t-chappl Thanks for this PR. lgtm.
3 participants
What this PR does / why we need it:
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
Release note: