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

ws-daemon: Remove the cpu bucket limit because we don't use #9915

Closed
wants to merge 1 commit into from

Conversation

utam0k
Copy link
Contributor

@utam0k utam0k commented May 10, 2022

Description

Remove the cpu bucket limit because we don't use it. now we are using only FixedLimiter. Saves testing and build time

Related Issue(s)

No

How to test

  • pass the go tests

Release Notes

ws-daemon: Remove the bucket CPULimit

Documentation

No

@utam0k utam0k requested a review from a team May 10, 2022 22:48
@github-actions github-actions bot added the team: workspace Issue belongs to the Workspace team label May 10, 2022
@utam0k utam0k force-pushed the to/remove-cpu-bucket branch from 8fc939b to a3c1532 Compare May 10, 2022 22:57
@utam0k utam0k requested a review from a team May 10, 2022 22:57
@github-actions github-actions bot added the team: delivery Issue belongs to the self-hosted team label May 10, 2022
@@ -222,79 +222,6 @@ func (f fixedLimiter) Limit(budgetSpent CPUTime) (newLimit Bandwidth) {
return f.FixedLimit
}

// Bucket describes a "pot of CPU time" which can be spent at a particular rate.
type Bucket struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is proof that it has not been used because this part has been erased and the other parts have not been changed.

@utam0k
Copy link
Contributor Author

utam0k commented May 10, 2022

@csweichel Any reason why you left the BucketLimit behind when you refactored CPULimit?
#8036

@utam0k utam0k changed the title Remove the cpu bucket limit because we don't use ws-daemon: Remove the cpu bucket limit because we don't use May 10, 2022
@utam0k
Copy link
Contributor Author

utam0k commented May 11, 2022

/werft run with-no-preview

👍 started the job as gitpod-build-to-remove-cpu-bucket.2
(with .werft/ from main)

@utam0k
Copy link
Contributor Author

utam0k commented May 11, 2022

/werft run

👍 started the job as gitpod-build-to-remove-cpu-bucket.3
(with .werft/ from main)

@nandajavarma
Copy link
Contributor

The changes generally looks good! A small heads up to remove the dynamicLimits reference from the installer config for the preview environment.

Comment on lines -223 to -225
DynamicLimits *struct {
CPU []cpulimit.Bucket // todo(sje): add custom validation
} `json:"dynamicLimits,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: this is backwards incompatible change. We load the config "strictly", i.e. removing this field might break existing configuration. We don't want that.

Even though this field hasn't been used (had no effect), removing it now might break someone's config.

IMHO we should do something like this:

DeprecatedDyanmicLimits json.RawMessage `json:"dynamicLimits,omitempty" validate:"deprecated"``

and add a config validation function which returns true but emits a log message that the field is deprecated and has no effect.

@csweichel
Copy link
Contributor

csweichel commented May 12, 2022

This PR introduces incompatible changes - holding because of that

/hold

@stale
Copy link

stale bot commented May 30, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label May 30, 2022
@stale stale bot closed this Jun 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold do-not-merge/work-in-progress meta: stale This issue/PR is stale and will be closed soon release-note size/L team: delivery Issue belongs to the self-hosted team team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants