ipam/aws: update the assigned IPs from aws to instance map #37650

liyihuang · 2025-02-15T04:54:42Z

Extracting and returning the assigned IP addresses from aws
Adding a new method to update ENI IP addresses in the instances manager

Fix the possible race condition caused by async update from aws to instance map in issue #36428

liyihuang · 2025-02-15T04:59:48Z

/test

liyihuang · 2025-02-15T05:52:53Z

/test

liyihuang · 2025-02-15T05:57:20Z

/test

liyihuang · 2025-02-15T13:21:27Z

I had the commit to print out the result to validate the result(print out the IP address getting from AWS and print out the IP in the ENI from before and after using AddIPsToENI in instance map). I pushed it to the PR and check it in the CI. Then I can see the log showing that the caculation is correct(available=5) right after the IP address has been added to the ENI. this should solve the issue.

2025-02-15T06:18:16.838766821Z time="2025-02-15T06:18:16.834201931Z" level=debug msg="Recalculated needed addresses" available=0 capacity=15 instanceID=i-0e41d8fadd6be8813 name=ip-192-168-174-15.ca-central-1.compute.internal remainingInterfaces=3 resyncNeeded="0001-01-01 00:00:00 +0000 UTC" subsys=ipam toAlloc=8 toRelease=0 used=0 waitingForPoolMaintenance=true
2025-02-15T06:18:17.211128260Z time="2025-02-15T06:18:17.210130985Z" level=warning msg="get ip from aws" assignedIPs="[192.168.175.89 192.168.189.173 192.168.160.142 192.168.178.174 192.168.181.39]" interfaceID=eni-0ded708bc192d31c9 subsys=eni
2025-02-15T06:18:17.211150690Z time="2025-02-15T06:18:17.210178056Z" level=warning msg="before add ip to eni" assignedIPs="[]" interfaceID=eni-0ded708bc192d31c9 subsys=eni
2025-02-15T06:18:17.211155250Z time="2025-02-15T06:18:17.210199046Z" level=warning msg="after add ip to eni" assignedIPs="[192.168.175.89 192.168.189.173 192.168.160.142 192.168.178.174 192.168.181.39]" interfaceID=eni-0ded708bc192d31c9 subsys=eni
2025-02-15T06:18:17.211159980Z time="2025-02-15T06:18:17.210231866Z" level=debug msg="Setting resync needed" instanceID=i-0e41d8fadd6be8813 name=ip-192-168-174-15.ca-central-1.compute.internal subsys=ipam
2025-02-15T06:18:17.211164480Z time="2025-02-15T06:18:17.210257987Z" level=debug msg="Recalculated needed addresses" available=5 capacity=15 instanceID=i-0e41d8fadd6be8813 name=ip-192-168-174-15.ca-central-1.compute.internal remainingInterfaces=2 resyncNeeded="2025-02-15 06:18:17.210240626 +0000 UTC m=+5.905289604" subsys=ipam toAlloc=3 toRelease=0 used=0 waitingForPoolMaintenance=false

I guess we should do similiar thing for prefix delegation but I will raise another PR for it later once we agree this is the best way to resolve the issue

liyihuang · 2025-02-15T13:21:56Z

/test

liyihuang · 2025-02-15T14:48:20Z

/ci-ipsec-e2e

liyihuang · 2025-02-15T14:55:36Z

/ci-ipsec-upgrade

doniacld

I don't see anything wrong in this PR but since @gandro was involved in the discussion, it would be nice to have his review.

gandro · 2025-02-18T10:28:18Z

Apologies for the delay in review. Since (at least to my knowledge) we still don't understand the root cause of the stale data in the instances manager, I'm a bit worried that this papers over a real issue that might affect other API calls (e.g. attaching a new ENI) as well.

Edit: Discussed offline, apparently attaching a new ENI already uses this approach:

cilium/pkg/aws/eni/node.go

Lines 535 to 536 in e21d1e4

    
           // Add the information of the created ENI to the instances manager 
        
           n.manager.UpdateENI(n.node.InstanceID(), eni)

Having said that, if we do not find the real root cause, I'm happy to merge this as a workaround.

gandro

I did some more thinking about this approach, and I now agree that it is overall the right thing to do.

There is one potential race that I think needs to be fixed, but otherwise I think this will improve the situation of us miscalculating the number of available addresses.

pkg/aws/eni/instances.go

liyihuang · 2025-02-26T14:21:52Z

/test

gandro

Thanks!

gandro · 2025-02-26T15:16:37Z

Note that this needs a rebase for CI to be able to pass https://cilium.slack.com/archives/C7PE7V806/p1740452895682009

liyihuang · 2025-02-26T16:18:12Z

Note that this needs a rebase for CI to be able to pass https://cilium.slack.com/archives/C7PE7V806/p1740452895682009

Great. I'm waiting for this PR get merged so I can work on my orginal PR(#36922).

I will also have the follow up PR to remove the IP and dealing with PD. Shall I open other issues? or I should open the agianst #36428

liyihuang · 2025-02-26T16:21:04Z

/test

liyihuang · 2025-02-26T16:59:13Z

/ci-clustermesh

liyihuang · 2025-02-26T16:59:40Z

/ci-integration

liyihuang · 2025-02-26T18:13:07Z

/ci-awscni

pkg/aws/eni/instances.go

liyihuang · 2025-02-27T03:43:08Z

/test

liyihuang · 2025-02-27T03:45:06Z

I re-request a review since I have a new commit but I don't have the permission to remove the ready to merge label. Please don't merge until I get a new approval

gandro

Thanks! Remove logic looks good to me! One subjective non-blocking nit

pkg/aws/eni/instances.go

- Extracting and returning the assigned IP addresses - Adding new methods to update ENI IP addresses in the instances manager - Update the IP to ENI after getting the IP from aws Signed-off-by: Liyi Huang <liyi.huang@isovalent.com>

liyihuang · 2025-02-27T13:03:48Z

/test

this is the temporary fix for ENI mode to avoid the sync from AWS getting the stale data since we would like to see if this can fix the CI issue for cilium#36428 based on the discussion here cilium#36428 (comment) if so, we will change how alibaba and azure behavior like PR cilium#37650 and remove this instanceSync trigger sync from here completely. I have tested skip this trigger sync with draft PR cilium#40760 for a few times and it seems to be working. we hope we can test this for 2-4 weeks so we can know if the CI issue got fixed. if we dont see the CI issue again, we will have the follow up PR to change Azure and Alibaba cloud and remove this trigger. Signed-off-by: Liyi Huang <liyi.huang@isovalent.com>

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 15, 2025

liyihuang force-pushed the pr/liyihuang/update_ip_to_eni_directly branch from bd54d82 to 054974f Compare February 15, 2025 05:52

liyihuang force-pushed the pr/liyihuang/update_ip_to_eni_directly branch from 054974f to ad5066d Compare February 15, 2025 05:57

liyihuang force-pushed the pr/liyihuang/update_ip_to_eni_directly branch from ad5066d to dabe5cd Compare February 15, 2025 13:10

liyihuang marked this pull request as ready for review February 15, 2025 16:16

liyihuang requested a review from a team as a code owner February 15, 2025 16:16

liyihuang requested a review from doniacld February 15, 2025 16:16

liyihuang mentioned this pull request Feb 15, 2025

CI: Conformance EKS - no-errors-in-logs - Unable to assign additional IPs to interface, will create new interface #36428

Open

doniacld reviewed Feb 17, 2025

View reviewed changes

gandro requested changes Feb 25, 2025

View reviewed changes

pkg/aws/eni/instances.go Outdated Show resolved Hide resolved

pkg/aws/eni/instances.go Show resolved Hide resolved

gandro added release-note/bug This PR fixes an issue in a previous release of Cilium. area/eni Impacts ENI based IPAM. needs-backport/1.17 This PR / issue needs backporting to the v1.17 branch labels Feb 25, 2025

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 25, 2025

liyihuang force-pushed the pr/liyihuang/update_ip_to_eni_directly branch from dabe5cd to 4bd1599 Compare February 26, 2025 14:21

gandro approved these changes Feb 26, 2025

View reviewed changes

doniacld approved these changes Feb 26, 2025

View reviewed changes

liyihuang force-pushed the pr/liyihuang/update_ip_to_eni_directly branch from 4bd1599 to 3411e23 Compare February 26, 2025 16:19

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 26, 2025

liyihuang force-pushed the pr/liyihuang/update_ip_to_eni_directly branch from 3411e23 to e4b6f0b Compare February 27, 2025 03:28

liyihuang commented Feb 27, 2025

View reviewed changes

pkg/aws/eni/instances.go Show resolved Hide resolved

liyihuang requested a review from gandro February 27, 2025 03:42

gandro removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 27, 2025

gandro approved these changes Feb 27, 2025

View reviewed changes

pkg/aws/eni/instances.go Outdated Show resolved Hide resolved

liyihuang force-pushed the pr/liyihuang/update_ip_to_eni_directly branch from e4b6f0b to 0801cff Compare February 27, 2025 13:03

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 27, 2025

joestringer added this pull request to the merge queue Feb 27, 2025

Merged via the queue into cilium:main with commit 45326ef Feb 27, 2025
63 checks passed

liyihuang deleted the pr/liyihuang/update_ip_to_eni_directly branch March 6, 2025 11:54

rastislavs mentioned this pull request Mar 10, 2025

v1.17 Backports 2025-03-10 #38104

Merged

14 tasks

rastislavs added backport-pending/1.17 The backport for Cilium 1.17.x for this PR is in progress. and removed needs-backport/1.17 This PR / issue needs backporting to the v1.17 branch labels Mar 10, 2025

liyihuang mentioned this pull request Mar 11, 2025

teams: add liyihuang to aws reviewers cilium/community#217

Merged

github-actions bot added backport-done/1.17 The backport for Cilium 1.17.x for this PR is done. and removed backport-pending/1.17 The backport for Cilium 1.17.x for this PR is in progress. labels Mar 11, 2025

liyihuang mentioned this pull request Aug 13, 2025

fix: temporary fix for intermittent CI issue #41140

Closed

liyihuang mentioned this pull request Aug 19, 2025

pkg/aws: check ENI ips with AWS #41278

Open

ipam/aws: update the assigned IPs from aws to instance map #37650

ipam/aws: update the assigned IPs from aws to instance map #37650

Uh oh!

Conversation

liyihuang commented Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liyihuang commented Feb 15, 2025

Uh oh!

liyihuang commented Feb 15, 2025

Uh oh!

liyihuang commented Feb 15, 2025

Uh oh!

liyihuang commented Feb 15, 2025

Uh oh!

liyihuang commented Feb 15, 2025

Uh oh!

liyihuang commented Feb 15, 2025

Uh oh!

liyihuang commented Feb 15, 2025

Uh oh!

doniacld left a comment

Choose a reason for hiding this comment

Uh oh!

gandro commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gandro left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

liyihuang commented Feb 26, 2025

Uh oh!

gandro left a comment

Choose a reason for hiding this comment

Uh oh!

gandro commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liyihuang commented Feb 26, 2025

Uh oh!

liyihuang commented Feb 26, 2025

Uh oh!

liyihuang commented Feb 26, 2025

Uh oh!

liyihuang commented Feb 26, 2025

Uh oh!

liyihuang commented Feb 26, 2025

Uh oh!

Uh oh!

liyihuang commented Feb 27, 2025

Uh oh!

liyihuang commented Feb 27, 2025

Uh oh!

gandro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

liyihuang commented Feb 27, 2025

Uh oh!

Uh oh!

Uh oh!

liyihuang commented Feb 15, 2025 •

edited

Loading

gandro commented Feb 18, 2025 •

edited

Loading

gandro left a comment •

edited

Loading

gandro commented Feb 26, 2025 •

edited

Loading