Skip to content
This repository has been archived by the owner on Jul 16, 2020. It is now read-only.

ciao-deploy: enable memory limits by default #1540

Merged
merged 1 commit into from
Oct 27, 2017

Conversation

kaccardi
Copy link
Contributor

@kaccardi kaccardi commented Oct 23, 2017

Enable memory limit checking by launcher by default,
and only disable if --disable-limits is passed to ciao-deploy setup.

Fixes: #1539

Signed-off-by: Kristen Carlson Accardi [email protected]

@kaccardi kaccardi requested a review from rbradford October 23, 2017 19:31
@coveralls
Copy link

coveralls commented Oct 23, 2017

Coverage Status

Coverage increased (+0.005%) to 65.743% when pulling 53ffe1d on kaccardi:topic/issue-1539 into f6b599e on ciao-project:master.

@rbradford
Copy link
Contributor

@kaccardi see @markdryan's comment on #1539 "Let's only enable limits for memory for now. The concept of disk limits is currently broken, see #1541."

@coveralls
Copy link

coveralls commented Oct 24, 2017

Coverage Status

Coverage increased (+0.005%) to 65.743% when pulling 3a1d6c0 on kaccardi:topic/issue-1539 into f6b599e on ciao-project:master.

@kaccardi kaccardi changed the title ciao-deploy: enable mem/disk limits by default ciao-deploy: enable memory limits by default Oct 24, 2017
@rbradford
Copy link
Contributor

@kaccardi Thanks for the update. The changes in ciao-deploy lgtm. However do you know if we need to disable in singlevm.

Could you also update the commit message (i see you've updated the PR description already.)

@coveralls
Copy link

coveralls commented Oct 26, 2017

Coverage Status

Coverage decreased (-0.005%) to 65.666% when pulling 8476826 on kaccardi:topic/issue-1539 into 9e6de96 on ciao-project:master.

@kaccardi
Copy link
Contributor Author

@rbradford I confirmed you do not need to disable memory in single vm, so I removed that.

config.Configure.Launcher.DiskLimit = false
config.Configure.Launcher.MemoryLimit = false
if clusterConf.DisableLimits {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see how this has any effect. By default,

config.Configure.Launcher.MemoryLimit = false

When we supply --disable-limits it will still be false.

Should this be

if !clusterConf.DisableLimits {
    config.Configure.Launcher.MemoryLimit = true
}

Copy link
Contributor

Choose a reason for hiding this comment

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

or simply

config.Configure.Launcher.MemoryLimit = !clusterConf.DisableLimits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I swear this was tested... but you are right, this doesn't do anything. I must have broken something when I was doing some quick adjustment. will fix, sorry.

Enable memory limit checking by launcher by default,
and only disable if --disable-limits is passed to ciao-deploy setup.

Signed-off-by: Kristen Carlson Accardi <[email protected]>
@coveralls
Copy link

coveralls commented Oct 27, 2017

Coverage Status

Coverage remained the same at 65.66% when pulling fc67855 on kaccardi:topic/issue-1539 into 3ea39a2 on ciao-project:master.

@markdryan markdryan merged commit 10215ed into ciao-project:master Oct 27, 2017
@kaccardi kaccardi deleted the topic/issue-1539 branch October 27, 2017 16:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants