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.

Sign up for GitHub

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

Jump to bottom

Requested service resources adaptation #164

Merged
merged 3 commits into from
Sep 20, 2024
Merged

Requested service resources adaptation #164

merged 3 commits into from
Sep 20, 2024

Conversation

mykola-kobets-epam
Copy link
Collaborator

No description provided.

@mykola-kobets-epam mykola-kobets-epam force-pushed the serv-req-resources-adaptation branch 3 times, most recently from b76c7ea to 4a25102 Compare September 5, 2024 16:54
@mykola-kobets-epam mykola-kobets-epam changed the title Serv req resources adaptation Requested service resources adaptation Sep 5, 2024
mlohvynenko
mlohvynenko reviewed Sep 6, 2024
View reviewed changes
launcher/launcher.go Outdated
func clampResource(value uint64, quota *uint64) uint64 {
if quota != nil && value > *quota {
return *quota
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's simple return value, without else branch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

mlohvynenko
mlohvynenko reviewed Sep 6, 2024
View reviewed changes
launcher/launcher.go Outdated
}

if nodeRatios != nil && nodeRatios.Storage != nil {
return float64(*nodeRatios.Storage) / 100
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this return required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, "missing return " error have otherwise

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't use naked return.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@mykola-kobets-epam mykola-kobets-epam changed the title Requested service resources adaptation [WIP]Requested service resources adaptation Sep 6, 2024
@mykola-kobets-epam mykola-kobets-epam changed the base branch from feature_dynamic_nodes to develop September 10, 2024 10:45
al1img
al1img reviewed Sep 18, 2024
View reviewed changes
launcher/launcher.go Outdated
}

if nodeRatios != nil && nodeRatios.Storage != nil {
return float64(*nodeRatios.Storage) / 100
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't use naked return.

launcher/launcher.go
return float64(*serviceRatios.Storage) / 100
func getReqDiskSize(serviceConfig aostypes.ServiceConfig, nodeRatios *aostypes.ResourceRatiosInfo,
) (stateSize, storageSize uint64) {
stateQuota := serviceConfig.Quotas.StateLimit
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are intermediate vars required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to make a line shorter in a place where it is used so it didnt move to a next line

launcher/launcher.go Outdated
storageQuota := serviceConfig.Quotas.StorageLimit

if serviceConfig.RequestedResources != nil && serviceConfig.RequestedResources.Storage != nil {
stateSize = clampResource(*serviceConfig.RequestedResources.Storage, stateQuota)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clamping should be done for all the cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not required to clamp values from NodeConfig as they are a ratios of quota,

launcher/nodehandler.go
@@ -311,10 +311,12 @@ func (node *nodeHandler) getRequestedCPU(
instanceIdent aostypes.InstanceIdent, serviceConfig aostypes.ServiceConfig,
) uint64 {
requestedCPU := uint64(0)
cpuQuota := serviceConfig.Quotas.CPUDMIPSLimit
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto for the intermediate variable and clamping

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not clear what should be done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Somehow my comments about clamping for all cases disappeared: clamping should be done for all cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

launcher/nodehandler.go
@@ -336,10 +338,12 @@ func (node *nodeHandler) getRequestedRAM(
instanceIdent aostypes.InstanceIdent, serviceConfig aostypes.ServiceConfig,
) uint64 {
requestedRAM := uint64(0)
ramQuota := serviceConfig.Quotas.RAMLimit
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

launcher/launcher.go
stateQuota := serviceConfig.Quotas.StateLimit
storageQuota := serviceConfig.Quotas.StorageLimit

if serviceConfig.RequestedResources != nil && serviceConfig.RequestedResources.Storage != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have different values for state and storage. Let's discuss with cloud.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@mykola-kobets-epam mykola-kobets-epam changed the title [WIP]Requested service resources adaptation Requested service resources adaptation Sep 20, 2024
al1img
al1img reviewed Sep 20, 2024
View reviewed changes
launcher/launcher.go
storageQuota := serviceConfig.Quotas.StorageLimit

if serviceConfig.RequestedResources != nil && serviceConfig.RequestedResources.State != nil {
stateSize = clampResource(*serviceConfig.RequestedResources.State, stateQuota)
Copy link
Collaborator

Choose a reason for hiding this comment

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

clamp should be done in all cases. Resource ration could exceed quota as well.

Copy link
Collaborator Author

@mykola-kobets-epam mykola-kobets-epam Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

in node config resource ratios specified in the percents of the quota. which means it should be less than resource quota be design.

launcher/nodehandler.go
@@ -311,10 +311,12 @@ func (node *nodeHandler) getRequestedCPU(
instanceIdent aostypes.InstanceIdent, serviceConfig aostypes.ServiceConfig,
) uint64 {
requestedCPU := uint64(0)
cpuQuota := serviceConfig.Quotas.CPUDMIPSLimit
Copy link
Collaborator

Choose a reason for hiding this comment

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

Somehow my comments about clamping for all cases disappeared: clamping should be done for all cases.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 20, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@al1img al1img merged commit cf48642 into aosedge:develop Sep 20, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@al1img al1img al1img left review comments

@mlohvynenko mlohvynenko mlohvynenko approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mykola-kobets-epam @al1img @mlohvynenko