Skip to content

schema, nm.sr-iov: extend SR-IOV VF configuration #648

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.

Sign up for GitHub

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

Jump to bottom
Merged
EdDev merged 2 commits into from
Jan 7, 2020
Merged

schema, nm.sr-iov: extend SR-IOV VF configuration #648

EdDev merged 2 commits into from
Jan 7, 2020

Conversation

@ffmancera
Copy link
Member

@ffmancera ffmancera commented Dec 13, 2019

This PR is going to extend SR-IOV in order to support VF configuration with nmstate.

@ffmancera
Copy link
Member Author

ffmancera commented Dec 13, 2019

I've pushed the schema proposal, I am working on the implementation but it would be nice to discuss the schema here. Thanks!

@ffmancera ffmancera added Wait_Review Work_Ongoing Not ready to be merged labels Dec 13, 2019
@ffmancera ffmancera changed the title schema: extend SR-IOV VF configuration schema, nm.sr-iov: extend SR-IOV VF configuration Dec 13, 2019
@coveralls
Copy link

coveralls commented Dec 13, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 87.146% when pulling 957d95b on ffmancera:sriov_extend into 4b99206 on nmstate:master.

cathay4t
cathay4t reviewed Dec 13, 2019
View reviewed changes
tyll
tyll reviewed Dec 13, 2019
View reviewed changes
tyll
tyll reviewed Dec 13, 2019
View reviewed changes
@ffmancera ffmancera force-pushed the sriov_extend branch 2 times, most recently from 03fabac to 0d57af7 Compare December 13, 2019 17:24
@ffmancera
Copy link
Member Author

ffmancera commented Dec 13, 2019

This patch have not been tested yet, I will try it out on Monday with special HW.

@lgtm-com
Copy link

lgtm-com bot commented Dec 13, 2019

This pull request introduces 1 alert when merging 0d57af7 into 055f39e - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@ffmancera ffmancera removed the Work_Ongoing Not ready to be merged label Dec 13, 2019
@lgtm-com
Copy link

lgtm-com bot commented Dec 17, 2019

This pull request introduces 3 alerts when merging 21cfb7e into 055f39e - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Suspicious unused loop iteration variable

@lgtm-com
Copy link

lgtm-com bot commented Dec 17, 2019

This pull request introduces 1 alert when merging 0b00285 into b306c3d - view on LGTM.com

new alerts:

  • 1 for Suspicious unused loop iteration variable

cathay4t
cathay4t requested changes Dec 18, 2019
View reviewed changes
@cathay4t cathay4t added Wait_on_submitter The submitter needs to do something before this can move on and removed Wait_Review labels Dec 18, 2019
@EdDev EdDev mentioned this pull request Dec 19, 2019
Closed
@ffmancera ffmancera added the Work_Ongoing Not ready to be merged label Dec 19, 2019
@lgtm-com
Copy link

lgtm-com bot commented Dec 20, 2019

This pull request introduces 1 alert when merging ac8b0c4 into 3978a1c - view on LGTM.com

new alerts:

  • 1 for Suspicious unused loop iteration variable

@codecov
Copy link

codecov bot commented Dec 20, 2019 •
edited
Loading

Codecov Report

Merging #648 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #648   +/-   ##
=======================================
  Coverage   64.48%   64.48%           
=======================================
  Files          28       28           
  Lines        2264     2264           
=======================================
  Hits         1460     1460           
  Misses        804      804

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3085b5d...937188f. Read the comment docs.

@ffmancera ffmancera added Wait_Review and removed Wait_on_submitter The submitter needs to do something before this can move on labels Jan 2, 2020
EdDev
EdDev reviewed Jan 2, 2020
View reviewed changes
EdDev
EdDev reviewed Jan 2, 2020
View reviewed changes
@ffmancera ffmancera force-pushed the sriov_extend branch 2 times, most recently from 55176e8 to 957d95b Compare January 4, 2020 23:51
EdDev
EdDev reviewed Jan 5, 2020
View reviewed changes
Copy link
Member

@EdDev EdDev left a comment

Choose a reason for hiding this comment

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

Thank you, looks great.
Only two comments to update the commit message with additional info (for the future us).
(related to nm.sriov: extend SR-IOV implementation commit)

@EdDev EdDev added Wait_on_submitter The submitter needs to do something before this can move on and removed Wait_Review labels Jan 5, 2020
@ffmancera ffmancera added Wait_Review and removed Wait_on_submitter The submitter needs to do something before this can move on labels Jan 6, 2020
cathay4t
cathay4t previously requested changes Jan 6, 2020
View reviewed changes
@cathay4t cathay4t added Wait_on_submitter The submitter needs to do something before this can move on and removed Wait_Review labels Jan 6, 2020
@ffmancera ffmancera mentioned this pull request Jan 6, 2020
Merged
@ffmancera ffmancera added Wait_Other_PR and removed Wait_on_submitter The submitter needs to do something before this can move on labels Jan 6, 2020
EdDev
EdDev reviewed Jan 7, 2020
View reviewed changes
Copy link
Member

@EdDev EdDev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Currently, only full editation of VF/s is supports.

... is supported.

@ffmancera
schema: extend SR-IOV schema
ab158f8 
This patch extends the SR-IOV schema in order to support VF
configuration with nmstate.

Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
@ffmancera
nm.sriov: extend SR-IOV implementation
937188f 
This patch extends the SR-IOV implementation in order to support VF
configuration with nmstate. Currently, only full editation of VF/s is
supported. Any VF/s that are emitted from the desired state are therefore
implicitly initialized.

In addition, current implementation reports the VF/s config state (from
NM) and not the kernel state.

Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
@EdDev EdDev dismissed cathay4t’s stale review January 7, 2020 15:40

All the points have been addressed.

@EdDev
Copy link
Member

EdDev commented Jan 7, 2020

@cathay4t , seems all your comments have been addressed.
If I missed something, please let me and @ffmancera know and we will make sure it will be fixed.

@EdDev EdDev merged commit 66afd9f into nmstate:master Jan 7, 2020
@ffmancera ffmancera deleted the sriov_extend branch January 7, 2020 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tyll tyll tyll left review comments

@EdDev EdDev EdDev approved these changes

@cathay4t cathay4t cathay4t left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ffmancera @coveralls @EdDev @zshi-redhat @tyll @cathay4t