Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

agent: Remove dev from pciDeviceMap when device is unplugged#730

Merged
devimc merged 1 commit intokata-containers:masterfrom
darfux:remove_dev_from_map_when_unplug
Mar 2, 2020
Merged

agent: Remove dev from pciDeviceMap when device is unplugged#730
devimc merged 1 commit intokata-containers:masterfrom
darfux:remove_dev_from_map_when_unplug

Conversation

@darfux
Copy link
Copy Markdown
Contributor

@darfux darfux commented Jan 30, 2020

Delete uEv.DevPath from pciDeviceMap when recieving remove uevent.

Fixes #729
Signed-off-by: Li Yuxuan liyuxuan04@baidu.com

@darfux darfux mentioned this pull request Jan 30, 2020
Merged
@darfux darfux force-pushed the remove_dev_from_map_when_unplug branch from 8c4fc51 to b3b9811 Compare January 30, 2020 05:49
@darfux darfux changed the title Remove dev from pciDeviceMap when device is unplugged agent: Remove dev from pciDeviceMap when device is unplugged Jan 30, 2020
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 30, 2020 •
edited
Loading

Codecov Report

Merging #730 into master will decrease coverage by 0.27%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #730      +/-   ##
==========================================
- Coverage   60.73%   60.45%   -0.28%     
==========================================
  Files          17       17              
  Lines        2544     2569      +25     
==========================================
+ Hits         1545     1553       +8     
- Misses        850      868      +18     
+ Partials      149      148       -1

@devimc
Copy link
Copy Markdown

devimc commented Jan 30, 2020

thanks @darfux

/test

@darfux darfux force-pushed the remove_dev_from_map_when_unplug branch from b3b9811 to 95c2953 Compare January 31, 2020 02:00
@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Jan 31, 2020

Added span.finish() before continue the loop. Sorry for missing it 😥

@devimc
Copy link
Copy Markdown

devimc commented Jan 31, 2020

hey @darfux don't worry 😄

/test

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Feb 6, 2020

Thanks @devimc , could you plz restart the failed cases? ubuntu-18-04-initrd is failed on rootfs.sh and I'm trying to find out why centos-8-q-35 is failed

@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Feb 6, 2020

@darfux - I've nudged the two failed CIs for rebuilds.

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Feb 6, 2020

Thanks @grahamwhaley ! ubuntu-18-04-initrd is ok now. And centos-8-q-35 was failed on docker build stage with Error: Failed to download metadata for repo 'AppStream'. So could you please restart it again?😂

@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Feb 6, 2020

done. kicked rebuild.

@devimc
Copy link
Copy Markdown

devimc commented Feb 6, 2020

one more review here please

@devimc
Copy link
Copy Markdown

devimc commented Feb 7, 2020

ping @jodh-intel @amshinde @grahamwhaley

grahamwhaley
grahamwhaley reviewed Feb 7, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

l g t m
would like @jodh-intel to check the span code of course ;-)

@amshinde
Copy link
Copy Markdown
Member

amshinde commented Feb 7, 2020

same comment as @grahamwhaley. Would like @jodh-intel to take a look at the span handling before this is merged.

@amshinde amshinde added the do-not-merge PR has problems or depends on another label Feb 7, 2020
@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Feb 20, 2020

Thanks for reviewing! Hi @jodh-intel could you plz take a look at the span handling? 😁

jodh-intel
jodh-intel approved these changes Feb 25, 2020
View reviewed changes
Copy link
Copy Markdown

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks @darfux.

lgtm

The spans seem fine although an alternative would be to do something like:

diff --git a/agent.go b/agent.go
index d49fb70..7823722 100644
--- a/agent.go
+++ b/agent.go
@@ -730,13 +730,11 @@ func (s *sandbox) listenToUdevEvents() {
                        s.Lock()
                        delete(s.pciDeviceMap, uEv.DevPath)
                        s.Unlock()
-                       span.finish()
-                       continue
+                       goto FINISH_SPAN
                }

                if uEv.Action != "add" {
-                       span.finish()
-                       continue
+                       goto FINISH_SPAN
                }

                fieldLogger.Infof("Received add uevent")
@@ -792,6 +790,7 @@ func (s *sandbox) listenToUdevEvents() {
                        }
                }

+       FINISH_SPAN:
                span.finish()
        }
 }

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Feb 25, 2020

Merging blocked due to the dnm label still - can this be removed now?

@darfux
agent: Remove dev from pciDeviceMap when device is unplugged
3f0d98e 
Delete `uEv.DevPath` from pciDeviceMap when recieving `remove` uevent.

Fixes kata-containers#729
Signed-off-by: Li Yuxuan <liyuxuan04@baidu.com>
@darfux darfux force-pushed the remove_dev_from_map_when_unplug branch from 95c2953 to 3f0d98e Compare February 27, 2020 11:22
@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Feb 27, 2020

Thanks @jodh-intel. I've applied your suggestion
/cc @amshinde for the DNM label :)

@jodh-intel
Copy link
Copy Markdown

jodh-intel commented Feb 27, 2020

Thanks @darfux.

@grahamwhaley grahamwhaley removed the do-not-merge PR has problems or depends on another label Feb 28, 2020
@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Feb 28, 2020

/test
and DNM dropped.

@darfux
Copy link
Copy Markdown
Contributor Author

darfux commented Mar 2, 2020

Thanks @grahamwhaley . The ubuntu-18-04-initrd case was failed in Spec Setup which is similar with http://jenkins.katacontainers.io/job/kata-containers-agent-ubuntu-18-04-PR-initrd/582. Could you plz restart it 😆

@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Mar 2, 2020

np, scheduled....

@devimc devimc merged commit 56190a9 into kata-containers:master Mar 2, 2020
@darfux darfux deleted the remove_dev_from_map_when_unplug branch March 4, 2020 01:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@grahamwhaley grahamwhaley grahamwhaley left review comments

@amshinde amshinde amshinde approved these changes

@devimc devimc devimc approved these changes

@jodh-intel jodh-intel jodh-intel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove dev from pciDeviceMap when device is unplugged

5 participants

@darfux @devimc @grahamwhaley @amshinde @jodh-intel