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.

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

Delete Pod specific VF resource cache from Podwatch controller, when a Pod gets deleted. #4285

Conversation

arunvelayutham
Copy link
Contributor

When a Pod gets deleted and recreated, secondary network configuration fails due to misconfigured VF resource cache. Pod specific VF resource must be cleared, when a Pod gets deleted everything.

NOTE: Upon Pod creation, Podwatch controller will build a VF resource cache, if the Pod annotates to configure SRIOV based secondary network interfaces.

Signed-off-by: Arunkumar Velayutham arunkumar.velayutham@intel.com

@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Merging #4285 (64b2744) into main (ac70351) will increase coverage by 1.13%.
The diff coverage is 100.00%.

❗ Current head 64b2744 differs from pull request most recent head fd2d8c8. Consider uploading reports for the commit fd2d8c8 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4285      +/-   ##
==========================================
+ Coverage   63.16%   64.29%   +1.13%     
==========================================
  Files         388      393       +5     
  Lines       55147    55548     +401     
==========================================
+ Hits        34833    35714     +881     
+ Misses      17766    17224     -542     
- Partials     2548     2610      +62     
Flag Coverage Δ
integration-tests 34.58% <ø> (+0.15%) ⬆️
kind-e2e-tests 48.47% <60.00%> (-0.08%) ⬇️
unit-tests 48.16% <100.00%> (+3.27%) ⬆️
Impacted Files Coverage Δ
pkg/agent/secondarynetwork/podwatch/controller.go 75.68% <100.00%> (+0.86%) ⬆️
pkg/agent/cniserver/ipam/ipam_service.go 76.40% <0.00%> (-12.36%) ⬇️
pkg/agent/controller/networkpolicy/reject.go 73.39% <0.00%> (-7.11%) ⬇️
pkg/controller/externalippool/controller.go 86.16% <0.00%> (-6.25%) ⬇️
...rs/multicluster/commonarea/clusterinfo_importer.go 69.64% <0.00%> (-4.87%) ⬇️
pkg/agent/consistenthash/consistenthash.go 93.75% <0.00%> (-3.75%) ⬇️
pkg/controller/networkpolicy/store/addressgroup.go 88.37% <0.00%> (-3.49%) ⬇️
pkg/controller/ipam/antrea_ipam_controller.go 75.25% <0.00%> (-3.02%) ⬇️
...lticluster/commonarea/resourceimport_controller.go 78.41% <0.00%> (-2.77%) ⬇️
pkg/agent/ipassigner/local_ip_detector_linux.go 75.75% <0.00%> (-2.03%) ⬇️
... and 71 more

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

given that this fixes a bug, the PR should also add a unit test to cover this scenario

podKey := podNamespace + "/" + podName
_, cacheFound := pc.vfDeviceIDUsageMap.Load(podKey)
if cacheFound {
klog.InfoS("Pod specific VF Cache cleared for ", "pod", podKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be

klog.InfoS("Pod specific VF cache cleared", "key", podKey)

But if you want to have this log message, I think the verbosity needs to be changed (e.g., to V(2)). It is also surprising that we have a log message for the deletion case but not when the cache entry is created or modified.

@@ -290,6 +300,8 @@ func (pc *PodController) handleRemovePod(key string) error {
} else {
// Delete cache entry from podCNIInfo.
pc.podCache.DeleteCNIConfigInfo(containerInfo)
// Delete Pod specific VF cache (if one exist)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/exist/exists

@jianjuns jianjuns self-requested a review October 21, 2022 00:34
@arunvelayutham arunvelayutham force-pushed the fix-to-clear-pod-vf-cache-upon-pod-delete branch from 44cc920 to 64b2744 Compare October 21, 2022 21:53
Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

@arunvelayutham : I saw irrelevant files in your comments (a.log, a.txt, etc.). Could you please fix?

@arunvelayutham arunvelayutham force-pushed the fix-to-clear-pod-vf-cache-upon-pod-delete branch from 64b2744 to 8afc1fe Compare October 24, 2022 21:31
@arunvelayutham
Copy link
Contributor Author

@jianjuns looks like a test commit got accidently push (while I was addressing the git issue last week). fix it now.

…atch controller

Signed-off-by: Arunkumar Velayutham <arunkumar.velayutham@intel.com>
@arunvelayutham arunvelayutham force-pushed the fix-to-clear-pod-vf-cache-upon-pod-delete branch 2 times, most recently from 5ae73fa to fd2d8c8 Compare October 25, 2022 18:01
@@ -290,6 +301,8 @@ func (pc *PodController) handleRemovePod(key string) error {
} else {
// Delete cache entry from podCNIInfo.
pc.podCache.DeleteCNIConfigInfo(containerInfo)
// Delete Pod specific VF cache (if one exists)
pc.deleteVFDeviceIDListPerPod(containerInfo.PodName, containerInfo.PodNameSpace)
Copy link
Contributor

Choose a reason for hiding this comment

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

Question - could we pass key argument of handleRemovePod() to deleteVFDeviceIDListPerPod()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, key from the function argument can be used as well.

@jianjuns
Copy link
Contributor

/test-e2e

@jianjuns
Copy link
Contributor

/test-networkpolicy

@jianjuns
Copy link
Contributor

/test-conformance

@jianjuns jianjuns merged commit 071df98 into antrea-io:main Oct 27, 2022
GraysonWu pushed a commit to GraysonWu/antrea that referenced this pull request Jan 27, 2023
Upon a CNF Pod delete, clear Pod VF cache in the secondary network's podwatch controller.

Signed-off-by: Arunkumar Velayutham <arunkumar.velayutham@intel.com>
heanlan pushed a commit to heanlan/antrea that referenced this pull request Mar 29, 2023
Upon a CNF Pod delete, clear Pod VF cache in the secondary network's podwatch controller.

Signed-off-by: Arunkumar Velayutham <arunkumar.velayutham@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants