update docker in Fedora to not read os-release in %posttrans #10
Comments
|
draft patch from colin: From 68e598f8bcd5bbd202a2f2e6c87b7d4ee03020a3 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Thu, 28 Jun 2018 18:46:57 +0000
Subject: [PATCH] wip
---
docker.spec | 80 ++---------------------------------------------------
1 file changed, 2 insertions(+), 78 deletions(-)
diff --git a/docker.spec b/docker.spec
index 8c336d9..7195f91 100644
--- a/docker.spec
+++ b/docker.spec
@@ -11,13 +11,6 @@
# modifying the dockerinit binary breaks the SHA1 sum check by docker
%global __os_install_post %{_rpmconfigdir}/brp-compress
-# default overlay2 storage only on Fedora 26 and later
-%if 0%{?fedora} >= 26 || 0%{?rhel} > 7
-%global custom_storage 1
-%else
-%global custom_storage 0
-%endif
-
%if 0%{?centos}
%global with_migrator 0
%endif
@@ -100,7 +93,7 @@ Name: %{repo}
Epoch: 2
%endif
Version: 1.13.1
-Release: 59.git%{shortcommit_docker}%{?dist}
+Release: 99.storage.git%{shortcommit_docker}%{?dist}
Summary: Automates deployment of containerized applications
License: ASL 2.0
URL: https://%{provider}.%{provider_tld}/projectatomic/%{repo}
@@ -225,13 +218,6 @@ Requires: criu
Requires: oci-umount >= 2:2.0.0-1
-%if %{custom_storage}
-Provides: variant_config(Atomic.host)
-Provides: variant_config(Cloud)
-Provides: variant_config(Server)
-Provides: variant_config(Workstation)
-%endif # custom_storage
-
%description
Docker is an open-source engine that automates the deployment of any
application as a lightweight, portable, self-sufficient container that will
@@ -530,25 +516,7 @@ cp %{SOURCE9} .
# untar d-s-s
tar zxf %{SOURCE1}
pushd container-storage-setup-%{commit_dss}
-%if %{custom_storage}
-# create default override config
ln -s %{repo}-storage-setup-override.conf %{repo}-storage-setup-default
-# create workstation override config
-cp %{repo}-storage-setup-override.conf %{repo}-storage-setup-workstation
-echo 'STORAGE_DRIVER=overlay2' >> %{repo}-storage-setup-workstation
-# create cloud override config
-ln -s %{repo}-storage-setup-workstation %{repo}-storage-setup-cloud
-# create server override config
-ln -s %{repo}-storage-setup-workstation %{repo}-storage-setup-server
-# create atomic override config; see https://pagure.io/atomic-wg/issue/281
-%if 0%{?fedora} >= 27 || 0%{?rhel} > 7
-ln -s %{repo}-storage-setup-workstation %{repo}-storage-setup-atomichost
-%else
-cp %{repo}-storage-setup-server %{repo}-storage-setup-atomichost
-echo 'CONTAINER_ROOT_LV_NAME=docker-root-lv' >> %{repo}-storage-setup-atomichost
-echo 'CONTAINER_ROOT_LV_MOUNT_PATH=/var/lib/docker' >> %{repo}-storage-setup-atomichost
-%endif #atomichost
-%endif # custom_storage
popd
# untar docker-novolume-plugin
@@ -789,14 +757,8 @@ install -p -m 644 %{SOURCE11} %{buildroot}%{_sysconfdir}/%{name}
# install d-s-s
pushd container-storage-setup-%{commit_dss}
make install-docker DESTDIR=%{buildroot}
-%if %{custom_storage}
install -dp %{buildroot}%{dss_datadir}
-install -p -m 644 %{repo}-storage-setup-atomichost %{buildroot}%{dss_datadir}
-install -p -m 644 %{repo}-storage-setup-cloud %{buildroot}%{dss_datadir}
-install -p -m 644 %{repo}-storage-setup-server %{buildroot}%{dss_datadir}
-install -p -m 644 %{repo}-storage-setup-workstation %{buildroot}%{dss_datadir}
-install -p -m 644 %{repo}-storage-setup-default %{buildroot}%{dss_datadir}
-%endif # custom_storage
+install -p -m 644 %{repo}-storage-setup-default %{buildroot}%{sysconfdir}/sysconfig/%{name}-storage-setup
popd
# install %%{_bindir}/%%{name}
@@ -885,34 +847,6 @@ install -p -m 644 %{repo}-lvm-plugin-%{commit_lvm}%{_sysconfdir}/%{repo}/%{repo}
%postun rhel-push-plugin
%systemd_postun_with_restart rhel-push-plugin.service
-%if %{custom_storage}
-%posttrans
-# If we don't yet have a symlink or existing file for
-# %%{name}-storage-setup.conf, create it.
-if [ ! -e %{_sysconfdir}/sysconfig/%{name}-storage-setup ]; then
- # Import /etc/os-release to get the variant definition
- . %{_sysconfdir}/os-release || :
-
- case "$VARIANT_ID" in
- atomic.host)
- cp %{dss_datadir}/%{name}-storage-setup-atomichost %{_sysconfdir}/sysconfig/%{name}-storage-setup || :
- ;;
- cloud)
- cp %{dss_datadir}/%{name}-storage-setup-cloud %{_sysconfdir}/sysconfig/%{name}-storage-setup || :
- ;;
- server)
- cp %{dss_datadir}/%{name}-storage-setup-server %{_sysconfdir}/sysconfig/%{name}-storage-setup || :
- ;;
- workstation)
- cp %{dss_datadir}/%{name}-storage-setup-workstation %{_sysconfdir}/sysconfig/%{name}-storage-setup || :
- ;;
- *)
- cp %{dss_datadir}/%{name}-storage-setup-default %{_sysconfdir}/sysconfig/%{name}-storage-setup || :
- ;;
- esac
-fi
-%endif # custom_storage
-
%if 0%{?with_migrator}
%triggerin -n %{repo}-v1.10-migrator -- %{repo} < %{version}
%{_bindir}/v1.10-migrator-local 2>/dev/null
@@ -940,17 +874,7 @@ exit 0
%{_udevrulesdir}/80-%{repo}.rules
%{_sysconfdir}/%{repo}
# d-s-s specific
-%if %{custom_storage}
-#ghost+config causes the file to stay after uninstalling the package and we do not want that
-%ghost %{_sysconfdir}/sysconfig/%{name}-storage-setup
-%config(noreplace) %{dss_datadir}/%{name}-storage-setup-atomichost
-%config(noreplace) %{dss_datadir}/%{name}-storage-setup-cloud
-%config(noreplace) %{dss_datadir}/%{name}-storage-setup-server
-%config(noreplace) %{dss_datadir}/%{name}-storage-setup-workstation
-%config(noreplace) %{dss_datadir}/%{name}-storage-setup-default
-%else # custom_storage
%config(noreplace) %{_sysconfdir}/sysconfig/%{name}-storage-setup
-%endif # custom_storage
%{_unitdir}/%{repo}-storage-setup.service
%{_bindir}/%{repo}-storage-setup
# >= 1.11 specific
--
2.17.1
|
|
I think we should not rely on the fact that our current defaults are same for all products so let us not read /etc/os-release. We introduced this for a reason. And our per product defaults were different and now they are same. What if tomorrow somebody comes back saying we need different default for a specific product and then we have no way to do that. Instead we need to fix the ordering issue. IIUC, you are saying that some other rpm puts /etc/os-release in %posttrans section and our posttrans section might run before these? Can docker rpm say "Requires:foo-rpm-providing-os-release" and will that make sure docker rpm's %postttrans will run after foo-rpm-providing-os-release's %posttrans. Or something else. IOW, I think we should fix ordering issue instead of stop parsing /etc/os-release. There are other instances in rhel rpm where we parse /etc/os-release to parse "release" and change defaults on the fly. So there is a value in fixing ordering in general. |
Broadly speaking our primary goal is to have users configure the system downstream. For anyone making Fedora derived products, IMO the sanest way is to have a "postprocessing" step that runs after everything else, like rpm-ostree and Kickstart support, rather than trying to wedge one's configuration into the "base" set itself. It's rarely worth it. Concretely anyone who wants to make ExampleAwesomeOS derived from Fedora CoreOS could just add a bit to the rpm-ostree (I can't imagine why someone would want a non-overlayfs default today anyways) |
I think this is valid and we should probably try to understand the ordering issue too. @cgwalters do you have any ideas here?
I agree, which is why the easy answer was to just get rid of docker parsing |
It does look like a But I still think all of this code was probably a mistake from the start, it's very complex and fragile. If I was doing it again I'd have just changed the Atomic Host builds to tweak the conf file in the rpm-ostree postprocess phase. And we don't need it anymore, and I would bet nontrivial amounts of money on no one else needing it. |
|
@cgwalters so how does a fedora OS provide per product default. I just want to make sure that we do one thing. Either we say per product defaults is a bad idea and we will not support it. Or we provide a way to support that even in coreos framework. Relying on the fact that current docker rpm does not need is not a very good idea, IMHO. |
|
For the question why would somebody want anything other than "overlay2", Giuseppe is writing overlay-fuse driver which will provide unprivileged mounts and possibly shifting. May be fedora will want to experiment with that as default driver on coreos/atomic (if results are good). So saying that will never need per product default, is not a very good answer. |
|
It's interesting to note that https://fedoraproject.org/wiki/Sgallagh:Per-Product_Configuration encourages differences across products to be implemented using That said, given that RHEL7 is now overlay2 by default and Fedora 25 is long EOL, |
As far as I know the other canonical example of this whole "read the os-release file and change something" is firewalld having Server/Workstation variants. It might be installs were just getting lucky with this so far? Or it might be something else is going wrong.
As I mentioned above, I would probably just do this in the |
|
@jlebon, RHEL7 will continue to have to parse /etc/os-release because we don't want to default to overlay2 for older kernel releases as older kernel did not have that good overlay2 support. For fedora it is easier as it is not expected to run on old kernels. So cleaning up all that code makes sense to me now. If we need per product defaults again, we will have to introduce it again, and this time introduce this dependency on os-release package as well. I think getting rid of that custom_storage is fine. Assuming that we are not expecting to run on older fedora kernels or rhel systems. Is that a fair assumption? I am not sure why that comparision "rhel > 7" is there to begin with. Did soebody expect fedora rpms to be installed on rhel systems? |
Hmm, though the conditionals don't check for minor releases -- or do you mean RHEL6? We don't support docker on RHEL6 there anyway :) I also mentioned in the PR that IIUC, older systems shouldn't suddenly be migrated to overlay2 now that c-s-s just no-ops if storage is already configured for a different backend, right?
Yeah, I think so.
I'm guessing it was kept generic so it could be used to build either Fedora or RHEL/CentOS RPMs? E.g. for ostree/rpm-ostree, we build git master for CentOS using the same spec file as Fedora. Though the official downstream builds have separate dist-gits with their own spec files of course. |
|
I did not mean RHEL6. I meant checking for minor releases. We are installing overlay2 default on rhel I think only 7.4 onwards. Upstream make install-docker will not overwrite existing /etc/sysconfig/docker-storage-setup. So upgrade on regular systems should be fine. But I think atomic upgrade can be an issue if previous releases shipped devicemapper as default. I think users already ran into that issue when we switched default to "overlay2" with atomic. So this is not going to open any new cases yet. But if we decide to switch our default again to say overlay-foo or add something else to the file, it will be broken again with coreos/projectatomic over upgrade. But that's a separate problem anyway. |
|
The https://src.fedoraproject.org/rpms/docker/pull-request/9 PR merged. Any remaining discussion needed here? |
|
It depends if we want to use this ticket to track the patch until it makes it into F28. I'm fine either way. |
|
FWIW I verify that an rpm built with this patch gets around my original problem where there was an ordering issue in posttrans. |
|
F28 version of the patch: https://src.fedoraproject.org/rpms/docker/pull-request/10. |
|
The ordering issue in %posttrans was probably due to switching away from using fedora-release-atomichost, which would have properly generated /etc/os-release in %post. Fedora-release only waits until %posttrans to handle non-edition cases. So if you want to avoid this ordering issue in the future, you should create fedora-release-coreos and have it drop the appropriate /etc/os-release in place during its %post. |
|
|
i think this can be closed since we have a fedora-release-coreos rpm now |
It tries to read /etc/os-release which is also updated in %posttrans so we have an ordering problem. This can be avoided since docker reading /etc/os-release is not really needed any longer (we have standardized storage configuration).
The text was updated successfully, but these errors were encountered: