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

spec: add os/unix/sysctl isolator #647

Merged
merged 2 commits into from
Aug 29, 2016
Merged

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Aug 17, 2016

This PR introduces an "os/unix/sysctl" isolator, which applies at pod-level.

Closes #645


Arbitrary/controversial choices made here:

  • isolator type, as it applies inside pod-context (like other isolators) instead of the host (like top-level stanzas: volumes, ports)
  • new "environment" isolator category, as it can also accommodate "ulimit" ones

@@ -383,6 +383,33 @@ Small quantities can be represented directly as decimals (e.g., 0.3), or using m

**NOTE**: Network limits MUST NOT apply to localhost communication between apps in a pod.

### Environment Isolators

An application or a group of applications may often require some tweaking to the environment where they run, in order to perform in an optimal way.
Copy link
Contributor

Choose a reason for hiding this comment

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

Awkward wording I think. I'll suggest "An application may want or need some changes to the system environment it is run in."

At the least, nix often.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.

@euank
Copy link
Contributor

euank commented Aug 17, 2016

I tossed on some nits, but I think this makes sense, both in terms of naming and as an isolator.

An application or a group of applications may often require some tweaking to the environment where they run, in order to perform in an optimal way.
Environment isolators take care of setting up the runtime environment as needed.

#### environment/sysctl
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to namespace this more specifically to Linux or *nix? /cc @mpasternacki if he has any thoughts from the BSD perspective

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 checked before venturing here, and at least FreeBSD has sysctl tuning support: https://www.freebsd.org/doc/handbook/configtuning-sysctl.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I'm just wondering if that suggests we could make this e.g. os/unix/environment/sysctl

Copy link
Contributor Author

@lucab lucab Aug 19, 2016

Choose a reason for hiding this comment

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

I'm more or less neutral here and your scoping also makes sense (maybe dropping the environment part). Just for future-proofing: we may have a posix-limits and a linux-limits upcoming after this, how would you scope those too in your mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lucab I would have expected them to be os/linux/limits and os/posix/limits or similar, per the existing ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack, I guess we can settle on os/unix/sysctl - os/unix/limits - os/linux/limits. This is aligned with current naming trend.

@jonboulle
Copy link
Contributor

bump - this still needs a rework right?

@lucab
Copy link
Contributor Author

lucab commented Aug 23, 2016

It does, plus a decision on #647 (comment)

@lucab lucab force-pushed the to-upstream/sysctl branch 2 times, most recently from 2f3b574 to c76acf6 Compare August 25, 2016 16:30
@lucab lucab changed the title spec: add environment/sysctl isolator spec: add os/unix/sysctl isolator Aug 25, 2016
lucab added 2 commits August 25, 2016 16:32
This commit introduces an "os/unix/sysctl" isolator, which
applies at pod-level.
@lucab lucab force-pushed the to-upstream/sysctl branch from c76acf6 to a00638c Compare August 25, 2016 16:32
@lucab
Copy link
Contributor Author

lucab commented Aug 25, 2016

Second iteration of this: renamed isolator to os/unix/sysctl and reworded the key validation note. PTAL.

@jonboulle
Copy link
Contributor

LGTM
@vbatts any comments?

@euank
Copy link
Contributor

euank commented Aug 26, 2016

LGTM as well

@jonboulle jonboulle merged commit af31bda into appc:master Aug 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants