Skip to content

consider placing rlimits in own namespace #413

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

Open
igalic opened this issue Jun 11, 2018 · 7 comments
Open

consider placing rlimits in own namespace #413

igalic opened this issue Jun 11, 2018 · 7 comments

Comments

@igalic
Copy link
Collaborator

igalic commented Jun 11, 2018

Currently, all properties are added to the "global namespace" in our config.

That pollutes it quite a bit, and is confusing, when comparing it with the defaults.
I propose moving rlimits to their own namespace, by prefixing them with rlimits..


n.b.: it might also be helpful to extend our getter to not just return one or all, but also all from a namespace..

@gronke
Copy link
Member

gronke commented Jun 13, 2018

I find it very reasonable to namespace all rlimits. The output format still needs to remain compatible with existing iocage versions, so that I suggest to omit the rlimits option and automatically enable it as soon as any rlimit_ prefixed property is enabled. All limit properties are mapped to a prefixed property name, e.g. vcpu maps to rlimit_vcpu.

@igalic
Copy link
Collaborator Author

igalic commented Jun 13, 2018

imo, an _ might still be misleading on first sight.
further, rlimits, is still a useful toggle, but i'm not sure about the cognitive cost


upon further contemplation, i'd like to weaken is a useful toggle to might be a useful toggle.

@gronke
Copy link
Member

gronke commented Jun 13, 2018

So if you have set an rlimit, having a toggle that enables or disables then seems dangerous. If you properly limit your resources, but forget to set that toggle, it could be the case that limits get silently skipped. So I'd prefer to assume them to be enabled as soon as at least one limit is set.

@igalic
Copy link
Collaborator Author

igalic commented Oct 2, 2018

now that we've implemented #470 in #524, i think it's time to revisit this issue.

@urosgruber
Copy link
Contributor

This one would be nice as we're also waiting to stabilize our infrastructure and with this in mind to also have native cpuset support.

@gronke
Copy link
Member

gronke commented Mar 19, 2019

Before working on this config property grouping, which I really would love to see rather sooner than later, the impact for legacy iocage support should be evaluated. So far we do set legacy compatible config values, if config properties are inherited from iocage. It is easy to map a value when saving or reading, but it's another issue if the config property name changes entirely. I'm not a fan of the idea having to maintain two config files, introducing breakage with iocage legacy (and python-iocage < 1.0). Maybe the best option we have is to store them as is, but present the values with a mapped name (X -> rlimit.X) from JailConfig.

@gronke
Copy link
Member

gronke commented Mar 19, 2019

Btw. there already is an Issue for the cpuset feature: #636

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants