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

feat(overcommit):add nodeOvercommitConfig crd #34

Merged

Conversation

WangZzzhe
Copy link
Collaborator

What type of PR is this?

Features

What this PR does / why we need it:

  1. add nodeOvercommitConfig CRD for managing node resource overcommit ratio in a new group 'overcommit'.
  2. add default node overcommit ratio and node overcommit selector key constants.

@caohe caohe added enhancement New feature or request workflow/need-review review: test succeeded, need to review labels Aug 30, 2023
@caohe caohe added this to the v0.4 milestone Aug 30, 2023
waynepeking348
waynepeking348 previously approved these changes Sep 5, 2023
waynepeking348
waynepeking348 previously approved these changes Sep 11, 2023
// ResourceOvercommitRatioConfig describes the resource overcommit ratio that needs to inject into Node.Annotations
// cpu,memory are supported.
// +optional
ResourceOvercommitRatioConfig map[v1.ResourceName]string `json:"resourceOvercommitRatioConfig,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not use a slice of custom defined structure with resource name as a field instead of map, because the scalebility of map is poor, and k8s also suggest using list instead of map https://github.com/kubernetes/kubernetes/issues/2004#issuecomment-60641437

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why not use a slice of custom defined structure with resource name as a field instead of map, because the scalebility of map is poor, and k8s also suggest using list instead of map https://github.com/kubernetes/kubernetes/issues/2004#issuecomment-60641437

thanks for the suggestion. The scalebility is pool that each resource only has a string value. I'll update it accroding to the suggestion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@luomingmeng I thought about it, maybe "ResourceOvercommitRatio" is more like "ResourceList" in container Resources, also we need a map to ensure the uniqueness of the resources. What do you think?
Do you have any suggestions? @caohe

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the field is similar to ResourceList, it is better to use the name ResourceOvercommitRatio directly, rather than using ResourceOvercommitRatioConfig.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, ResourceOvercommitRatioConfig is renamed to ResourceOvercommitRatio.

luomingmeng
luomingmeng previously approved these changes Sep 20, 2023
@caohe caohe added the workflow/merge-ready merge-ready: code is ready and can be merged label Sep 22, 2023
@waynepeking348 waynepeking348 merged commit 92231a6 into kubewharf:main Sep 22, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request workflow/merge-ready merge-ready: code is ready and can be merged workflow/need-review review: test succeeded, need to review
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

4 participants