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

VDPA enhancement #1198

Merged
merged 7 commits into from
Oct 19, 2022
Merged

VDPA enhancement #1198

merged 7 commits into from
Oct 19, 2022

Conversation

lmilleri
Copy link
Contributor

This enhancement provides a proposal for integrating vDPA (Virtio Data Path Acceleration) in Openshift.

@openshift-ci openshift-ci bot requested review from mandre and tjungblu July 26, 2022 14:51
@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 26, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 26, 2022

Hi @lmilleri. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@zshi-redhat
Copy link
Contributor

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 27, 2022
Signed-off-by: Leonardo Milleri <lmilleri@redhat.com>
…ment)

Signed-off-by: Leonardo Milleri <lmilleri@redhat.com>
Signed-off-by: Leonardo Milleri <lmilleri@redhat.com>
@lmilleri
Copy link
Contributor Author

@tjungblu tjungblu removed their request for review July 27, 2022 10:09
@dhellmann
Copy link
Contributor

/test markdownlint

@dhellmann
Copy link
Contributor

Those linter errors are real.

Summary: 17 error(s)
enhancements/vdpa/overview.md:26:401 MD013/line-length Line length [Expected: 400; Actual: 475]
enhancements/vdpa/overview.md:57:401 MD013/line-length Line length [Expected: 400; Actual: 540]
enhancements/vdpa/overview.md:63:401 MD013/line-length Line length [Expected: 400; Actual: 464]
enhancements/vdpa/overview.md:65:401 MD013/line-length Line length [Expected: 400; Actual: 599]
enhancements/vdpa/overview.md:71:401 MD013/line-length Line length [Expected: 400; Actual: 823]
enhancements/vdpa/vdpa-introduction.md:76:1 MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 1]
enhancements/vdpa/vdpa-introduction.md:77:1 MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 1]
enhancements/vdpa/vdpa-introduction.md:78:1 MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 1]
enhancements/vdpa/vdpa-introduction.md:79:1 MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 1]
enhancements/vdpa/vdpa-introduction.md:80:1 MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 1]
enhancements/vdpa/vdpa-introduction.md:81:1 MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 1]
enhancements/vdpa/vdpa-introduction.md:82:1 MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 1]
enhancements/vdpa/vdpa-introduction.md:83:1 MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 1]
enhancements/vdpa/vdpa-introduction.md:84:1 MD007/ul-indent Unordered list indentation [Expected: 0; Actual: 1]
enhancements/vdpa/vdpa-introduction.md:85:1 MD005/list-indent Inconsistent indentation for list items at the same level [Expected: 1; Actual: 0]
enhancements/vdpa/vdpa-introduction.md:86:1 MD005/list-indent Inconsistent indentation for list items at the same level [Expected: 1; Actual: 0]
enhancements/vdpa/vdpa-introduction.md:87:1 MD005/list-indent Inconsistent indentation for list items at the same level [Expected: 1; Actual: 0]

@lmilleri
Copy link
Contributor Author

/test markdownlint

@lmilleri
Copy link
Contributor Author

@dhellmann Can you please advise how to fix the remaining lint issues?

@dhellmann
Copy link
Contributor

@dhellmann Can you please advise how to fix the remaining lint issues?

The issues are because of how the documents are structured. It looks like you're setting up vdpa as a new domain, which is fine. Within a domain there are some expectations about how the files are named so that the filename and title inside the document match.

For example, you could rename enhancements/vdpa/overview.md to enhancements/vdpa/vdpa-support overview.md and then set the title to VDPA Support Overview in the document and then rename enhancements/vdpa/vdpa-introduction.md to enhancements/vdpa/virtio-with-ovn-hw-offloading.md and change the title to Virtio with OVN HW offloading. Those are examples, you could use different names and titles if you prefer. But the filename and title need to match so that people who remember the title but not the URL can find the document through the GitHub UI.

The other errors about the tracking links are because the metadata for each enhancement needs to link to a Jira or Bugzilla ticket. See the top of the template file for the tracking-link field. I also see that you've added a status field to the header with provisional. We don't use that field any more for enhancements. Every enhancement needs to be linked to work we've committed to doing. Speculative proposals can be shared in other forms, including as PRs here marked WIP. See the top level readme in this repo for details. If you have a Jira ticket that's scheduled for work and tied to these documents, set the tracking-link and remove the status field and you'll be all set.

Signed-off-by: Leonardo Milleri <lmilleri@redhat.com>
enhancements/vdpa/vdpa-support-overview.md Outdated Show resolved Hide resolved
enhancements/vdpa/virtio-vdpa-ovn-hw-offload.md Outdated Show resolved Hide resolved
enhancements/vdpa/virtio-vdpa-ovn-hw-offload.md Outdated Show resolved Hide resolved
enhancements/vdpa/virtio-vdpa-ovn-hw-offload.md Outdated Show resolved Hide resolved
enhancements/vdpa/virtio-vdpa-ovn-hw-offload.md Outdated Show resolved Hide resolved
- minor changes
- architecture diagram (step 4 in step 1, OVS and port reps)

Signed-off-by: Leonardo Milleri <lmilleri@redhat.com>
enhancements/vdpa/virtio-vdpa-ovn-hw-offload.md Outdated Show resolved Hide resolved
enhancements/vdpa/virtio-vdpa-ovn-hw-offload.md Outdated Show resolved Hide resolved
enhancements/vdpa/virtio-vdpa-ovn-hw-offload.md Outdated Show resolved Hide resolved
enhancements/vdpa/virtio-vdpa-ovn-hw-offload.md Outdated Show resolved Hide resolved
Signed-off-by: Leonardo Milleri <lmilleri@redhat.com>
@zshi-redhat
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2022
@zshi-redhat
Copy link
Contributor

@lmilleri one more question: would it make sense to move the vdpa directory under enhancements/network?

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2022
@lmilleri
Copy link
Contributor Author

@zshi-redhat moved vdpa to enhancements/network as suggested

@zshi-redhat
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2022
@zshi-redhat
Copy link
Contributor

@dcbw @dougbtv Could you review and approve if it looks good to you?

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2022
- API change preferred option
- removed unneeded sections
- minor changes
- vdpa moved to enhancement/network directory

Signed-off-by: Leonardo Milleri <lmilleri@redhat.com>
@wizhaoredhat
Copy link

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2022
@lmilleri
Copy link
Contributor Author

@dcbw @dougbtv Could you please review and approve if it looks good to you?

@dougbtv
Copy link
Member

dougbtv commented Oct 19, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 19, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dougbtv

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 19, 2022

@lmilleri: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit cf899e9 into openshift:master Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants