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

Handle PCI paths consistently and more generally #855

Merged
merged 4 commits into from
Oct 8, 2020
Merged

Handle PCI paths consistently and more generally #855

merged 4 commits into from
Oct 8, 2020

Conversation

dgibson
Copy link
Contributor

@dgibson dgibson commented Oct 7, 2020

This series is designed to change the agent to handle PCI paths more consistently, see issue #854 for more information.

@dgibson dgibson added do-not-merge PR has problems or depends on another needs-forward-port Changes need to be applied to a newer branch / repository no-backport-needed Changed do not need to be applied to an older branch / repository labels Oct 7, 2020
@dgibson dgibson self-assigned this Oct 7, 2020
@dgibson
Copy link
Contributor Author

dgibson commented Oct 7, 2020

/test-vfio

2 similar comments
@dgibson
Copy link
Contributor Author

dgibson commented Oct 7, 2020

/test-vfio

@dgibson
Copy link
Contributor Author

dgibson commented Oct 7, 2020

/test-vfio

c3d
c3d approved these changes Oct 7, 2020
View reviewed changes
Copy link
Member

@c3d c3d left a comment

Choose a reason for hiding this comment

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

Looks good to me. Two questions, just to educate myself.

device.go Outdated Show resolved Hide resolved
device.go Outdated Show resolved Hide resolved
@dgibson dgibson added the wip Work in Progress (PR incomplete - needs more work or rework) label Oct 7, 2020
@dgibson
Copy link
Contributor Author

dgibson commented Oct 7, 2020

/test-vfio

2 similar comments
@dgibson
Copy link
Contributor Author

dgibson commented Oct 7, 2020

/test-vfio

@dgibson
Copy link
Contributor Author

dgibson commented Oct 7, 2020

/test-vfio

@dgibson dgibson changed the title Handle PCI paths consistently Handle PCI paths consistently and more generally Oct 7, 2020
@dgibson dgibson requested a review from sboeuf October 7, 2020 12:17
@dgibson dgibson marked this pull request as draft October 7, 2020 12:17
@dgibson
Copy link
Contributor Author

dgibson commented Oct 7, 2020

/test-vfio

1 similar comment
@dgibson
Copy link
Contributor Author

dgibson commented Oct 7, 2020

/test-vfio

@dgibson dgibson marked this pull request as ready for review October 7, 2020 12:28
@dgibson dgibson added wip Work in Progress (PR incomplete - needs more work or rework) and removed do-not-merge PR has problems or depends on another wip Work in Progress (PR incomplete - needs more work or rework) labels Oct 7, 2020
@dgibson
Copy link
Contributor Author

dgibson commented Oct 7, 2020

/test-vfio

@devimc
Copy link

devimc commented Oct 7, 2020

thanks @dgibson - lgtm but travis is failing

Found 3 commits between commit f2db939b620618987d350f941a0c8d13941c7db1 and branch master

ERROR: No "Fixes" found

ERROR: checkcommits failed. See the document below for help on formatting

commits for the project.

@dgibson dgibson requested a review from devimc October 7, 2020 14:27
@dgibson
Copy link
Contributor Author

dgibson commented Oct 7, 2020

/retest-vfio

@devimc
Copy link

devimc commented Oct 7, 2020

now a different error 🤓

/home/travis/gopath/src/github.com/kata-containers/agent/protocols/mockserver

device.go:45: File is not `gofmt`-ed with `-s` (gofmt)
	sysBusPrefix        = sysfsDir + "/bus/pci/devices"
	pciBusRescanFile    = sysfsDir + "/bus/pci/rescan"
	pciBusPathFormat    = "%s/%s/pci_bus/"
	systemDevPath       = "/dev"
	getSCSIDevPath      = getSCSIDevPathImpl
	getPmemDevPath      = getPmemDevPathImpl
	getPCIDeviceName    = getPCIDeviceNameImpl
	pciPathToSysfs      = pciPathToSysfsImpl
	scanSCSIBus         = scanSCSIBusImpl

@dgibson
device: Rename and clarify semantics of getDevicePCIAddress
0eb612f 
getDevicePCIAddress() has pretty confusing semantics.  Both its input
and output are in other parts of the code described as a "PCI
address", but neither is *actually* a PCI address (in the standard
DDDD:BB:DD.F format).

What it's really about is resolving a "PCI path" - that is way to
locate a PCI device by using it's slot number and the slot number of
the bridge leading to it - into a sysfs path.

Rename the function, and change a bunch of variable names to make
those semantics clearer.

Signed-off-by: David Gibson <[email protected]>
@dgibson
Copy link
Contributor Author

dgibson commented Oct 7, 2020

/test-vfio

