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

koordlet: fix NUMA info reporting #1587

Merged

Conversation

saintube
Copy link
Member

@saintube saintube commented Aug 28, 2023

Ⅰ. Describe what this PR does

koordlet: Fix the NUMA ZoneList reporting in NRT, and revise the NRT updating check.

Ⅱ. Does this pull request fix one issue?

fixes #1585.

Ⅲ. Describe how to verify it

Expect the NRT is like:

apiVersion: topology.node.k8s.io/v1alpha1
kind: NodeResourceTopology
metadata:
  annotations:
    kubelet.koordinator.sh/cpu-manager-policy: '{"policy":"none"}'
    node.koordinator.sh/cpu-shared-pools: '[{"socket":0,"node":0,"cpuset":"0-15"}]'
    node.koordinator.sh/cpu-topology: '{"detail":[{"id":0,"core":0,"socket":0,"node":0},{"id":1,"core":0,"socket":0,"node":0},{"id":2,"core":1,"socket":0,"node":0},{"id":3,"core":1,"socket":0,"node":0},{"id":4,"core":2,"socket":0,"node":0},{"id":5,"core":2,"socket":0,"node":0},{"id":6,"core":3,"socket":0,"node":0},{"id":7,"core":3,"socket":0,"node":0},{"id":8,"core":4,"socket":0,"node":0},{"id":9,"core":4,"socket":0,"node":0},{"id":10,"core":5,"socket":0,"node":0},{"id":11,"core":5,"socket":0,"node":0},{"id":12,"core":6,"socket":0,"node":0},{"id":13,"core":6,"socket":0,"node":0},{"id":14,"core":7,"socket":0,"node":0},{"id":15,"core":7,"socket":0,"node":0}]}'
    node.koordinator.sh/reservation: '{}'
    node.koordinator.sh/system-qos-resource: '{}'
  labels:
    app.kubernetes.io/managed-by: Koordinator
  name: test-node
  ownerReferences:
  - apiVersion: v1
    blockOwnerDeletion: true
    controller: true
    kind: Node
    name: test-node
topologyPolicies:
- None
zones:
- name: node-0
  resources:
  - allocatable: "16"
    available: "16"
    capacity: "16"
    name: cpu
  - allocatable: 64610984Ki
    available: 64610984Ki
    capacity: 64610984Ki
    name: memory
  type: Node

Ⅳ. Special notes for reviews

V. Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in make test

@saintube saintube changed the title koordlet: fix numa info reporting koordlet: fix NUMA info reporting Aug 28, 2023
@saintube saintube requested a review from eahydra August 28, 2023 08:37
@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Patch coverage: 52.94% and no project coverage change.

Comparison is base (8ed0f75) 65.17% compared to head (c587d43) 65.17%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1587   +/-   ##
=======================================
  Coverage   65.17%   65.17%           
=======================================
  Files         352      352           
  Lines       36353    36400   +47     
=======================================
+ Hits        23692    23723   +31     
- Misses      10918    10928   +10     
- Partials     1743     1749    +6     
Flag Coverage Δ
unittests 65.17% <52.94%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...statesinformer/impl/states_noderesourcetopology.go 67.84% <52.38%> (-1.72%) ⬇️
...advisor/collectors/nodeinfo/node_info_collector.go 80.32% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zwzhang0107
Copy link
Contributor

/lgtm
/approve

@koordinator-bot koordinator-bot bot merged commit fa037da into koordinator-sh:main Aug 28, 2023
@koordinator-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jasonliu747, zwzhang0107

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

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

Successfully merging this pull request may close these issues.

[BUG] koordlet reported NRT missing ZoneList
3 participants