-
Notifications
You must be signed in to change notification settings - Fork 353
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
Master enable ostree containers #4561
Changes from all commits
51b7b14
3a316f6
db9e3e6
86047da
200cc07
8260a9c
377f54a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
:Type: Kickstart | ||
:Summary: Add support for OSTree native containers (#2125655) | ||
|
||
:Description: | ||
Fedora is adding a new enhanced container support for the (rpm-)ostree stack to | ||
natively support OCI/Docker containers as a transport and delivery mechanism | ||
for operating system content. Anaconda now supports these containers by | ||
a new kickstart command `ostreecontainer`. | ||
|
||
:Links: | ||
- https://bugzilla.redhat.com/show_bug.cgi?id=2125655 | ||
- https://fedoraproject.org/wiki/Changes/OstreeNativeContainer | ||
- https://fedoraproject.org/wiki/Changes/OstreeNativeContainerStable | ||
- https://pykickstart.readthedocs.io/en/latest/kickstart-docs.html#ostreecontainer |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,6 +54,60 @@ def safe_exec_with_redirect(cmd, argv, successful_return_codes=(0,), **kwargs): | |
) | ||
|
||
|
||
def _get_ref(data): | ||
"""Get ref or name based on source. | ||
|
||
OSTree container don't have ref because it's specified by the container. In that case let's | ||
return just url for reporting. | ||
|
||
:param data: OSTree source structure | ||
:return str: ref or name based on source | ||
""" | ||
# Variable substitute the ref: https://pagure.io/atomic-wg/issue/299 | ||
if data.is_container(): | ||
# we don't have ref with container; there are not multiple references in one container | ||
return data.url | ||
else: | ||
return RpmOstree.varsubst_basearch(data.ref) | ||
Comment on lines
+66
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment is above both calls, so perhaps you should substitute also for the url? |
||
|
||
|
||
def _get_stateroot(data): | ||
"""Get stateroot. | ||
|
||
The OSTree renamed old osname to stateroot for containers. | ||
|
||
:param data: OSTree source structure | ||
:return str: stateroot or osname value based on source | ||
""" | ||
if data.is_container(): | ||
# osname was renamed to stateroot so let's use the new name | ||
if data.stateroot: | ||
return data.stateroot | ||
else: | ||
# The stateroot doesn't have to be defined | ||
# https://github.com/ostreedev/ostree-rs-ext/pull/462/files | ||
# However, it's working just for a subset of calls now. | ||
# TODO: Remove this when all ostree commands undestarstands this | ||
return "default" | ||
else: | ||
return data.osname | ||
Comment on lines
+74
to
+93
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of this code simply does not have to exist. I think you should have renamed one of these fields so that they match. Especially when the first thing "stateroot" docstring says is that it's the osname... |
||
|
||
|
||
def _get_verification_enabled(data): | ||
"""Find out if source has enabled verification. | ||
|
||
OSTree sources has different names for enabled verification. This helper function | ||
will make the access consistent. | ||
|
||
:param data: OSTree source structure | ||
:return bool: True if verification is enabled | ||
""" | ||
if data.is_container(): | ||
return data.signature_verification_enabled | ||
else: | ||
return data.gpg_verification_enabled | ||
Comment on lines
+105
to
+108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto here. If it's the same thing, just call it the same. The docstrings also say that it's just gpg now, so why did we have to invent a new name? |
||
|
||
|
||
class PrepareOSTreeMountTargetsTask(Task): | ||
"""Task to prepare OSTree mount targets.""" | ||
|
||
|
@@ -132,7 +186,10 @@ def _handle_var_mount_point(self, existing_mount_points): | |
|
||
:param [] existing_mount_points: a list of existing mount points | ||
""" | ||
var_root = '/ostree/deploy/' + self._data.osname + '/var' | ||
# osname was used for ostreesetup but ostreecontainer renamed it to stateroot | ||
stateroot = _get_stateroot(self._data) | ||
|
||
var_root = '/ostree/deploy/' + stateroot + '/var' | ||
if existing_mount_points.get("/var") is None: | ||
self._setup_internal_bindmount(var_root, dest='/var', recurse=False) | ||
else: | ||
|
@@ -397,11 +454,14 @@ def run(self): | |
# We don't support resuming from interrupted installs. | ||
repo.set_disable_fsync(True) | ||
|
||
# Remote is set or it should be named as stateroot is | ||
remote = self._data.remote or _get_stateroot(self._data) | ||
|
||
# Add a remote if it doesn't exist. | ||
repo.remote_change( | ||
sysroot_file if self._sysroot else None, | ||
OSTree.RepoRemoteChange.ADD_IF_NOT_EXISTS, | ||
self._data.remote, | ||
remote, | ||
self._data.url, | ||
self._get_remote_options(), | ||
cancellable=None | ||
|
@@ -411,7 +471,7 @@ def _get_remote_options(self): | |
"""Get the remote options.""" | ||
remote_options = {} | ||
|
||
if not self._data.gpg_verification_enabled: | ||
if not _get_verification_enabled(self._data): | ||
remote_options['gpg-verify'] = Variant('b', False) | ||
|
||
if not conf.payload.verify_ssl: | ||
|
@@ -486,8 +546,8 @@ class DeployOSTreeTask(Task): | |
def __init__(self, data, physroot): | ||
"""Create a new task. | ||
|
||
:param str physroot: a path to the physical root | ||
:param data: an RPM OSTree configuration | ||
:param str physroot: a path to the physical root | ||
""" | ||
super().__init__() | ||
self._data = data | ||
|
@@ -498,8 +558,8 @@ def name(self): | |
return "Deploy OSTree" | ||
|
||
def run(self): | ||
# Variable substitute the ref: https://pagure.io/atomic-wg/issue/299 | ||
ref = RpmOstree.varsubst_basearch(self._data.ref) | ||
ref = _get_ref(self._data) | ||
stateroot = _get_stateroot(self._data) | ||
|
||
self.report_progress(_("Deployment starting: {}").format(ref)) | ||
|
||
|
@@ -508,19 +568,37 @@ def run(self): | |
["admin", | ||
"--sysroot=" + self._physroot, | ||
"os-init", | ||
self._data.osname] | ||
stateroot] | ||
) | ||
|
||
log.info("ostree admin deploy starting") | ||
if self._data.is_container(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This part and below feel as if you forgot that polymorphism exists? |
||
log.info("ostree image deploy starting") | ||
|
||
safe_exec_with_redirect( | ||
"ostree", | ||
["admin", | ||
"--sysroot=" + self._physroot, | ||
"deploy", | ||
"--os=" + self._data.osname, | ||
self._data.remote + ':' + ref] | ||
) | ||
args = ["container", "image", "deploy", | ||
"--sysroot=" + self._physroot, | ||
"--image=" + ref] | ||
|
||
if self._data.transport: | ||
args.append("--transport=" + self._data.transport) | ||
if self._data.stateroot: | ||
args.append("--stateroot=" + self._data.stateroot) | ||
if not self._data.signature_verification_enabled: | ||
args.append("--no-signature-verification") | ||
VladimirSlavik marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
safe_exec_with_redirect( | ||
"ostree", | ||
args | ||
) | ||
else: | ||
log.info("ostree admin deploy starting") | ||
safe_exec_with_redirect( | ||
"ostree", | ||
["admin", | ||
"--sysroot=" + self._physroot, | ||
"deploy", | ||
"--os=" + stateroot, | ||
self._data.remote + ':' + ref] | ||
) | ||
|
||
log.info("ostree config set sysroot.readonly true") | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You extended this class so it's the same as the new class... but then made it different anyway?