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

node-agent config for data mover micro service pod resources #8143

Merged

Conversation

Lyndon-Li
Copy link
Contributor

Fix issue #8134, allow to config resource request/limit for data mover micro service pods

@Lyndon-Li Lyndon-Li force-pushed the data-mover-ms-pod-resource-limit branch from bdbe66c to 627e2fe Compare August 22, 2024 07:07
@Lyndon-Li Lyndon-Li marked this pull request as ready for review August 22, 2024 07:07
Copy link

codecov bot commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 54.16667% with 11 lines in your changes missing coverage. Please review.

Project coverage is 59.06%. Comparing base (934b3ea) to head (252e8a8).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
pkg/cmd/cli/nodeagent/server.go 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8143      +/-   ##
==========================================
- Coverage   59.07%   59.06%   -0.02%     
==========================================
  Files         364      364              
  Lines       30298    30321      +23     
==========================================
+ Hits        17899    17909      +10     
- Misses      10956    10969      +13     
  Partials     1443     1443              

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

}

type PodResources struct {
CPURequest string `json:"cpuRequest,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also allow users to set requests and limits for emphemeral-storage as well ?

Copy link
Contributor Author

@Lyndon-Li Lyndon-Li Aug 23, 2024

Choose a reason for hiding this comment

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

Do you mean the root file system of the pod? We already have the backup repo config for users to set the cacheLimit which consumes the root file system space. Isn't it enough?

And for the current change in 1.15, we just need to fix the gap comparing to the previous releases, for any new functionalities, we can add post 1.15.
So we can keep this discussion rolling but we will not include changes into this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is what I meant root file system of the pod. We don't need to let users config it, instead, we let users to config the cacheLimit which consumes the most proportion of the root file system space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shubham-pampattiwar Does the above comment answer your question? Or do you have more comments? Once you complete the review, please help to approve it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks good, Ack!

}

type PodResources struct {
CPURequest string `json:"cpuRequest,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks good, Ack!

Copy link
Collaborator

@shubham-pampattiwar shubham-pampattiwar left a comment

Choose a reason for hiding this comment

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

/lgtm

@Lyndon-Li Lyndon-Li merged commit cb7eebd into vmware-tanzu:main Aug 29, 2024
44 of 45 checks passed
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.

3 participants