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

feat(workflow): set the weight of cpu request of custom containers #1518

Closed
wants to merge 2 commits into from
Closed

feat(workflow): set the weight of cpu request of custom containers #1518

wants to merge 2 commits into from

Conversation

knight42
Copy link
Collaborator

@knight42 knight42 commented Nov 3, 2020

What this PR does / why we need it:

  • Add a field WorkloadCustomContainerCPUWeight to pkg/server/config.CycloneServerConfig
  • applyResourceRequirements would adjust the cpu request of the containers according to the above setting

Which issue(s) this PR is related to (optional, link to 3rd issue(s)):

Fixes #

Reference to #

Special notes for your reviewer:

/cc @zhujian7

Release note:

NONE

@caicloud-bot caicloud-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Nov 3, 2020
@caicloud-bot
Copy link
Collaborator

Thanks for your pull request. Before we can look at your pull request, you'll need to finish a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/caicloud/engineering/blob/master/guidelines/cla.md to complete the CLA.

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 caicloud/tools repository. I understand the commands that are listed here.

@caicloud-bot caicloud-bot added the caicloud-cla: no Indicates the PR's author has not signed the Caicloud CLA. label Nov 3, 2020
@caicloud-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

Assign the PR to them by writing /assign @zhujian7 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

@caicloud-bot caicloud-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 3, 2020
@caicloud-bot caicloud-bot added caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. and removed caicloud-cla: no Indicates the PR's author has not signed the Caicloud CLA. labels Nov 3, 2020
zhujian7
zhujian7 reviewed Nov 3, 2020
View reviewed changes
pkg/server/config/config.go Outdated
@@ -75,6 +75,8 @@ type CycloneServerConfig struct {

// Artifact config for artifacts which are managed by cyclone server
Artifact ArtifactConfig `json:"artifact"`

WorkloadCustomContainerCPUWeight int `json:"workload_custom_container_cpu_weight"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add some comments for the field

zhujian7
zhujian7 requested changes Nov 3, 2020
View reviewed changes
pkg/workflow/workload/pod/builder.go Outdated
@@ -994,8 +1036,8 @@ func (m *Builder) ApplyResourceRequirements() error {
requirements = m.wf.Spec.Resources
}

m.pod.Spec.InitContainers = applyResourceRequirements(m.pod.Spec.InitContainers, requirements, false)
m.pod.Spec.Containers = applyResourceRequirements(m.pod.Spec.Containers, requirements, true)
m.pod.Spec.InitContainers = applyResourceRequirements(m.pod.Spec.InitContainers, requirements, false, config.Config.WorkloadCustomContainerCPUWeight)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Init containers executes one by one, so I think it is not necessary to comply with this rule.

@knight42
Copy link
Collaborator Author

knight42 commented Nov 3, 2020

@zhujian7 PTAL

zhujian7
zhujian7 reviewed Nov 3, 2020
View reviewed changes
pkg/server/config/config.go Outdated

// WorkloadCustomContainerCPUWeight represents the proportion of cpu resource that custom containers would consume(1-100).
// If this field is equal to 0, its value is changed to 100.
WorkloadCustomContainerCPUWeight int `json:"workload_custom_container_cpu_weight"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

CustomContainerCPUWeight is simpler?

@zhujian7
Copy link
Collaborator

zhujian7 commented Nov 3, 2020

@knight42 please fix the CI.

@caicloud-bot caicloud-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 3, 2020
@caicloud-bot caicloud-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 3, 2020
@zhujian7
Copy link
Collaborator

zhujian7 commented Nov 3, 2020

/cherrypick cps-2.8

@caicloud-bot
Copy link
Collaborator

@zhujian7: once the present PR merges, I will cherry-pick it on top of cps-2.8 in a new PR and assign it to you.

In response to this:

/cherrypick cps-2.8

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.

@caicloud-bot caicloud-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 3, 2020
@knight42
Copy link
Collaborator Author

knight42 commented Nov 3, 2020

/hold
Leave time for review. @zhujian7 PTAL

@caicloud-bot caicloud-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2020
@knight42
Copy link
Collaborator Author

knight42 commented Nov 4, 2020

/close

b/c this is not the root cause.

@knight42 knight42 deleted the feat/workflow-cpu-weight branch November 4, 2020 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhujian7 zhujian7 Awaiting requested review from zhujian7

Assignees
No one assigned
Labels
caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@knight42 @caicloud-bot @zhujian7