Skip to content

Commit 39339ad

Browse files
jaydeokarjaydeokar
committed
Fix delete with prev result logic
1 parent 24a9c98 commit 39339ad

File tree

6 files changed

+89
-68
lines changed

6 files changed

+89
-68
lines changed

‎cmd/routed-eni-cni-plugin/cni.go

+30-28
Original file line numberDiff line numberDiff line change
@@ -170,11 +170,11 @@ func add(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
170170
requiresMultiNICAttachment = true
171171
} else {
172172
log.Debugf("Multi-NIC annotation mismatch: found=%q, required=%q. Falling back to default configuration.",
173-
multiNICAttachment, conf.RuntimeConfig.PodAnnotations[multiNICPodAnnotation])
173+
conf.RuntimeConfig.PodAnnotations[multiNICPodAnnotation], multiNICAttachment)
174174
}
175175
}
176176

177-
log.Debugf("pod requires Multi-NIC attachment: %t", requiresMultiNICAttachment)
177+
log.Debugf("pod requires multi-nic attachment: %t", requiresMultiNICAttachment)
178178

179179
// Set up a connection to the ipamD server.
180180
conn, err := grpcClient.Dial(ipamdAddress, grpc.WithTransportCredentials(insecure.NewCredentials()))
@@ -245,12 +245,11 @@ func add(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
245245
HostVethName: hostVethName,
246246
ContainerVethName: containerVethName,
247247
})
248-
249-
// Check if we can use PCIID to store RT-ID ?
250-
// The RT here is the host route table Id not the container
248+
// CNI stores the route table ID in the MAC field of the interface. The Route table ID is on the host side
249+
// and is used during cleanup to remove the ip rules when IPAMD is not reachable.
251250
podInterfaces = append(podInterfaces,
252251
&current.Interface{Name: hostVethName},
253-
&current.Interface{Name: containerVethName, Sandbox: args.Netns, PciID: fmt.Sprint(ip.RouteTableId)},
252+
&current.Interface{Name: containerVethName, Sandbox: args.Netns, Mac: fmt.Sprint(ip.RouteTableId)},
254253
)
255254

256255
// This index always points to the container interface so that we get the IP address corresponding to the interface
@@ -276,7 +275,7 @@ func add(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
276275
} else {
277276
err = driverClient.SetupPodNetwork(vethMetadata, args.Netns, mtu, log)
278277
// For non-branch ENI, the pod VLAN ID value of 0 is packed in Interface.Mac, while the interface device number is packed in Interface.Sandbox
279-
dummyInterface = &current.Interface{Name: dummyInterfaceName, Mac: fmt.Sprint(0), Sandbox: fmt.Sprint(vethMetadata[0].DeviceNumber), SocketPath: fmt.Sprint(len(r.IPAddress))}
278+
dummyInterface = &current.Interface{Name: dummyInterfaceName, Mac: fmt.Sprint(0), Sandbox: fmt.Sprint(vethMetadata[0].DeviceNumber)}
280279
}
281280

282281
log.Debugf("Using dummy interface: %v", dummyInterface)
@@ -311,6 +310,8 @@ func add(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
311310
}
312311

313312
// dummy interface is appended to PrevResult for use during cleanup
313+
// The interfaces field should only include host,container and dummy interfaces in the list.
314+
// Revisit the prevResult cleanup logic if this changes
314315
result.Interfaces = append(result.Interfaces, dummyInterface)
315316

316317
// Set up a connection to the network policy agent
@@ -345,7 +346,6 @@ func add(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
345346
}
346347

347348
log.Debugf("Network Policy agent for EnforceNpToPod returned Success : %v", npr.Success)
348-
349349
return cniTypes.PrintResult(result, conf.CNIVersion)
350350
}
351351

