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

Allow creating policies that ignore NUMA topology #435

Merged
merged 3 commits into from
Jun 1, 2023

Conversation

zeeke
Copy link
Member

@zeeke zeeke commented May 8, 2023

This PR allows users to create policies that set the sriov-network-device plugin ExcludeTopology field in the advertised resource list.

The feature is helpful in scenarios where the kubelet is configured with --topology-manager-policy single-numa-node [2] and some workloads may run with a NIC that is not aligned to the CPU/Memory NUMA node.

[1] https://github.com/k8snetworkplumbingwg/sriov-network-device-plugin/blob/master/pkg/types/types.go#L101
[2] https://kubernetes.io/docs/tasks/administer-cluster/topology-manager/#policy-single-numa-node

@github-actions
Copy link

github-actions bot commented May 8, 2023

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 force-pushed the exclude-topology branch from 118e154 to 634362e Compare May 8, 2023 10:01
@github-actions
Copy link

github-actions bot commented May 8, 2023

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 force-pushed the exclude-topology branch from 634362e to 1d82c87 Compare May 8, 2023 11:22
@github-actions
Copy link

github-actions bot commented May 8, 2023

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.

@coveralls
Copy link

coveralls commented May 8, 2023

Pull Request Test Coverage Report for Build 5135001239

  • 50 of 139 (35.97%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 25.842%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/webhook/validate.go 18 20 90.0%
controllers/sriovnetworknodepolicy_controller.go 32 119 26.89%
Totals Coverage Status
Change from base Build 5131893583: 0.3%
Covered Lines: 1964
Relevant Lines: 7600

💛 - Coveralls

@zeeke zeeke changed the title Allow creating policies that ignore NUMA topologly Allow creating policies that ignore NUMA topology May 8, 2023
@zeeke zeeke force-pushed the exclude-topology branch from 1d82c87 to c03a132 Compare May 9, 2023 09:27
@github-actions
Copy link

github-actions bot commented May 9, 2023

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 force-pushed the exclude-topology branch from c03a132 to 0318327 Compare May 9, 2023 09:41
@github-actions
Copy link

github-actions bot commented May 9, 2023

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.

@SchSeba SchSeba self-requested a review May 9, 2023 10:25
Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

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

Great work!
clean PR :)

/lgtm

@zeeke zeeke force-pushed the exclude-topology branch from 0318327 to 9024862 Compare May 30, 2023 09:08
@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.

Copy link
Collaborator

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

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

minor nit otherwise lgtm

+1 on the refactors here :)

@@ -57,6 +57,8 @@ type SriovNetworkNodePolicySpec struct {
// +kubebuilder:validation:Enum=virtio
// VDPA device type. Allowed value "virtio"
VdpaType string `json:"vdpaType,omitempty"`
// Exclude device's NUMA node when advertising this resource. Default to false.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Exclude device's NUMA node when advertising this resource by sriov network device plugin

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks good, thanks

@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

@zeeke lets rebase and we are good 2 go with merging this one

zeeke added 3 commits May 31, 2023 17:43
The function mainly creates and updates ResourceConfig
in the list. Split the function into two smaller ones.

Adjust unit test to satisfy `SriovNetworkNodePolicyReconciler`
dependencies through fake client.

Signed-off-by: Andrea Panattoni <[email protected]>
Add field `SriovNetworkNodePolicy.Spec.ExcludeTopology` and
forward it to the sriov-device-plugin configuration.

Signed-off-by: Andrea Panattoni <[email protected]>
Multiple SriovNetworkNodePolicies may target the same
resource, but the new field must have the same value. Add
a resource validation step to ensure this condition is met.

Refactor `Spec.NicSelector.PfNames` check to its own function.

Signed-off-by: Andrea Panattoni <[email protected]>
@zeeke zeeke force-pushed the exclude-topology branch from 495679b to f55a17e Compare May 31, 2023 15:52
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants