Handle PCI paths more consistently #3003

dgibson · 2020-10-08T03:28:52Z

This addresses issue #3002

jodh-intel

Thanks @dgibson. This looks much better. A couple of suggestions inline. Also, any way to add some further unit tests for this change?

virtcontainers/qemu.go

virtcontainers/pkg/types/types.go

likebreath

@dgibson LGTM. Thanks for the diligent refactoring that clarifies the confusions of PCIAddr vs PCI path. As the agent side PR is landed, you should be able to rework the first commit and make the TravisCI happy.

virtcontainers/pkg/types/types.go

amshinde · 2020-10-09T06:07:43Z

Havent been able to spend time reviewing PR's related to device handling lately, but will like to take a closer look at the changes introduced here.
Will schedule some time tomorrow for a review.

dgibson · 2020-10-09T06:15:11Z

Havent been able to spend time reviewing PR's related to device handling lately, but will like to take a closer look at the changes introduced here.
Will schedule some time tomorrow for a review.

@amshinde maybe hold off for a bit, I'm looking at doing some reworks based on @jodh-intel's comments next week.

dgibson · 2020-10-15T05:35:23Z

/test-ubuntu
/test-vfio

dgibson · 2020-10-15T05:42:53Z

/test-vfio

dgibson · 2020-10-15T06:48:12Z

/test

dgibson · 2020-10-15T22:56:19Z

/test-vfio
/test-ubuntu

codecov · 2020-10-16T00:09:01Z

Codecov Report

Merging #3003 into master will increase coverage by 0.02%.
The diff coverage is 49.31%.

@@            Coverage Diff             @@
##           master    #3003      +/-   ##
==========================================
+ Coverage   50.24%   50.27%   +0.02%     
==========================================
  Files         119      120       +1     
  Lines       15794    15849      +55     
==========================================
+ Hits         7936     7968      +32     
- Misses       6776     6799      +23     
  Partials     1082     1082

dgibson · 2020-10-16T01:21:06Z

Reworking this completely, it now makes much more thorough use of typing.

jodh-intel · 2020-10-16T08:13:30Z

⌨️ 😄

jodh-intel

Thanks @dgibson. A few comments.

jodh-intel · 2020-10-16T08:03:15Z

virtcontainers/pkg/types/pcipath.go

+	return PciPath{path: slots}, nil
+}
+
+func MakePciPath(slots ...uint8) (PciPath, error) {


Why is this variadic? It seems we only ever pass two values...?

For now, but I have future plans that will change that. With the agent side changes, it will now accept and work with paths of an arbitrary number of elements.

Hmm. Conventionally PRs shouldn't do this as you are spreading a feature across multiple PRs which, is cause for potential confusion. Why not just make the function accept 2 params and then the follow-on PR (when written) can change this function to be a variadic since that change is part of the future work?

Hmm. (a) that seems unnecessarily bureaucratic. (b) I don't think the division you propose is actually conceptually sounder than this one.

What this series is doing - and the first patch in particular - is introducing a data structure which will handle general pci paths to replace the ad-hoc and limited form of pci paths we deal with now. At present we only use one special case of pci paths, but it doesn't seem sensible to artificially limit the data structure just because we don't need the general variant yet.

virtcontainers/pkg/types/pcipath.go

likebreath · 2020-10-16T21:09:53Z

/test-clh

likebreath

Thanks @dgibson. PR looks good to me. I will send follow-up PRs to remove special code path on handling virtio-blk device w/ cloud-hypervisor.

virtcontainers/pkg/types/pcipath.go

likebreath · 2020-10-20T02:26:03Z

Note: The failure on podman-clh CI is a known issue, and is irrelevant to the PR itself.

Update the agent code under vendor/, to pull in kata-containers/agent#855. This is a change to the grpc protocol files, but not an actual protocol change, since it's just renaming a field without changing the field number or structure. Shortlog: Archana Shinde (2): Merge pull request #826 from jodh-intel/actions-for-issue-backlog Merge pull request #830 from keloyang/resolv David Gibson (9): device: Index all devices in spec before updating them device: Check type as well as major:minor when looking up devices device: Rename pciDeviceMap in sandbox struct device: Simplify uevent matching in listenToUdevEvents() Merge pull request #849 from dgibson/bug848 device: Rename and clarify semantics of getDevicePCIAddress device: Introduce PciPath type, name things consistently device: Reorganize TestPciPathToSysfs device: Generalize PCI paths to any number of bridges Eric Ernst (2): release: Kata Containers 1.12.0-alpha1 Merge pull request #822 from egernst/1.12.0-alpha1-branch-bump James O. D. Hunt (6): Merge pull request #798 from bpradipt/hook-fix action: Require PR porting labels action: Add issue to project and move to "In progress" on linked PR Merge pull request #828 from jodh-intel/actions-require-pr-porting-labels action: Improve porting checks Merge pull request #836 from dgibson/bug834and835 Julio Montes (2): Merge pull request #844 from jodh-intel/action-improve-porting-checks Merge pull request #855 from dgibson/pcipath Pradipta Kr. Banerjee (1): oci: Fix running of OCI hooks Shukui Yang (1): network: Fix Could not create destination mount point: /etc/resolv.conf Signed-off-by: David Gibson <[email protected]>

dgibson · 2020-10-26T15:16:05Z

/test

dgibson · 2020-10-26T15:17:33Z

@jodh-intel I've reorganized how I structured the datatype in a way I think should address some of your comments. As a bonus it should be easier to extend for multifunction devices in future.

jodh-intel

Thanks @dgibson - just a query on the copyright but other than that...

lgtm

virtcontainers/pkg/types/pcipath.go

dgibson · 2020-10-27T10:17:38Z

/test

virtcontainers/pkg/types/pcipath.go

dgibson · 2020-10-27T14:20:17Z

Is the jenkins-metrics-ubuntu-18-04 failure a known problem? I can't see why it's failing from the logs.

virtcontainers/pkg/types/pcipath_test.go

devimc · 2020-10-27T14:29:05Z

restarting metrics-ci, there is a bug that is messing up docker

This is a dedicated data type for representing PCI paths, that is, PCI devices described by the slot numbers of the bridges we need to reach them. There are a number of places that uses strings with that structure for things. The plan is to use this data type to consolidate their handling. Signed-off-by: David Gibson <[email protected]>

There's nothing in there which can fail now, but I'm planning to add something, and it turns out it's pretty easy for the single caller to handle. Signed-off-by: David Gibson <[email protected]>

The "PCI address" returned by Endpoint::PciPath() isn't actually a PCI address (DDDD:BB:DD.F), but rather a PCI path. Rename and use the PciPath type to clean this up and the various parts of the network code connected to it. fixes #3002 Signed-off-by: David Gibson <[email protected]>

BlockDrive::PCIAddr doesn't actually store a PCI address (DDDD:BB:DD.F) but a PCI path. Use the PciPath type and rename things to make that clearer. TestHandleBlockVolume() previously used a bizarre value "0002:01" for the "PCI address" which was neither an actual PCI address, nor a PCI path. Update it to use a PCI path - the actual value appears not to matter in this test, as long as its consistent throughout. fixes #3002 Signed-off-by: David Gibson <[email protected]>

VhostUserDeviceAttrs::PCIAddr didn't actually store a PCI address (DDDD:BB:DD.F), but rather a PCI path. Use the PciPath type and rename things to make that clearer. TestHandleBlockVolume previously used the bizarre value "0001:01" which is neither a PCI address nor a PCI path for this value. Change it to a valid PCI path - it appears the actual value didn't matter for that test, as long as it was consistent. fixes #3002 Signed-off-by: David Gibson <[email protected]>

dgibson · 2020-10-27T14:34:44Z

/test

dgibson · 2020-10-28T03:28:13Z

/retest-vfio

dgibson · 2020-10-28T04:36:55Z

/retest-metrics

dgibson · 2020-10-28T04:45:02Z

/retest-ubuntu-qemu-metrics

devimc

thanks @dgibson - lgtm

virtcontainers/kata_agent.go

jodh-intel · 2020-11-04T17:21:25Z

Hi @dgibson - It looks like this hasn't been ported to 2.x (https://github.com/kata-containers/kata-containers/tree/2.0-dev/src/runtime) yet. Would you be able to look at doing that?

dgibson added wip Work in Progress (PR incomplete - needs more work or rework) 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 8, 2020

dgibson requested review from amshinde, likebreath and sboeuf October 8, 2020 03:28

dgibson self-assigned this Oct 8, 2020

jodh-intel reviewed Oct 8, 2020

View reviewed changes

virtcontainers/qemu.go Outdated Show resolved Hide resolved

virtcontainers/pkg/types/types.go Outdated Show resolved Hide resolved

likebreath approved these changes Oct 8, 2020

View reviewed changes

virtcontainers/pkg/types/types.go Outdated Show resolved Hide resolved

virtcontainers/pkg/types/types.go Outdated Show resolved Hide resolved

dgibson marked this pull request as draft October 15, 2020 04:40

dgibson removed the wip Work in Progress (PR incomplete - needs more work or rework) label Oct 15, 2020

dgibson marked this pull request as ready for review October 16, 2020 00:09

dgibson requested review from jodh-intel and likebreath October 16, 2020 00:10

dgibson linked an issue Oct 16, 2020 that may be closed by this pull request

PCI paths handling is confusing in the code #3002

Closed

jodh-intel reviewed Oct 16, 2020

View reviewed changes

likebreath approved these changes Oct 20, 2020

View reviewed changes

virtcontainers/pkg/types/pcipath.go Outdated Show resolved Hide resolved

virtcontainers/pkg/types/pcipath.go Outdated Show resolved Hide resolved

dgibson requested review from jodh-intel and likebreath October 26, 2020 15:17

jodh-intel approved these changes Oct 27, 2020

View reviewed changes

virtcontainers/pkg/types/pcipath.go Outdated Show resolved Hide resolved

virtcontainers/pkg/types/pcipath.go Outdated Show resolved Hide resolved

devimc reviewed Oct 27, 2020

View reviewed changes

virtcontainers/pkg/types/pcipath.go Show resolved Hide resolved

jodh-intel reviewed Oct 27, 2020

View reviewed changes

virtcontainers/pkg/types/pcipath_test.go Outdated Show resolved Hide resolved

dgibson added 5 commits October 28, 2020 01:34

network: Allow convertToInterface to fail

bfbfab3

There's nothing in there which can fail now, but I'm planning to add something, and it turns out it's pretty easy for the single caller to handle. Signed-off-by: David Gibson <[email protected]>

devimc mentioned this pull request Oct 27, 2020

[RFE] Switch to Q35 as the default machine type for QEMU for 2.1.0 kata-containers/kata-containers#1038

Closed

dgibson requested a review from devimc October 28, 2020 14:19

devimc approved these changes Oct 28, 2020

View reviewed changes

devimc merged commit 9380b6e into kata-containers:master Oct 28, 2020

amshinde reviewed Oct 28, 2020

View reviewed changes

virtcontainers/kata_agent.go Show resolved Hide resolved

dgibson mentioned this pull request Oct 29, 2020

listInterfaces() will fail to report errors from convertToInterfaces() #3032

Closed

dgibson mentioned this pull request Dec 11, 2020

Clean up PCI path handling kata-containers/kata-containers#1190

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle PCI paths more consistently #3003

Handle PCI paths more consistently #3003

dgibson commented Oct 8, 2020

jodh-intel left a comment

likebreath left a comment

amshinde commented Oct 9, 2020

dgibson commented Oct 9, 2020

dgibson commented Oct 15, 2020

dgibson commented Oct 15, 2020

dgibson commented Oct 15, 2020

dgibson commented Oct 15, 2020

codecov bot commented Oct 16, 2020 •

edited

Loading

dgibson commented Oct 16, 2020

jodh-intel commented Oct 16, 2020

jodh-intel left a comment

jodh-intel Oct 16, 2020

dgibson Oct 17, 2020

jodh-intel Oct 19, 2020

dgibson Oct 19, 2020

likebreath commented Oct 16, 2020

likebreath left a comment

likebreath commented Oct 20, 2020

dgibson commented Oct 26, 2020

dgibson commented Oct 26, 2020

jodh-intel left a comment

dgibson commented Oct 27, 2020

dgibson commented Oct 27, 2020

devimc commented Oct 27, 2020

dgibson commented Oct 27, 2020

dgibson commented Oct 28, 2020

dgibson commented Oct 28, 2020

dgibson commented Oct 28, 2020

devimc left a comment

jodh-intel commented Nov 4, 2020

Handle PCI paths more consistently #3003

Handle PCI paths more consistently #3003

Conversation

dgibson commented Oct 8, 2020

jodh-intel left a comment

Choose a reason for hiding this comment

likebreath left a comment

Choose a reason for hiding this comment

amshinde commented Oct 9, 2020

dgibson commented Oct 9, 2020

dgibson commented Oct 15, 2020

dgibson commented Oct 15, 2020

dgibson commented Oct 15, 2020

dgibson commented Oct 15, 2020

codecov bot commented Oct 16, 2020 • edited Loading

Codecov Report

dgibson commented Oct 16, 2020

jodh-intel commented Oct 16, 2020

jodh-intel left a comment

Choose a reason for hiding this comment

jodh-intel Oct 16, 2020

Choose a reason for hiding this comment

dgibson Oct 17, 2020

Choose a reason for hiding this comment

jodh-intel Oct 19, 2020

Choose a reason for hiding this comment

dgibson Oct 19, 2020

Choose a reason for hiding this comment

likebreath commented Oct 16, 2020

likebreath left a comment

Choose a reason for hiding this comment

likebreath commented Oct 20, 2020

dgibson commented Oct 26, 2020

dgibson commented Oct 26, 2020

jodh-intel left a comment

Choose a reason for hiding this comment

dgibson commented Oct 27, 2020

dgibson commented Oct 27, 2020

devimc commented Oct 27, 2020

dgibson commented Oct 27, 2020

dgibson commented Oct 28, 2020

dgibson commented Oct 28, 2020

dgibson commented Oct 28, 2020

devimc left a comment

Choose a reason for hiding this comment

jodh-intel commented Nov 4, 2020

codecov bot commented Oct 16, 2020 •

edited

Loading