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

add config with no exists config #671

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

himrock922
Copy link

@himrock922 himrock922 commented Feb 23, 2019

ref #630

I commit of parameter that was said "unknown" with config.
For example, `The config property 'vnet_default_interface' is unknown`
Because I don't know very well about config detail, I need refactoring it.

This PR related to issue #626

Hi All,
I'm sorry, after rebase execute, confflict code(like as <<<HEAD, ======) mixed PR(Also, reflog failed). So, recreate branch...

Make sure to follow and check these boxes before submitting a PR! Thank you.

@gronke
Copy link
Member

gronke commented Feb 23, 2019

Hi All,
I'm sorry, after rebase execute, confflict code(like as <<<HEAD, ======) mixed PR(Also, reflog failed). So, recreate branch...

No worries, your set of changes is great to spot the differences to other variants.

Adding the values to the defaults has the effect that ioc does no longer complain about unknown JailConfig properties, but they will just be ignored when processing a jail. There are the cases:

  1. A feature is missing in libioc
    1. feature needs to be implemented
    2. config property can be added to Globals
  2. An iocage feature is unfortunately named
    1. the value can be mapped to a more reasonable internal name
    2. mapped name is added to the Globals
  3. iocage duplicates a feature with other name
    1. ignore the value on read (maybe verify with the other settings, but still ignore it)
    2. calculate the value on configuration write, so that the result remains compatible
  4. The iocage feature has design flaws
    1. will not be implemented
    2. warn users with error
    3. a --ignore-compat flag (or similar) can turn the error (2) into a warning and continue

As one with the ideas from your other branch, @himrock922, I'd like to derive more Issues from each individual change you suggest to the Globals. Doing so, while following the workflow described above, will lead to a user friendly solution with minimal breakage for iocage users. (iocage_legacy should not be affected).

@himrock922 himrock922 force-pushed the feature/add_config_after_migrate_from_globals_to_defaults branch from 1e65386 to 22d2592 Compare February 24, 2019 13:16
allow_mlock propetry changes from 0 to None.
ref2 https://www.freebsd.org/cgi/man.cgi?jail(8)
enforce_staffs property equal 2 So, set allow_mount_fusefs values False(0)
Copy link
Member

@gronke gronke left a comment

Choose a reason for hiding this comment

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

Hi @himrock922, the comments I left are a start to implement your suggested changes. Some changes require new features to be implemented, which you can find linked in my comments. If you would create one PR per feature, we can individually discuss and test the implications for libioc.

@@ -67,7 +67,7 @@
"allow_mlock": None,
"allow_mount": 0,
"allow_mount_devfs": 0,
"allow_mount_fusefs": 0,
"allow_mount_fusefs": False,
Copy link
Member

Choose a reason for hiding this comment

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

Interesting idea to actually turn the allow_mount_* properties into booleans. The sysctl value is an integer type, but there is a translation of boolean values happening. We should just be consistent and change similar properties among this one.

Copy link
Author

Choose a reason for hiding this comment

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

OK. I think should keep a consistency for code(For example, 0 or 1 integer only used, or boolean type only used).
If possible, To refactoring for future, I think should only use boolean type.

Copy link
Author

Choose a reason for hiding this comment

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

Anyway, I do fix such allow_mount.* parameters from 0,1 to True, False.

libioc/Config/Jail/Globals.py Outdated Show resolved Hide resolved
"bpf": None,
"comment": None,
"compression": None,
"compressratio": None,
Copy link
Member

Choose a reason for hiding this comment

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

This is not a property that can be set. It should not be in the jail configuration at all. Is that the case in python-iocage?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, such parameters using with python-iocage.
https://www.freebsd.org/cgi/man.cgi?query=iocage&sektion=8

     available	   Available space in the jail's dataset.  The amount of space
		   available to	the dataset and	all its	children, assuming
		   that	there is no other activity in the pool.	 Because space
		   is shared within a pool, availability can be	limited	by any
		   number of factors, including	physical pool size, quotas,
		   reservations, or other datasets within the pool.

		   Source: zfs(8)
  • bpf

https://github.com/iocage/iocage/blob/528f007e92f6e081614d8a09186c4831fc9f3e32/iocage.8#L1019

Toggle starting the jail with Berkely Packet Filter devices enabled.
  • compression

https://github.com/iocage/iocage/blob/528f007e92f6e081614d8a09186c4831fc9f3e32/iocage.8#L1724

Controls the compression algorithm used for this dataset.
The lzjb compression algorithm is optimized for performance while
providing decent data compression.
Setting compression to on uses the lzjb compression algorithm.
The gzip algorithm uses the same compression as the
  • compressratio

https://github.com/iocage/iocage/blob/528f007e92f6e081614d8a09186c4831fc9f3e32/iocage.8#L1789

Compression ratio.
Read-only.
For non-snapshots, the compression ratio achieved for the used space
of this dataset, expressed as a multiplier.
The used property includes descendant datasets, and, for clones, does
not include the space shared with the origin snapshot.

Also, Also, comment property setting a lower layer for user property.

ref #630 (comment)

For example

user {
  comment: None,
  notes: None
}

"compression": None,
"compressratio": None,
"count": None,
"cpuset": False,
Copy link
Member

Choose a reason for hiding this comment

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

There is a new feature suggested in #636 - this change should be handled in a separate PR that addresses the GitHub Issue.

"provision": {
"method": None,
"source": None,
"rev": "master"
}
},
"last_started": None
Copy link
Member

Choose a reason for hiding this comment

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

Missing feature tracked in #632 - this change belongs in a separate PR.

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.

None yet

2 participants