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.

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

[BUG] SR-IOV Virtual Functions can be 0 or -N integers #3977

Closed
irishgordo opened this issue May 25, 2023 · 8 comments
Closed

[BUG] SR-IOV Virtual Functions can be 0 or -N integers #3977

irishgordo opened this issue May 25, 2023 · 8 comments
Assignees
@irishgordo @torchiaf
Labels
area/rancher Rancher related issues area/ui kind/bug Issues that are defects reported by users or that we know have reached a real release not-require/test-plan Skip to create a e2e automation test issue priority/1 Highly recommended to fix in this release priority/2 Nice to fix in this release reproduce/always Reproducible 100% of the time severity/4 Function working but has a minor issue (a minor incident with low impact)
Milestone
v1.2.2

Comments

@irishgordo
Copy link

Describe the bug
Can pass in 0 or negative numbers for enabling virtual functions on sr-iov.

To Reproduce
Steps to reproduce the behavior:

  1. Go to addons, enable pci devices
  2. Goto SR-IOV devices, enable, pass in 0
  3. Disable
  4. Enable, pass in -230

Expected behavior
Not allow negative integers or 0 when specifiying the number of virtual functions on sr-iov

Environment

  • Harvester ISO version: v1.2.0-rc1
  • Underlying Infrastructure: 2 node bare metal

Additional context

Screenshot from 2023-05-25 16-10-05
@irishgordo irishgordo added kind/bug Issues that are defects reported by users or that we know have reached a real release severity/4 Function working but has a minor issue (a minor incident with low impact) reproduce/always Reproducible 100% of the time area/ui labels May 25, 2023
irishgordo pushed a commit to irishgordo/dashboard that referenced this issue May 25, 2023
fix: Toss Out Invalid Amount On VFs For SR-IOV
8b53e19 
* toss out anything less than or equal to zero for the SR-IOV device
  virtual function addresses
* avoid making an API call out to the endpoint, reducing network chatter

Resolves: github.com/harvester/harvester/issues/3977
@irishgordo irishgordo mentioned this issue May 25, 2023
Merged
3 tasks
irishgordo pushed a commit to irishgordo/dashboard that referenced this issue May 26, 2023
fix: Toss Out Invalid Amount On VFs For SR-IOV
308ded9 
* toss out anything less than or equal to zero for the SR-IOV device
  virtual function addresses
* avoid making an API call out to the endpoint, reducing network chatter

Resolves: github.com/harvester/harvester/issues/3977
@irishgordo
Copy link
Author

Not sure if the drafted pr from above fixes what's happening 😅 - but was thinking perhaps something along those lines for the implementation of the fix 😄 - if the fix might work I can un-draft that PR 😄

cc: @DaiYuzeng

@irishgordo irishgordo mentioned this issue May 26, 2023
Closed
@DaiYuzeng DaiYuzeng mentioned this issue May 30, 2023
Merged
3 tasks
@DaiYuzeng
Copy link

DaiYuzeng commented May 30, 2023 •
edited

Thanks for your dedication! I sent a PR to your branch. Could you take a look when you are free? Thanks!

@irishgordo
Copy link
Author

Looks great 😄 - I've gone ahead and merged that in 👍
Thanks for the PR addition

DaiYuzeng pushed a commit to harvester/dashboard that referenced this issue May 30, 2023
@DaiYuzeng
fix: Toss Out Invalid Amount On VFs For SR-IOV
a8e8ac7 
* toss out anything less than or equal to zero for the SR-IOV device
  virtual function addresses
* avoid making an API call out to the endpoint, reducing network chatter

Resolves: github.com/harvester/harvester/issues/3977
WuJun2016 pushed a commit to WuJun2016/dashboard that referenced this issue Jun 12, 2023
@WuJun2016
fix: Toss Out Invalid Amount On VFs For SR-IOV
123f2e2 
* toss out anything less than or equal to zero for the SR-IOV device
  virtual function addresses
* avoid making an API call out to the endpoint, reducing network chatter

Resolves: github.com/harvester/harvester/issues/3977
@WuJun2016 WuJun2016 added this to the v1.2.1 milestone Jul 12, 2023
@WuJun2016
Copy link
Contributor

@DaiYuzeng We need to raise a pr upstream (component or vue directive) to solve the problem of inputting negative numbers.

@WuJun2016 WuJun2016 added area/rancher Rancher related issues priority/2 Nice to fix in this release priority/1 Highly recommended to fix in this release labels Jul 13, 2023
@DaiYuzeng DaiYuzeng mentioned this issue Jul 31, 2023
Merged
@DaiYuzeng DaiYuzeng added the not-require/test-plan Skip to create a e2e automation test issue label Jul 31, 2023
@harvesterhci-io-github-bot
Copy link

harvesterhci-io-github-bot commented Jul 31, 2023 •
edited by DaiYuzeng

Pre Ready-For-Testing Checklist

  • [ ] If labeled: require/HEP Has the Harvester Enhancement Proposal PR submitted?
    The HEP PR is at:

  • [ ] Where is the reproduce steps/test steps documented?
    The reproduce steps/test steps are at:

  • [ ] Is there a workaround for the issue? If so, where is it documented?
    The workaround is at:

  • [ ] Have the backend code been merged (harvester, harvester-installer, etc) (including backport-needed/*)?
    The PR is at:

    • Does the PR include the explanation for the fix or the feature?

    • Does the PR include deployment change (YAML/Chart)? If so, where are the PRs for both YAML file and Chart?
      The PR for the YAML change is at:
      The PR for the chart change is at:

  • If labeled: area/ui Has the UI issue filed or ready to be merged?
    The UI issue/PR is at: Implement the directive of positive number rancher/dashboard#9451

  • [ ] If labeled: require/doc, require/knowledge-base Has the necessary document PR submitted or merged?
    The documentation/KB PR is at:

  • If NOT labeled: not-require/test-plan Has the e2e test plan been merged? Have QAs agreed on the automation test case? If only test case skeleton w/o implementation, have you created an implementation issue?

    • The automation skeleton PR is at:
    • The automation test case PR is at:

  • If the fix introduces the code for backward compatibility Has a separate issue been filed with the label release/obsolete-compatibility?
    The compatibility issue is filed at:

@bk201 bk201 assigned torchiaf and unassigned DaiYuzeng Mar 21, 2024
@bk201
Copy link
Member

bk201 commented Mar 21, 2024

@torchiaf Can you help check if we can move to ready for testing and confirm we have this in v1.3.0+. Thanks.

@torchiaf
Copy link

I confirm this has been fixed in 1.2 and 1.3.0. Moving in Ready for tests.

@irishgordo irishgordo self-assigned this Apr 16, 2024
@irishgordo
Copy link
Author

Thanks for the fix @torchiaf 😄 👍
Validated this looks good on v1.2-head
Version: v1.2-4db9146e-head

Screenshot from 2024-04-16 14-26-04

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@irishgordo irishgordo

@torchiaf torchiaf

Labels
area/rancher Rancher related issues area/ui kind/bug Issues that are defects reported by users or that we know have reached a real release not-require/test-plan Skip to create a e2e automation test issue priority/1 Highly recommended to fix in this release priority/2 Nice to fix in this release reproduce/always Reproducible 100% of the time severity/4 Function working but has a minor issue (a minor incident with low impact)
Projects
None yet
Milestone
v1.2.2
Development

No branches or pull requests
6 participants
@bk201 @irishgordo @DaiYuzeng @WuJun2016 @torchiaf @harvesterhci-io-github-bot