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

Externally managed fixes #567

Merged
merged 2 commits into from
Jan 10, 2024
Merged

Externally managed fixes #567

merged 2 commits into from
Jan 10, 2024

Conversation

adrianchiris
Copy link
Collaborator

  • block mtu change on PF when externally managed
  • fix handling in mellanox plugin for externally managed interfaces

@github-actions GitHub Actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@adrianchiris
Copy link
Collaborator Author

@SchSeba PTAL

@coveralls
Copy link

coveralls commented Dec 27, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 7472646223

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 23.347%
Totals Coverage Status
Change from base Build 7463482879: -0.02%
Covered Lines: 2302
Relevant Lines: 9860
💛 - Coveralls

adrianchiris
adrianchiris commented Dec 28, 2023
View reviewed changes
pkg/daemon/plugin.go Outdated

// populate Set of vendors for plugin creation
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

while at it, i added this to instantiate plugin only once per vendor, hope its ok

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we need a separate set here? Is it possible to simply check if existing vendorPlugins map already contains a vendor name and skip plugin initialization in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is, and will result in less code changes ! will do the modification

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

going back to this change,

vendorPlugins is keyed by plugin name and not by vendor.

to instantiate once we then need this set.
if we want just to set a plugin once in vendorPlugins map then we can do as u say.

seeing its not a really big deal, i will just set plugin in map once.

@github-actions GitHub Actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@adrianchiris
Copy link
Collaborator Author

@SchSeba can we merge this one ? these fixes are much needed

mlguerrero12
mlguerrero12 reviewed Jan 8, 2024
View reviewed changes
pkg/utils/utils.go Outdated Show resolved Hide resolved
@adrianchiris
Copy link
Collaborator Author

@ykulazhenkov PTAL
@SchSeba PTAL, PR rebased and ready for review.

@adrianchiris
Copy link
Collaborator Author

@zeeke could you take a look ? :)

@adrianchiris
Do not reset Firmware config in Mellanox plugin
dc5c9d2 
When interface(pf) is externally managed, we need to avoid
resetting firmware configurations.

- use store manager to get latest pf config from host so we can reliably
  determine if interface is externally managed if it does not appear in
  spec
- when interface appears in spec, block both sriov and link type
  firmware configurations if interface is externally managed
- skip pfs that are externally managed

Signed-off-by: adrianc <adrianc@nvidia.com>
@adrianchiris
block mtu change on pf when externally managed
2af158f 
Signed-off-by: adrianc <adrianc@nvidia.com>
@github-actions GitHub Actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

zeeke
zeeke approved these changes Jan 10, 2024
View reviewed changes
Copy link
Member

@zeeke zeeke left a comment

Choose a reason for hiding this comment

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

LGTM

@adrianchiris adrianchiris merged commit 5752f32 into k8snetworkplumbingwg:master Jan 10, 2024
11 checks passed
@adrianchiris adrianchiris deleted the externally-managed-fixes branch July 16, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlguerrero12 mlguerrero12 mlguerrero12 left review comments

@ykulazhenkov ykulazhenkov ykulazhenkov left review comments

@zeeke zeeke zeeke approved these changes

@SchSeba SchSeba Awaiting requested review from SchSeba

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@adrianchiris @coveralls @zeeke @mlguerrero12 @ykulazhenkov