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

Fix default and minimum value for CPU Shares #289

Merged
merged 1 commit into from
Jan 13, 2020

Conversation

marusak
Copy link
Member

@marusak marusak commented Jan 10, 2020

Minimum value is 2.
Default value is 1024. We still test that if we don't set it up, it is
0, but for that see containers/podman#4822

Fixes #287

@@ -205,7 +205,7 @@ export class ImageRunModal extends React.Component {
publish: [],
image: props.image,
memory: 512,
cpuShares: 0,
cpuShares: 1024,
Copy link
Member

Choose a reason for hiding this comment

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

Admitteldy I don't like hardcoding this default very much, but of course the UI needs to know what to set the value to once you enable CPU shares. But given that "0" means "use the internal default", not literally "zero shares", could we just continue to interpret it that way? I. e. with "0" the setting would just appear disabled?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that 0 means that. 0 is not defined anywhere what it mean, and seems that podman inspects shows 0 wrongly.
If we don't want 1024, then I would rather suggest not having any value.

Copy link
Member

Choose a reason for hiding this comment

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

Right, if it works with null and then just not gets passed, that'd be even better of course -- we leave it as internal podman default then.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, so I dropped the default value.
Forgot to change the commit message, but I'll update that on marging.

src/ImageRunModal.jsx Show resolved Hide resolved
Minimum value is 2.
Default value is 1024. We still test that if we don't set it up, it is
0, but for that see containers/podman#4822

Fixes: cockpit-project#287
Closes: cockpit-project#289
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks! "2" instead of "null" is so that it gets along with the input element, I suppose?

@marusak
Copy link
Member Author

marusak commented Jan 13, 2020

Thanks! "2" instead of "null" is so that it gets along with the input element, I suppose?

Yeah, we could either have no check for minimum value or check for specific value. And it is always the best approach to have native elements doing as much work as possible - rather than we would manually check if the value is >= 2.

@marusak marusak merged commit 0185702 into cockpit-project:master Jan 13, 2020
@marusak marusak deleted the 1024_and_2 branch January 13, 2020 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minimum cpu-shares is 2
2 participants