@dgibson
device: Introduce PciPath type, name things consistently
da4bc1d 
pciPathToSysfs takes a PCI path, with a particular format.  A number
of places implicitly need strings in that format, many of them repeat
the description.  To make things safer and briefer create a newtype
wrapper for strings in this format, and just describe the internals at
the type definition.

Then, update variable names and comments throughout to call things in
this format "PCI path", rather than "PCI identifier", which is vague
or "PCI address" which is just plain wrong.  Likewise we change names and
comments which incorrectly refer to sysfs paths as a "PCI address".

This changes the grpc proto definitions, but because it's just
changing the name of a field without changing the field number, it
shouldn't change the actual protocol.  It also trivially changes the
copyright notice, because Travis whinges otherwise.

Signed-off-by: David Gibson <[email protected]>
@dgibson
device: Reorganize TestPciPathToSysfs
134f55a 
This does some general reorganization of TestPciPathToSysfs.  It tests the
same things (plus a few trivial extras), but is set out in a way that will
be easier to extend when we broaden the allowed pciPath values we accept.

It also removes some intermediate variables that seemed to make things
harder rather than easier to follow.

Signed-off-by: David Gibson <[email protected]>
@dgibson
device: Generalize PCI paths to any number of bridges
9804b1e 
Currently pciPathToSysfsImpl, which translates PCI paths into sysfs paths
accepts only pci paths with exactly 2 components; which represents PCI
devices separated from the root bus by exactly one PCI to PCI bridge (which
could be a virtual P2P bridge, such as a PCI-E root port).

There are cases we might reasonably want to support which have devices
either plugged directly into the root bus (zero bridges), or under
multiple layers of P2P bridge (a PCI-E switch would require at least 2
layers).

So, generalize pciPathToSysfs to support any number of components in the
PCI path.

We need to adjust some tests to match, not only because of the intended
change in behaviour, but also because the new version probes sysfs a bit
differently, so we need to mock things to match.

fixes #854

Signed-off-by: David Gibson <[email protected]>
@codecov
Copy link

codecov bot commented Oct 8, 2020 •
edited
Loading

Codecov Report

Merging #855 into master will decrease coverage by 0.05%.
The diff coverage is 78.26%.

@@            Coverage Diff             @@
##           master     #855      +/-   ##
==========================================
- Coverage   57.85%   57.80%   -0.06%     
==========================================
  Files          17       17              
  Lines        2373     2370       -3     
==========================================
- Hits         1373     1370       -3     
  Misses        839      839              
  Partials      161      161

@dgibson
Copy link
Contributor Author

dgibson commented Oct 8, 2020

/test-vfio
/test-ubuntu

@dgibson dgibson requested a review from c3d October 8, 2020 03:00
@dgibson dgibson mentioned this pull request Oct 8, 2020
Closed
@dgibson dgibson removed the wip Work in Progress (PR incomplete - needs more work or rework) label Oct 8, 2020
@dgibson dgibson mentioned this pull request Oct 8, 2020
Closed
@dgibson
Copy link
Contributor Author

dgibson commented Oct 8, 2020

/test

@jodh-intel
Copy link
Contributor

Restarted Debian CI which timed out.

Also restarted the CentOS one but I think we may have seen this issue before @GabyCT?

bats integration/netmon/netmon_test.bats
1..1
not ok 1 test netmon
# (in test file integration/netmon/netmon_test.bats, line 60)
#   `[ "$after_interfaces" -gt "$before_interfaces" ]' failed
# 
# e6153b744972f86bffc5573fdc0f2a52eea49ebdcb78510d3599aa5e13ffcfb6
# 0ee6e23ddfe3a27d0cea0d80768de2d882bfe5bd5ca92f940b62a9fbfce42ef7
# containerA
# containerA
# test
#

devimc
devimc approved these changes Oct 8, 2020
View reviewed changes
Copy link

@devimc devimc left a comment

Choose a reason for hiding this comment

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

thanks @dgibson

@devimc devimc merged commit f9eab0f into kata-containers:master Oct 8, 2020
@dgibson dgibson deleted the pcipath branch October 8, 2020 23:18
@dgibson dgibson mentioned this pull request Oct 9, 2020
Closed
@dgibson dgibson mentioned this pull request Dec 11, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@devimc devimc devimc approved these changes

@likebreath likebreath Awaiting requested review from likebreath

@sboeuf sboeuf Awaiting requested review from sboeuf

@amshinde amshinde Awaiting requested review from amshinde

@c3d c3d Awaiting requested review from c3d

Assignees

@dgibson dgibson

Labels
needs-forward-port Changes need to be applied to a newer branch / repository no-backport-needed Changed do not need to be applied to an older branch / repository
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dgibson @devimc @jodh-intel @c3d