@@ -532,8 +532,13 @@ func getContainerNetworkMetadata(prevResult *current.Result, contVethName string
532532
// returns true if the del request is handled.
533533
func tryDelWithPrevResult(driverClient driver.NetworkAPIs, conf *NetConf, k8sArgs K8sArgs, contVethName string, netNS string, log logger.Logger) (bool, error) {
534534
// prevResult might not be available, if we are still using older cni spec < 0.4.0.
535-
prevResult, ok := conf.PrevResult.(*current.Result)
536-
if !ok {
535+
if conf.PrevResult == nil {
536+
return false, nil
537+
}
538+
539+
prevResult, err := current.NewResultFromResult(conf.PrevResult)
540+
if err != nil {
541+
log.Info("PrevResult not available for pod or parsing failed")
537542
return false, nil
538543
}
539544

@@ -574,8 +579,13 @@ func tryDelWithPrevResult(driverClient driver.NetworkAPIs, conf *NetConf, k8sArg
574579
// Returns true if pod network is torn down
575580
func teardownPodNetworkWithPrevResult(driverClient driver.NetworkAPIs, conf *NetConf, k8sArgs K8sArgs, contVethName string, log logger.Logger) bool {
576581
// For non-branch ENI, prevResult is only available in v1.12.1+
577-
prevResult, ok := conf.PrevResult.(*current.Result)
578-
if !ok {
582+
if conf.PrevResult == nil {
583+
log.Infof("PrevResult not available for pod. Pod may have already been deleted.")
584+
return false
585+
}
586+
587+
prevResult, err := current.NewResultFromResult(conf.PrevResult)
588+
if err != nil {
579589
log.Infof("PrevResult not available for pod. Pod may have already been deleted.")
580590
return false
581591
}
@@ -600,32 +610,24 @@ func teardownPodNetworkWithPrevResult(driverClient driver.NetworkAPIs, conf *Net
600610
// RT ID for NC-0 is also stored in the container interface entry. So we have a path for migration where
601611
// getting the device number from dummy interface can be deprecated entirely. This is currently done to keep it backwards compatible
602612
routeTableId := deviceNumber + 1
603-
604-
var interfacesAttached = 1
605-
if dummyIface.SocketPath != "" {
606-
interfacesAttached, err = strconv.Atoi(dummyIface.SocketPath)
607-
if err != nil {
608-
log.Errorf("error getting number of interfaces attached to the pod: %s", dummyIface.SocketPath)
609-
return false
610-
}
611-
}
612-
613+
// The number of interfaces attached to the pod is taken as the length of the interfaces array - 1 (for dummy interface) divided by 2 (for host and container interface)
614+
var interfacesAttached = (len(prevResult.Interfaces) - 1) / 2
613615
var vethMetadata []driver.VirtualInterfaceMetadata
614616
for v := range interfacesAttached {
615617
containerInterfaceName := networkutils.GenerateContainerVethName(contVethName, containerVethNamePrefix, v)
616618
containerIP, containerInterface, err := getContainerNetworkMetadata(prevResult, containerInterfaceName)
617619
if err != nil {
618-
log.Errorf("Failed to get container IP: %v", err)
619-
return false
620+
log.Errorf("container interface name %s does not exist %v", containerInterfaceName, err)
621+
continue
620622
}
621623

622624
// If this property is set, that means the container metadata has the route table ID which we can use
623625
// If this is not set, it is a pod launched before this change was introduced.
624-
// So it is only managing network card 0 at that time and device number + 1 is the route table ID which we have above
625-
if dummyIface.SocketPath != "" {
626-
routeTableId, err = strconv.Atoi(containerInterface.PciID)
626+
// So it is only managing network card 0 at that time and device number + 1 is the route table ID which we calculate from device number
627+
if containerInterface.Mac != "" {
628+
routeTableId, err = strconv.Atoi(containerInterface.Mac)
627629
if err != nil {
628-
log.Errorf("error getting route table number of the interface %s", containerInterface.PciID)
630+
log.Errorf("error getting route table number of the interface %s", containerInterface.Mac)
629631
return false
630632
}
631633
}

‎pkg/awsutils/awsutils.go

+2-5
Original file line numberDiff line numberDiff line change
@@ -918,12 +918,9 @@ func (cache *EC2InstanceMetadataCache) attachENI(eniID string, networkCard int)
918918

919919
// createENITags creates all the tags required to be added to the ENI
920920
//
921-
// Parameters:
922-
// - eniDescription (string):
923-
//
924921
// Returns:
925922
// - []ec2types.TagSpecification: Returns the tags by converting it into AWS SDK class
926-
func (cache *EC2InstanceMetadataCache) createENITags(eniDescription string) []ec2types.TagSpecification {
923+
func (cache *EC2InstanceMetadataCache) createENITags() []ec2types.TagSpecification {
927924

928925
tags := map[string]string{
929926
eniCreatedAtTagKey: time.Now().Format(time.RFC3339),
@@ -966,7 +963,7 @@ func (cache *EC2InstanceMetadataCache) createENIInput(eniDescription string, tag
966963
// return ENI id, error
967964
func (cache *EC2InstanceMetadataCache) createENI(sg []*string, eniCfgSubnet string, numIPs int) (string, error) {
968965
eniDescription := eniDescriptionPrefix + cache.instanceID
969-
tags := cache.createENITags(eniDescription)
966+
tags := cache.createENITags()
970967

971968
var needIPs = numIPs
972969

‎pkg/ipamd/datastore/data_store.go

+26-15
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,7 @@ type DataStore struct {
281281
netLink netlinkwrapper.NetLink
282282
isPDEnabled bool
283283
ipCooldownPeriod time.Duration
284+
networkCard int
284285
}
285286

286287
// ENIInfos contains ENI IP information
@@ -294,14 +295,15 @@ type ENIInfos struct {
294295
}
295296

296297
// NewDataStore returns DataStore structure
297-
func NewDataStore(log logger.Logger, backingStore Checkpointer, isPDEnabled bool) *DataStore {
298+
func NewDataStore(log logger.Logger, backingStore Checkpointer, isPDEnabled bool, networkCard int) *DataStore {
298299
return &DataStore{
299300
eniPool: make(ENIPool),
300301
log: log,
301302
backingStore: backingStore,
302303
netLink: netlinkwrapper.NewNetLink(),
303304
isPDEnabled: isPDEnabled,
304305
ipCooldownPeriod: getCooldownPeriod(),
306+
networkCard: networkCard,
305307
}
306308
}
307309

@@ -743,7 +745,7 @@ func (ds *DataStore) AssignPodIPv4Address(ipamKey IPAMKey, ipamMetadata IPAMMeta
743745

744746
// assignPodIPAddressUnsafe mark Address as assigned.
745747
func (ds *DataStore) assignPodIPAddressUnsafe(addr *AddressInfo, ipamKey IPAMKey, ipamMetadata IPAMMetadata, assignedTime time.Time) {
746-
ds.log.Infof("assignPodIPAddressUnsafe: Assign IP %v to sandbox %s with metadata %",
748+
ds.log.Infof("assignPodIPAddressUnsafe: Assign IP %v to sandbox %s with metadata %+v",
747749
addr.Address, ipamKey, ipamMetadata)
748750

749751
if addr.Assigned() {
@@ -1360,6 +1362,10 @@ func (ds *DataStore) getUnusedIP(availableCidr *CidrInfo) (string, error) {
13601362
return "", fmt.Errorf("no free IP available in the prefix - %s/%s", availableCidr.Cidr.IP, availableCidr.Cidr.Mask)
13611363
}
13621364

1365+
func (ds *DataStore) GetNetworkCard() int {
1366+
return ds.networkCard
1367+
}
1368+
13631369
func getNextIPAddr(ip net.IP) {
13641370
for j := len(ip) - 1; j >= 0; j-- {
13651371
ip[j]++
@@ -1547,34 +1553,39 @@ func (ds *DataStore) DeleteFromContainerRule(entry *CheckpointEntry) {
15471553
}
15481554

15491555
type DataStoreAccess struct {
1550-
DataStores map[int]*DataStore
1556+
DataStores []*DataStore
15511557
}
15521558

15531559
func InitializeDataStores(skipNetworkCards []bool, defaultDataStorePath string, enablePD bool, log logger.Logger) *DataStoreAccess {
15541560

1555-
var dsMap = make(map[int]*DataStore, len(skipNetworkCards))
1556-
for i, shouldSkip := range skipNetworkCards {
1557-
if shouldSkip {
1558-
continue
1559-
} else {
1561+
var dsList = make([]*DataStore, 0, len(skipNetworkCards))
1562+
for networkCard, shouldSkip := range skipNetworkCards {
1563+
1564+
if !shouldSkip {
15601565
dsBackingStorePath := defaultDataStorePath
1561-
if i > 0 {
1566+
if networkCard > 0 {
15621567
baseName := strings.TrimSuffix(defaultDataStorePath, ".json")
1563-
dsBackingStorePath = fmt.Sprintf("%s-nic-%d.json", baseName, i)
1568+
dsBackingStorePath = fmt.Sprintf("%s-nic-%d.json", baseName, networkCard)
15641569
}
15651570
checkpointer := NewJSONFile(dsBackingStorePath)
1566-
dsMap[i] = NewDataStore(log, checkpointer, enablePD)
1567-
log.Infof("initialized datastore for network cards index %d", i)
1571+
dsList = append(dsList, NewDataStore(log, checkpointer, enablePD, networkCard))
1572+
log.Infof("initialized datastore for network cards index %d", networkCard)
15681573
}
15691574
}
15701575

15711576
return &DataStoreAccess{
1572-
DataStores: dsMap,
1577+
DataStores: dsList,
15731578
}
15741579
}
15751580

1576-
func (ds *DataStoreAccess) GetDataStore(index int) *DataStore {
1577-
return ds.DataStores[index]
1581+
func (ds *DataStoreAccess) GetDataStore(networkCard int) *DataStore {
1582+
1583+
for index, datastore := range ds.DataStores {
1584+
if datastore.GetNetworkCard() == networkCard {
1585+
return ds.DataStores[index]
1586+
}
1587+
}
1588+
return nil
15781589
}
15791590

15801591
func (ds *DataStoreAccess) ReadAllDataStores(enableIPv6 bool) error {

‎pkg/ipamd/introspect.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -125,8 +125,8 @@ func (c *IPAMContext) setupIntrospectionServer() *http.Server {
125125
func eniV1RequestHandler(ipam *IPAMContext) func(http.ResponseWriter, *http.Request) {
126126
return func(w http.ResponseWriter, r *http.Request) {
127127
eniInfos := make(map[int]*datastore.ENIInfos, len(ipam.dataStoreAccess.DataStores))
128-
for networkCard, ds := range ipam.dataStoreAccess.DataStores {
129-
eniInfos[networkCard] = ds.GetENIInfos()
128+
for _, ds := range ipam.dataStoreAccess.DataStores {
129+
eniInfos[ds.GetNetworkCard()] = ds.GetENIInfos()
130130
}
131131
responseJSON, err := json.Marshal(eniInfos)
132132
if err != nil {

‎pkg/ipamd/ipamd.go

+11-8
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,6 @@ var (
207207
// IPAMContext contains node level control information
208208
type IPAMContext struct {
209209
awsClient awsutils.APIs
210-
dataStore []*datastore.DataStore
211210
dataStoreAccess *datastore.DataStoreAccess
212211
k8sClient client.Client
213212
enableIPv4 bool
@@ -940,7 +939,8 @@ func (c *IPAMContext) createSecondaryIPv6ENIs(ctx context.Context) error {
940939

941940
log.Info("Attaching ENIs allowed")
942941

943-
for networkCard, ds := range c.dataStoreAccess.DataStores {
942+
for _, ds := range c.dataStoreAccess.DataStores {
943+
networkCard := ds.GetNetworkCard()
944944
if networkCard == DefaultNetworkCardIndex {
945945
continue
946946
}
@@ -1503,7 +1503,8 @@ func (c *IPAMContext) nodeIPPoolReconcile(ctx context.Context, interval time.Dur
15031503
allManagedAndAttachedENIs := c.filterUnmanagedENIs(allENIs)
15041504
attachedENIsByNetworkCard := c.getENIsByNetworkCard(allManagedAndAttachedENIs)
15051505

1506-
for networkCard, ds := range c.dataStoreAccess.DataStores {
1506+
for _, ds := range c.dataStoreAccess.DataStores {
1507+
networkCard := ds.GetNetworkCard()
15071508
currentENIs := ds.GetENIInfos().ENIs
15081509
attachedENIs := attachedENIsByNetworkCard[networkCard]
15091510
trunkENI := ds.GetTrunkENI()
@@ -2204,7 +2205,7 @@ func (c *IPAMContext) AnnotatePod(podName string, podNamespace string, key strin
22042205
if ok {
22052206
log.Debugf("Existing annotation value: %s", oldVal)
22062207
if oldVal != releasedIP {
2207-
return fmt.Errorf("Released IP %s does not match existing annotation. Not patching pod.", releasedIP)
2208+
return fmt.Errorf("released IP %s does not match existing annotation. Not patching pod", releasedIP)
22082209
}
22092210
newPod.Annotations[key] = ""
22102211
}
@@ -2224,7 +2225,8 @@ func (c *IPAMContext) AnnotatePod(podName string, podNamespace string, key strin
22242225
func (c *IPAMContext) tryUnassignIPsFromENIs() {
22252226
log.Debugf("tryUnassignIPsFromENIs")
22262227
// From all datastores, get ENIInfos and unassign IPs
2227-
for networkCard, ds := range c.dataStoreAccess.DataStores {
2228+
for _, ds := range c.dataStoreAccess.DataStores {
2229+
networkCard := ds.GetNetworkCard()
22282230
eniInfos := ds.GetENIInfos()
22292231
for eniID := range eniInfos.ENIs {
22302232
c.tryUnassignIPFromENI(eniID, networkCard)
@@ -2269,7 +2271,8 @@ func (c *IPAMContext) tryUnassignPrefixesFromENIs() {
22692271
log.Debugf("tryUnassignPrefixesFromENIs")
22702272

22712273
// From all datastores get ENIs and remove prefixes
2272-
for networkCard, ds := range c.dataStoreAccess.DataStores {
2274+
for _, ds := range c.dataStoreAccess.DataStores {
2275+
networkCard := ds.GetNetworkCard()
22732276
eniInfos := ds.GetENIInfos()
22742277
for eniID := range eniInfos.ENIs {
22752278
c.tryUnassignPrefixFromENI(eniID, networkCard)
@@ -2366,8 +2369,8 @@ func (c *IPAMContext) isDatastorePoolTooLow() map[int]Decisions {
23662369
totalIPs = maxIpsPerPrefix
23672370
}
23682371

2369-
for networkCard, ds := range c.dataStoreAccess.DataStores {
2370-
2372+
for _, ds := range c.dataStoreAccess.DataStores {
2373+
networkCard := ds.GetNetworkCard()
23712374
stats := ds.GetIPStats(ipV4AddrFamily)
23722375
// If max pods has been reached, pool is not too low
23732376
if stats.TotalIPs >= c.maxPods {

‎pkg/ipamd/rpc_handler.go

+18-10
Original file line numberDiff line numberDiff line change
@@ -201,8 +201,8 @@ func (s *server) AddNetwork(ctx context.Context, in *rpc.AddNetworkRequest) (*rp
201201
return &failureResponse, nil
202202
}
203203

204-
for networkCard, ds := range s.ipamContext.dataStoreAccess.DataStores {
205-
204+
for _, ds := range s.ipamContext.dataStoreAccess.DataStores {
205+
networkCard := ds.GetNetworkCard()
206206
if ipsAllocated == ipsRequired {
207207
break
208208
}
@@ -246,14 +246,20 @@ func (s *server) AddNetwork(ctx context.Context, in *rpc.AddNetworkRequest) (*rp
246246
}
247247

248248
if errors != nil {
249-
for index := range ipAddrs {
250-
log.Infof("Unassigning IPs from datastore for Network Card %d as there was failure", index)
251-
// Try to unassign all the assigned IPs in case of errors.
252-
// This is best effort even if it fails we have the reconciler which can clean up leaked IPs for non existing pods
253-
s.ipamContext.dataStoreAccess.GetDataStore(index).UnassignPodIPAddress(ipamKey)
249+
unAssignedIPs := 0
250+
// Try to unassign all the assigned IPs in case of errors.
251+
// This is best effort even if it fails we have the reconciler which can clean up leaked IPs for non existing pods
252+
for _, ds := range s.ipamContext.dataStoreAccess.DataStores {
253+
networkCard := ds.GetNetworkCard()
254+
if unAssignedIPs == len(ipAddrs) {
255+
break
256+
}
257+
log.Infof("Unassigning IPs from datastore for Network Card %d as there was failure", networkCard)
258+
ds.UnassignPodIPAddress(ipamKey)
259+
unAssignedIPs += 1
254260
}
255261
} else {
256-
log.Infof("Address assigned from DS: %d", len(ipAddrs))
262+
log.Infof("Address assigned from datastores: %d", len(ipAddrs))
257263
var pbVPCV4cidrs, pbVPCV6cidrs []string
258264
var useExternalSNAT bool
259265

@@ -313,7 +319,7 @@ func (s *server) AddNetwork(ctx context.Context, in *rpc.AddNetworkRequest) (*rp
313319

314320
resp.Success = errors == nil
315321

316-
log.Infof("Send AddNetworkReply: Success: %t IPAddr: %+v, resp: %+v, err: %v", resp.Success, resp.IPAddress, resp, err)
322+
log.Infof("Send AddNetworkReply: Success: %t IPAddr: %+v, resp: %+v, err: %v", resp.Success, resp.IPAddress, &resp, err)
317323
return &resp, nil
318324
}
319325

@@ -346,7 +352,9 @@ func (s *server) DelNetwork(ctx context.Context, in *rpc.DelNetworkRequest) (*rp
346352

347353
ipsToMatch := 1
348354
ipsFound := 0
349-
for networkCard, ds := range s.ipamContext.dataStoreAccess.DataStores {
355+
for _, ds := range s.ipamContext.dataStoreAccess.DataStores {
356+
networkCard := ds.GetNetworkCard()
357+
350358
// All datastores will store this count in its datastore
351359
if ipsToMatch == ipsFound {
352360
break

0 commit comments

Comments
 (0)