Skip to content
This repository was archived by the owner on Apr 12, 2022. It is now read-only.

Max-allowed Pods for nodes #112

Closed
wants to merge 2 commits into from
Closed

Max-allowed Pods for nodes #112

wants to merge 2 commits into from

Conversation

islinwb
Copy link
Contributor

@islinwb islinwb commented Jun 5, 2018 •
edited
Loading

for #88
Firmament part: huawei-cloudnative/firmament#3

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 5, 2018
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 5, 2018
@m1093782566
Copy link

m1093782566 commented Jun 5, 2018 •
edited
Loading

Great @islinwb

It looks promising though it's still WIP - it's a good start point.

We are looking forward to seeing your PR to firmament as well :)

@m1093782566
Copy link

/assign

@islinwb islinwb mentioned this pull request Jun 5, 2018
Open
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: islinwb
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: m1093782566

Assign the PR to them by writing /assign @m1093782566 in a comment when ready.

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

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 9, 2018
@islinwb islinwb changed the title [WIP]Max-allowed Pods for nodes Max-allowed Pods for nodes Jun 9, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2018
@m1093782566
Copy link

@islinwb

Should we merge the firmament changes first?

@islinwb
Copy link
Contributor Author

islinwb commented Jun 9, 2018

@m1093782566 Yes. The added e2e test case can pass only after merging huawei-cloudnative/firmament#3.

m1093782566
m1093782566 reviewed Jun 9, 2018
View reviewed changes
pkg/k8sclient/nodewatcher.go Outdated
@@ -106,6 +106,9 @@ func (nw *NodeWatcher) parseNode(node *v1.Node, phase NodePhase) *Node {
memCap, _ := memCapQuantity.AsInt64()
memAllocQuantity := node.Status.Allocatable[v1.ResourceMemory]
memAlloc, _ := memAllocQuantity.AsInt64()
podCapQuantity := node.Status.Capacity[v1.ResourcePods]
Copy link

Choose a reason for hiding this comment

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

It's never really used by firmament?

m1093782566
m1093782566 reviewed Jun 9, 2018
View reviewed changes
pkg/k8sclient/nodewatcher.go Outdated
@@ -115,6 +118,8 @@ func (nw *NodeWatcher) parseNode(node *v1.Node, phase NodePhase) *Node {
CPUAllocatable: cpuAllocQuantity.MilliValue(),
MemCapacityKb: memCap / bytesToKb,
MemAllocatableKb: memAlloc / bytesToKb,
PodCapacity: podCapQuantity.Value(),
Copy link

Choose a reason for hiding this comment

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

It's never really used by firmament? We just pass the PodAllocatable to firmament?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@m1093782566 Only use PodAllocatable now. I'll check whether PodCapacity should be used.

@shivramsrivastava
Copy link
Contributor

@islinwb Thanks for the PR.
I will review it in sometime.

For the E2E i would suggest to create an image and upload it here, with a tag like 'e2e' or 'v0.3_beta'. But the firmament side code needs to be reviewed and merged first.

Also shouldn't the test case modify capacity for nodes and check, i don't know if an update to node will set the capacity, or one needs to restart the kubelet of the node to change the capacity.

Because what i understands from the k8s documentation, the max pods limit is set at the time of bringing up the node, and later changes we need to restart the kubelet ???

If the later case is true, where we have to restart the nodes agent to make the capacity change.
Then we can simply the code on firmament side?
We don't have to consult the knowledgeable for the node capacity calculation ?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 11, 2018
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 11, 2018
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 11, 2018
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 11, 2018
@islinwb islinwb force-pushed the max_pod_num branch 3 times, most recently from 1e22c74 to 78da596 Compare June 12, 2018 04:41
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 14, 2018
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 19, 2018
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 19, 2018
@islinwb
Copy link
Contributor Author

islinwb commented Jun 22, 2018 •
edited
Loading

@shivramsrivastava @m1093782566 I've tested in my local VM with the modified Firmament code and the kubelet flag max-pods works.

@m1093782566
Copy link

We don't need to consider other scheduler for the time being.

@hanxiaoshuai
Copy link
Contributor

/test ci-poseidon-e2e-gce

@k8s-ci-robot
Copy link
Contributor

@islinwb: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci-poseidon-e2e-gce f059feb link /test ci-poseidon-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@m1093782566 m1093782566 mentioned this pull request Oct 10, 2018
Closed
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@m1093782566 m1093782566 m1093782566 left review comments

@dhilipkumars dhilipkumars Awaiting requested review from dhilipkumars

@irfanurrehman irfanurrehman Awaiting requested review from irfanurrehman

Assignees

@m1093782566 m1093782566

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@islinwb @m1093782566 @k8s-ci-robot @shivramsrivastava @hanxiaoshuai @fejta-bot