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.

Sign up for GitHub

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

Make a deep copy for introspection #1179

Merged
merged 1 commit into from
Aug 31, 2020
Merged

Make a deep copy for introspection #1179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions pkg/ipamd/datastore/data_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -819,7 +819,7 @@ func (ds *DataStore) FreeableIPs(eniID string) []string {
return freeable
}

// GetENIInfos provides ENI IP information to introspection endpoint
// GetENIInfos provides ENI and IP information about the datastore
func (ds *DataStore) GetENIInfos() *ENIInfos {
ds.lock.Lock()
defer ds.lock.Unlock()
Expand All @@ -831,7 +831,14 @@ func (ds *DataStore) GetENIInfos() *ENIInfos {
}

for eni, eniInfo := range ds.eniPool {
eniInfos.ENIs[eni] = *eniInfo
tmpENIInfo := *eniInfo
tmpENIInfo.IPv4Addresses = make(map[string]*AddressInfo, len(eniInfo.IPv4Addresses))
// Since IP Addresses might get removed, we need to make a deep copy here.
for eni, ipAddrInfoRef := range eniInfo.IPv4Addresses {
ipAddrInfo := *ipAddrInfoRef
tmpENIInfo.IPv4Addresses[eni] = &ipAddrInfo
}
eniInfos.ENIs[eni] = tmpENIInfo
}
return &eniInfos
}
Expand Down
4 changes: 1 addition & 3 deletions pkg/ipamd/introspect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,7 @@ func (c *IPAMContext) setupIntrospectionServer() *http.Server {

func eniV1RequestHandler(ipam *IPAMContext) func(http.ResponseWriter, *http.Request) {
return func(w http.ResponseWriter, r *http.Request) {
eniInfos := ipam.dataStore.GetENIInfos()
log.Debugf("eniInfos: %+v", eniInfos)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we could have caught this yesterday itself.. can you add the logger back and verify the change by looking at the output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I did have the logger there when testing the change, but it's hard to say from those logs what the pointer is pointing to:

{"level":"info","ts":"2020-08-30T17:17:54.515Z","caller":"http/server.go:2416","msg":"Handling http request: GET, from: 127.0.0.1:41846, URI: /v1/enis"}
{"level":"debug","ts":"2020-08-30T17:17:54.515Z","caller":"http/server.go:2041","msg":"eniInfos: &{TotalIPs:28 AssignedIPs:3 ENIs:map[eni-04e635dd5bb78338c:{ID:eni-04e635dd5bb78338c createTime:{wall:13820163718338356131 ext:1018422474 loc:0x558e92c0b3c0} IsPrimary:true IsTrunk:false DeviceNumber:0 IPv4Addresses:map[192.168.67.189:0xc000373020 192.168.69.119:0xc0003734a0 192.168.73.173:0xc0003733e0 192.168.74.37:0xc0003731a0 192.168.76.135:0xc000373200 192.168.79.14:0xc000373260 192.168.84.205:0xc000373140 192.168.86.63:0xc000373380 192.168.87.28:0xc000373440 192.168.88.155:0xc000372fc0 192.168.88.244:0xc0003730e0 192.168.90.222:0xc000373080 192.168.92.61:0xc000373320 192.168.94.188:0xc0003732c0]} eni-099ea6395cdb509e6:{ID:eni-099ea6395cdb509e6 createTime:{wall:13820163718338447800 ext:1018514140 loc:0x558e92c0b3c0} IsPrimary:false IsTrunk:true DeviceNumber:4 IPv4Addresses:map[192.168.69.116:0xc000373620 192.168.70.178:0xc000373800 192.168.70.45:0xc0003736e0 192.168.72.114:0xc000373980 192.168.77.192:0xc000373920 192.168.79.195:0xc000373560 192.168.80.157:0xc0003737a0 192.168.84.107:0xc000373500 192.168.87.154:0xc0003735c0 192.168.87.206:0xc0003738c0 192.168.87.234:0xc000373680 192.168.88.225:0xc000373740 192.168.91.242:0xc0003739e0 192.168.92.75:0xc000373860]}]}"}
{"level":"info","ts":"2020-08-30T17:17:57.426Z","caller":"http/server.go:2416","msg":"Handling http request: GET, from: 127.0.0.1:41866, URI: /v1/enis"}

These refs now point to tmpENIInfo.IPv4Addresses[eni] = &ipAddrInfo instead of the values in the map owned by *eniInfo. Do we still want this debug log added?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we are good now :) Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

curl output:

$ curl localhost:61679/v1/enis
{"TotalIPs":42,"AssignedIPs":3,"ENIs":{"eni-04e635dd5bb78338c":{"ID":"eni-04e635dd5bb78338c","IsPrimary":true,"IsTrunk":false,"DeviceNumber":0,"IPv4Addresses":{"192.168.67.189":{"IPAMKey":{"networkName":"","containerID":"","ifName":""},"Address":"192.168.67.189","UnassignedTime":"0001-01-01T00:00:00Z"},"192.168.69.119":{"IPAMKey":{"networkName":"","containerID":"","ifName":""},"Address":"192.168.69.119","UnassignedTime":"0001-01-01T00:00:00Z"},"192.168.73.173":{"IPAMKey":{"networkName":"","containerID":"","ifName":""},"Address":"192.168.73.173","UnassignedTime":"0001-01-01T00:00:00Z"},"192.168.74.37":{"IPAMKey":{"networkName":"","containerID":"","ifName":""},"Address":"192.168.74.37","UnassignedTime":"0001-01-01T00:00:00Z"},...

responseJSON, err := json.Marshal(eniInfos)
responseJSON, err := json.Marshal(ipam.dataStore.GetENIInfos())
if err != nil {
log.Errorf("Failed to marshal ENI data: %v", err)
http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
Expand Down
10 changes: 5 additions & 5 deletions pkg/ipamd/ipamd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -908,13 +908,13 @@ func (c *IPAMContext) nodeIPPoolReconcile(interval time.Duration) {
return
}
attachedENIs := c.filterUnmanagedENIs(allENIs)
currentENIIPPools := c.dataStore.GetENIInfos().ENIs
currentENIs := c.dataStore.GetENIInfos().ENIs
trunkENI := c.dataStore.GetTrunkENI()

// Check if a new ENI was added, if so we need to update the tags.
needToUpdateTags := false
for _, attachedENI := range attachedENIs {
if _, ok := currentENIIPPools[attachedENI.ENIID]; !ok {
if _, ok := currentENIs[attachedENI.ENIID]; !ok {
needToUpdateTags = true
break
}
Expand Down Expand Up @@ -950,8 +950,8 @@ func (c *IPAMContext) nodeIPPoolReconcile(interval time.Duration) {
log.Debugf("Reconcile existing ENI %s IP pool", attachedENI.ENIID)
// Reconcile IP pool
c.eniIPPoolReconcile(eniIPPool, attachedENI, attachedENI.ENIID)
// Mark action, remove this ENI from currentENIIPPools map
delete(currentENIIPPools, attachedENI.ENIID)
// Mark action, remove this ENI from currentENIs map
delete(currentENIs, attachedENI.ENIID)
continue
}

Expand All @@ -968,7 +968,7 @@ func (c *IPAMContext) nodeIPPoolReconcile(interval time.Duration) {
}

// Sweep phase: since the marked ENI have been removed, the remaining ones needs to be sweeped
for eni := range currentENIIPPools {
for eni := range currentENIs {
log.Infof("Reconcile and delete detached ENI %s", eni)
// Force the delete, since aws local metadata has told us that this ENI is no longer
// attached, so any IPs assigned from this ENI will no longer work.
Expand Down