Bug 1630824 - engine-backup should backup ovirt-provider-ovn
Summary: engine-backup should backup ovirt-provider-ovn
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Virtualization Manager
Classification: Red Hat
Component: ovirt-engine
Version: 4.2.5
Hardware: Unspecified
OS: Unspecified
high
high
Target Milestone: ovirt-4.3.5
: 4.3.5
Assignee: Yedidyah Bar David
QA Contact: Petr Matyáš
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-09-19 11:13 UTC by Marian Jankular
Modified: 2021-12-10 17:40 UTC (History)
10 users (show)

Fixed In Version: ovirt-engine-4.3.5
Doc Type: Bug Fix
Doc Text:
In this release, engine-backup correctly backs up and restores configuration and data files of Open vSwitch and ovirt-provider-ovn.
Clone Of:
Environment:
Last Closed: 2019-08-12 11:53:27 UTC
oVirt Team: Integration
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Issue Tracker RHV-44281 0 None None None 2021-12-10 17:40:25 UTC
Red Hat Product Errata RHEA-2019:2431 0 None None None 2019-08-12 11:53:39 UTC
oVirt gerrit 98657 0 master MERGED packaging: engine-backup: Handle OVN/OVS 2020-08-04 09:08:21 UTC
oVirt gerrit 99768 0 ovirt-engine-4.3 MERGED packaging: engine-backup: Handle OVN/OVS 2020-08-04 09:08:21 UTC

Description Marian Jankular 2018-09-19 11:13:25 UTC
Description of problem:
engine-backup should backup ovirt-provider-ovn 

Version-Release number of selected component (if applicable):
ovirt-provider-ovn-1.2.14-1.el7ev.noarch
ovirt-engine-tools-backup-4.2.6.4-0.1.el7ev.noarch

How reproducible:
always

Steps to Reproduce:
1. setup manager with ovirt-provider-ovn hosted on manager
2. take engine-backup
3. restore it to new server, run engine-setup


Actual results:
ovirt-provider-ovn config file is missing in backup (/etc/ovirt-provider-ovn/conf.d/10-setup-ovirt-provider-ovn.conf) 
even if custmer make ovirt-provider-ovn work again, openvswitch conf.db is missing so customer have to recreate all the networks from the scratch

Expected results:
engine-backup will backup ovirt-provider-ovn config file all the time
engine-backup will backup openvswitch conf.db at least when it is hosted on manager itself

Additional info:

Comment 1 Sandro Bonazzola 2018-12-13 12:52:13 UTC
Postponing to 4.3.0 not being identified as blocker for 4.2.8

Comment 3 Sandro Bonazzola 2019-02-18 07:54:53 UTC
Moving to 4.3.2 not being identified as blocker for 4.3.1.

Comment 4 Yedidyah Bar David 2019-03-05 08:23:43 UTC
What exact files should be backed up and restored?

We allow restoring a backup taken with a different, even older, .z version - e.g. 4.3.0 can restore a backup taken in 4.3.1. In the past we also allowed restoring on 4.0 a backup taken in 3.6, for upgrading the engine machine from el6 to el7. We might do this in the future too.

All of /etc/ovirt-provider-ovn ?

/etc/ovirt-engine/ovirt-provider-ovn-conf.example is already handled (all of /etc/ovirt-engine)

/etc/firewalld/services/ovn-central-firewall-service.xml ?

/etc/firewalld/services/ovirt-provider-ovn.xml ?

Not sure we must, engine-setup should recreate I think. But we do handle other firewalld service files, so I guess these too

I now realized that we do not backup/restore anything in /etc/logrotate.d . Not sure that's a bug, and if it is, it's not specific to ovn. Will ignore for now.

systemd? We do not backup currently, and assume that engine-setup will enable/start services. Need to verify this.

All of /var/lib/openvswitch ?

Comment 5 Yedidyah Bar David 2019-03-05 08:28:25 UTC
Also: Do we want to patch engine-backup? engine-backup also allows adding shell snippets to be sourced in /etc/ovirt-engine-backup, so we can in principle push a patch to ovn to add there stuff. This will be useful if you expect to have more/different files in the future that need to be backed up.

Comment 6 Marcin Mirecki 2019-03-05 09:20:40 UTC
ovirt-provider-ovn config:
/etc/ovirt-provider-ovn/ovirt-provider-ovn.conf
/etc/ovirt-provider-ovn/conf.d/*

ovirt-provider-ovn is statless, so there is no data to backup. The state is in ovs, so we should backup the ovs databases:
/var/lib/openvswitch/ovnnb_db.db
/var/lib/openvswitch/ovnsb_db.db
or maybe the whole dir: /var/lib/openvswitch/*?

service firewall (as above)

Do we want to backup PKI?

Comment 7 Yedidyah Bar David 2019-03-05 09:33:48 UTC
(In reply to Marcin Mirecki from comment #6)
> ovirt-provider-ovn config:
> /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf
> /etc/ovirt-provider-ovn/conf.d/*

Any problem handling the entire directory /etc/ovirt-provider-ovn ?

> 
> ovirt-provider-ovn is statless, so there is no data to backup. The state is
> in ovs, so we should backup the ovs databases:
> /var/lib/openvswitch/ovnnb_db.db
> /var/lib/openvswitch/ovnsb_db.db
> or maybe the whole dir: /var/lib/openvswitch/*?

Yes, is that ok?

> 
> service firewall (as above)

ok

> 
> Do we want to backup PKI?

We handle all of /etc/pki/ovirt-engine but exclude these:

/etc/pki/ovirt-engine/cacert.template.in
/etc/pki/ovirt-engine/cert.template.in
/etc/pki/ovirt-engine/openssl.conf"

because they are packaged and should be handled by the package manager, see bug 1452182.

When answering "is it ok to handle entire directory?", consider what files you might want to add there in the future. If it's more likely that we'll want to handle them, handle the entire dir. Otherwise, handle specific files.

I keep writing "handle" and not "backup", because it's extremely important to realize that we backup (that's easy), but also later on might restore. Restore is hard. It's done under pressure, and should be as quick and riskless as possible. If during restore we overwrite a packaged file, this will go unnoticed, and in principle can cause hard-to-debug failures.

Comment 8 Marcin Mirecki 2019-03-18 08:54:46 UTC
(In reply to Yedidyah Bar David from comment #7)
> (In reply to Marcin Mirecki from comment #6)
> > ovirt-provider-ovn config:
> > /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf
> > /etc/ovirt-provider-ovn/conf.d/*
> 
> Any problem handling the entire directory /etc/ovirt-provider-ovn ?

Thinking about this a little more.

/etc/ovirt-provider-ovn/ovirt-provider-ovn.conf  -- this should be the default config, one not modified by the user. Any changes should be made in the conf.d directory. Hence I'm not quite sure we should backup it (it could break things if newer versions introduce new config values for example)

/etc/ovirt-provider-ovn/conf.d/*  -- contains all the custom config which overrides the values defined in the default config. We only look at *.conf files, and I don't really expect anything else to be present there.
We could backup /etc/ovirt-provider-ovn/conf.d/*.conf only.



> 
> > 
> > ovirt-provider-ovn is statless, so there is no data to backup. The state is
> > in ovs, so we should backup the ovs databases:
> > /var/lib/openvswitch/ovnnb_db.db
> > /var/lib/openvswitch/ovnsb_db.db
> > or maybe the whole dir: /var/lib/openvswitch/*?
> 
> Yes, is that ok?

I'll check with the ovs team if it's safe to overwrite the whole dir.


> 
> > 
> > service firewall (as above)
> 
> ok

We would only need this if the user does any customizations to it.
Otherwise it should be recreated on reinstalling.

> 
> > 
> > Do we want to backup PKI?
> 
> We handle all of /etc/pki/ovirt-engine but exclude these:
> 
> /etc/pki/ovirt-engine/cacert.template.in
> /etc/pki/ovirt-engine/cert.template.in
> /etc/pki/ovirt-engine/openssl.conf"
> 
> because they are packaged and should be handled by the package manager, see
> bug 1452182.
> 
> When answering "is it ok to handle entire directory?", consider what files
> you might want to add there in the future. If it's more likely that we'll
> want to handle them, handle the entire dir. Otherwise, handle specific files.
> 
> I keep writing "handle" and not "backup", because it's extremely important
> to realize that we backup (that's easy), but also later on might restore.
> Restore is hard. It's done under pressure, and should be as quick and
> riskless as possible. If during restore we overwrite a packaged file, this
> will go unnoticed, and in principle can cause hard-to-debug failures.

Comment 9 Yedidyah Bar David 2019-03-18 09:56:41 UTC
(In reply to Marcin Mirecki from comment #8)
> (In reply to Yedidyah Bar David from comment #7)
> > (In reply to Marcin Mirecki from comment #6)
> > > ovirt-provider-ovn config:
> > > /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf
> > > /etc/ovirt-provider-ovn/conf.d/*
> > 
> > Any problem handling the entire directory /etc/ovirt-provider-ovn ?
> 
> Thinking about this a little more.
> 
> /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf  -- this should be the
> default config, one not modified by the user. Any changes should be made in
> the conf.d directory. Hence I'm not quite sure we should backup it (it could
> break things if newer versions introduce new config values for example)

In the spec file, you have:

%config(noreplace) %{_sysconfdir}/ovirt-provider-ovn

I admit it's the first time I see such a thing - a directory (not a file) set as %config(noreplace). Searched a bit and couldn't find how this should behave. You might want to file a bug about this. If this behaves as "recursively handle everything underneath", then your assumption above (which might also be official documentation, didn't check) might break. In any case, if you indeed open a bug (and even if not), you should probably at least add a comment inside it saying that it should not be edited.

Considering the above, not sure what to do :-(. I think it's best to backup and restore everything. If we want to do that, we should commit to not change the file inside z-stream (as restore refuses to restore a backup taken with a different x.y version).

> 
> /etc/ovirt-provider-ovn/conf.d/*  -- contains all the custom config which
> overrides the values defined in the default config. We only look at *.conf
> files, and I don't really expect anything else to be present there.

Most likely, (some) people that update these files also keep there backups, and imo we should backup/restore these backups as well.

> We could backup /etc/ovirt-provider-ovn/conf.d/*.conf only.

See above. What do you think?

IMO:

1. Currently, backup/restore all of /etc/ovirt-provider-ovn
2. If needed, consider saving in the backup file the exact version of ovs/ovn used during backup, and refuse, or at least warn/prompt, if trying to restore with a different version. If you you/we want that, please open another bug.
3. Open another bug (or a few) about '%config(noreplace) %{_sysconfdir}/ovirt-provider-ovn' - check what it does, decide if that's ok for us (and then officially document that), and if not, do something (and then document the change).
4. If needed, open another bug to refine the list of files to backup/restore.

> 
> 
> 
> > 
> > > 
> > > ovirt-provider-ovn is statless, so there is no data to backup. The state is
> > > in ovs, so we should backup the ovs databases:
> > > /var/lib/openvswitch/ovnnb_db.db
> > > /var/lib/openvswitch/ovnsb_db.db
> > > or maybe the whole dir: /var/lib/openvswitch/*?
> > 
> > Yes, is that ok?
> 
> I'll check with the ovs team if it's safe to overwrite the whole dir.

Thanks.

> 
> 
> > 
> > > 
> > > service firewall (as above)
> > 
> > ok
> 
> We would only need this if the user does any customizations to it.
> Otherwise it should be recreated on reinstalling.

Thinking about this, we indeed probably backup/restore the existing files mainly because we allow overriding some ports (although IIRC that's not documented):

packaging/firewalld 2430 $ grep -r '@' .
./vmconsole-proxy/ovirt-vmconsole-proxy.xml.in:  <port protocol="tcp" port="@VMCONSOLE_PROXY_PORT@"/>
./ovirt-common/ovirt-https.xml.in:  <port protocol="tcp" port="@HTTPS_PORT@"/>
./ovirt-common/ovirt-http.xml.in:  <port protocol="tcp" port="@HTTP_PORT@"/>
./websocket-proxy/ovirt-websocket-proxy.xml.in:  <port protocol="tcp" port="@WEBSOCKET_PROXY_PORT@"/>
./ovirt-engine/ovirt-jboss-https.xml.in:  <port protocol="tcp" port="@JBOSS_HTTPS_PORT@"/>
./ovirt-engine/ovirt-jboss-http.xml.in:  <port protocol="tcp" port="@JBOSS_HTTP_PORT@"/>

I think it would be best to backup/restore, even though engine-setup does not customize ovn firewalld service files. When user then runs engine-setup, we always ask again "Configure firewall?", and if user says "no", we should not touch these files (but I didn't verify that for ovn, although the code looks ok to me).

Comment 10 Marcin Mirecki 2019-03-26 11:29:43 UTC
(In reply to Yedidyah Bar David from comment #9)
> (In reply to Marcin Mirecki from comment #8)
> > (In reply to Yedidyah Bar David from comment #7)
> > > (In reply to Marcin Mirecki from comment #6)
> > > > ovirt-provider-ovn config:
> > > > /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf
> > > > /etc/ovirt-provider-ovn/conf.d/*
> > > 
> > > Any problem handling the entire directory /etc/ovirt-provider-ovn ?
> > 
> > Thinking about this a little more.
> > 
> > /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf  -- this should be the
> > default config, one not modified by the user. Any changes should be made in
> > the conf.d directory. Hence I'm not quite sure we should backup it (it could
> > break things if newer versions introduce new config values for example)
> 
> In the spec file, you have:
> 
> %config(noreplace) %{_sysconfdir}/ovirt-provider-ovn
> 
> I admit it's the first time I see such a thing - a directory (not a file)
> set as %config(noreplace). Searched a bit and couldn't find how this should
> behave. You might want to file a bug about this. If this behaves as
> "recursively handle everything underneath", then your assumption above
> (which might also be official documentation, didn't check) might break. In
> any case, if you indeed open a bug (and even if not), you should probably at
> least add a comment inside it saying that it should not be edited.
> 
> Considering the above, not sure what to do :-(. I think it's best to backup
> and restore everything. If we want to do that, we should commit to not
> change the file inside z-stream (as restore refuses to restore a backup
> taken with a different x.y version).

I don't think this was meant to work recursively.
{_sysconfdir}/ovirt-provider-ovn/ovirt-provider-ovn.conf is meant to be updated.
We do not expect to ever provide any files in the conf.d directory during updates,
so they do not need to be protected. 
I think the only purpose of this entry is to remove the config dir on rpm remove,
and it is not intended to protect any files inside the dir.

> 
> > 
> > /etc/ovirt-provider-ovn/conf.d/*  -- contains all the custom config which
> > overrides the values defined in the default config. We only look at *.conf
> > files, and I don't really expect anything else to be present there.
> 
> Most likely, (some) people that update these files also keep there backups,
> and imo we should backup/restore these backups as well.
> 
> > We could backup /etc/ovirt-provider-ovn/conf.d/*.conf only.
> 
> See above. What do you think

Any extra files in the directory do not cause any harm (the provider
will just ignore additional files).
If there is any value in backing them up, let's have them.


> 
> IMO:
> 
> 1. Currently, backup/restore all of /etc/ovirt-provider-ovn

Maybe /etc/ovirt-provider-ovn/conf.d?
Do we want to support edits in /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf?
This could lead to situations where a backup is applied on a newer version, but
we restore a file from a previous version. The provider will use default values
for missing config, but still this would be as if we sanctioned changing this file.

> 2. If needed, consider saving in the backup file the exact version of
> ovs/ovn used during backup, and refuse, or at least warn/prompt, if trying
> to restore with a different version. If you you/we want that, please open
> another bug.

Do we need this of ovs/ovn? Let me check with the ovn team again if replacing
the config with one of a different version could harm us.

> 3. Open another bug (or a few) about '%config(noreplace)

As stated above we only need this for cleanup during "rpm remove".


> %{_sysconfdir}/ovirt-provider-ovn' - check what it does, decide if that's ok
> for us (and then officially document that), and if not, do something (and
> then document the change).
> 4. If needed, open another bug to refine the list of files to backup/restore.
> 
> > 
> > 
> > 
> > > 
> > > > 
> > > > ovirt-provider-ovn is statless, so there is no data to backup. The state is
> > > > in ovs, so we should backup the ovs databases:
> > > > /var/lib/openvswitch/ovnnb_db.db
> > > > /var/lib/openvswitch/ovnsb_db.db
> > > > or maybe the whole dir: /var/lib/openvswitch/*?
> > > 
> > > Yes, is that ok?
> > 
> > I'll check with the ovs team if it's safe to overwrite the whole dir.
> 
> Thanks.

Overwriting the whole dir is ok (confirmed by nsiddique)

> 
> > 
> > 
> > > 
> > > > 
> > > > service firewall (as above)
> > > 
> > > ok
> > 
> > We would only need this if the user does any customizations to it.
> > Otherwise it should be recreated on reinstalling.
> 
> Thinking about this, we indeed probably backup/restore the existing files
> mainly because we allow overriding some ports (although IIRC that's not
> documented):
> 
> packaging/firewalld 2430 $ grep -r '@' .
> ./vmconsole-proxy/ovirt-vmconsole-proxy.xml.in:  <port protocol="tcp"
> port="@VMCONSOLE_PROXY_PORT@"/>
> ./ovirt-common/ovirt-https.xml.in:  <port protocol="tcp"
> port="@HTTPS_PORT@"/>
> ./ovirt-common/ovirt-http.xml.in:  <port protocol="tcp" port="@HTTP_PORT@"/>
> ./websocket-proxy/ovirt-websocket-proxy.xml.in:  <port protocol="tcp"
> port="@WEBSOCKET_PROXY_PORT@"/>
> ./ovirt-engine/ovirt-jboss-https.xml.in:  <port protocol="tcp"
> port="@JBOSS_HTTPS_PORT@"/>
> ./ovirt-engine/ovirt-jboss-http.xml.in:  <port protocol="tcp"
> port="@JBOSS_HTTP_PORT@"/>
> 
> I think it would be best to backup/restore, even though engine-setup does
> not customize ovn firewalld service files. When user then runs engine-setup,
> we always ask again "Configure firewall?", and if user says "no", we should
> not touch these files (but I didn't verify that for ovn, although the code
> looks ok to me).

Makes sense. Let's include it.

Comment 13 Yedidyah Bar David 2019-03-28 11:25:34 UTC
(In reply to Marcin Mirecki from comment #10)
> (In reply to Yedidyah Bar David from comment #9)
> > (In reply to Marcin Mirecki from comment #8)
> > > (In reply to Yedidyah Bar David from comment #7)
> > > > (In reply to Marcin Mirecki from comment #6)
> > > > > ovirt-provider-ovn config:
> > > > > /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf
> > > > > /etc/ovirt-provider-ovn/conf.d/*
> > > > 
> > > > Any problem handling the entire directory /etc/ovirt-provider-ovn ?
> > > 
> > > Thinking about this a little more.
> > > 
> > > /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf  -- this should be the
> > > default config, one not modified by the user. Any changes should be made in
> > > the conf.d directory. Hence I'm not quite sure we should backup it (it could
> > > break things if newer versions introduce new config values for example)
> > 
> > In the spec file, you have:
> > 
> > %config(noreplace) %{_sysconfdir}/ovirt-provider-ovn
> > 
> > I admit it's the first time I see such a thing - a directory (not a file)
> > set as %config(noreplace). Searched a bit and couldn't find how this should
> > behave. You might want to file a bug about this. If this behaves as
> > "recursively handle everything underneath", then your assumption above
> > (which might also be official documentation, didn't check) might break. In
> > any case, if you indeed open a bug (and even if not), you should probably at
> > least add a comment inside it saying that it should not be edited.
> > 
> > Considering the above, not sure what to do :-(. I think it's best to backup
> > and restore everything. If we want to do that, we should commit to not
> > change the file inside z-stream (as restore refuses to restore a backup
> > taken with a different x.y version).
> 
> I don't think this was meant to work recursively.

Perhaps it was not "meant", but I now verified and it does:
# rpm -qc ovirt-provider-ovn
/etc/logrotate.d/ovirt-provider-ovn
/etc/ovirt-provider-ovn/conf.d/README
/etc/ovirt-provider-ovn/logger.conf
/etc/ovirt-provider-ovn/ovirt-provider-ovn.conf

I also edited ovirt-provider-ovn.conf and updated the package, and the file was not changed back.

I also pushed https://gerrit.ovirt.org/99001 to see what happens when you update the file in the new version, and updated from the jenkins result, and got:

  Updating   : ovirt-provider-ovn-1.2.21-0.20190328105938.git34d44b5.el7.noarch                                                                                                          1/2 
warning: /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf created as /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf.rpmnew

> {_sysconfdir}/ovirt-provider-ovn/ovirt-provider-ovn.conf is meant to be
> updated.

I guess you meant "meant to be updated by the rpm package, not by users". But nothing prevent users from updating it, and if they do, if you have 'config(noreplace)', rpm will not overwrite their changed file.

> We do not expect to ever provide any files in the conf.d directory during
> updates,
> so they do not need to be protected. 
> I think the only purpose of this entry is to remove the config dir on rpm
> remove,
> and it is not intended to protect any files inside the dir.

Whatever.

> 
> > 
> > > 
> > > /etc/ovirt-provider-ovn/conf.d/*  -- contains all the custom config which
> > > overrides the values defined in the default config. We only look at *.conf
> > > files, and I don't really expect anything else to be present there.
> > 
> > Most likely, (some) people that update these files also keep there backups,
> > and imo we should backup/restore these backups as well.
> > 
> > > We could backup /etc/ovirt-provider-ovn/conf.d/*.conf only.
> > 
> > See above. What do you think
> 
> Any extra files in the directory do not cause any harm (the provider
> will just ignore additional files).
> If there is any value in backing them up, let's have them.
> 
> 
> > 
> > IMO:
> > 
> > 1. Currently, backup/restore all of /etc/ovirt-provider-ovn
> 
> Maybe /etc/ovirt-provider-ovn/conf.d?
> Do we want to support edits in
> /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf?

Official support is one thing, actual code is anther.

Your code already supports editing this file by the user.

Now we have to decide:

1. What do we want in the future. Either document that this is not supported, and perhaps change the code to not support that as well, or to not update this file anymore and find some other solution (e.g. add new stuff inside conf.d, or whatever).

2. What do to for current bug :-), backup/restore.

> This could lead to situations where a backup is applied on a newer version,
> but
> we restore a file from a previous version. The provider will use default
> values
> for missing config, but still this would be as if we sanctioned changing
> this file.

Exactly. But as I wrote above, you have the exact same problem also with normal update, without backup/restore.

So I still vote, for current bug, to backup/restore all of /etc/ovirt-provider-ovn , and later on decide about (1.) above.

> 
> > 2. If needed, consider saving in the backup file the exact version of
> > ovs/ovn used during backup, and refuse, or at least warn/prompt, if trying
> > to restore with a different version. If you you/we want that, please open
> > another bug.
> 
> Do we need this of ovs/ovn? Let me check with the ovn team again if replacing
> the config with one of a different version could harm us.

Thanks. But note that it will complicate things for users, e.g. if they had
non-latest version of package during backup, but when restoring had to use
latest (or spend much more time finding the exact needed version they had).
So it's best if we strictly keep compatible versions inside z-stream, so that
users can safely backup at X.Y.Z and restore at X.Y.ZZ.

> 
> > 3. Open another bug (or a few) about '%config(noreplace)
> 
> As stated above we only need this for cleanup during "rpm remove".
> 
> 
> > %{_sysconfdir}/ovirt-provider-ovn' - check what it does, decide if that's ok
> > for us (and then officially document that), and if not, do something (and
> > then document the change).
> > 4. If needed, open another bug to refine the list of files to backup/restore.
> > 
> > > 
> > > 
> > > 
> > > > 
> > > > > 
> > > > > ovirt-provider-ovn is statless, so there is no data to backup. The state is
> > > > > in ovs, so we should backup the ovs databases:
> > > > > /var/lib/openvswitch/ovnnb_db.db
> > > > > /var/lib/openvswitch/ovnsb_db.db
> > > > > or maybe the whole dir: /var/lib/openvswitch/*?
> > > > 
> > > > Yes, is that ok?
> > > 
> > > I'll check with the ovs team if it's safe to overwrite the whole dir.
> > 
> > Thanks.
> 
> Overwriting the whole dir is ok (confirmed by nsiddique)
> 
> > 
> > > 
> > > 
> > > > 
> > > > > 
> > > > > service firewall (as above)
> > > > 
> > > > ok
> > > 
> > > We would only need this if the user does any customizations to it.
> > > Otherwise it should be recreated on reinstalling.
> > 
> > Thinking about this, we indeed probably backup/restore the existing files
> > mainly because we allow overriding some ports (although IIRC that's not
> > documented):
> > 
> > packaging/firewalld 2430 $ grep -r '@' .
> > ./vmconsole-proxy/ovirt-vmconsole-proxy.xml.in:  <port protocol="tcp"
> > port="@VMCONSOLE_PROXY_PORT@"/>
> > ./ovirt-common/ovirt-https.xml.in:  <port protocol="tcp"
> > port="@HTTPS_PORT@"/>
> > ./ovirt-common/ovirt-http.xml.in:  <port protocol="tcp" port="@HTTP_PORT@"/>
> > ./websocket-proxy/ovirt-websocket-proxy.xml.in:  <port protocol="tcp"
> > port="@WEBSOCKET_PROXY_PORT@"/>
> > ./ovirt-engine/ovirt-jboss-https.xml.in:  <port protocol="tcp"
> > port="@JBOSS_HTTPS_PORT@"/>
> > ./ovirt-engine/ovirt-jboss-http.xml.in:  <port protocol="tcp"
> > port="@JBOSS_HTTP_PORT@"/>
> > 
> > I think it would be best to backup/restore, even though engine-setup does
> > not customize ovn firewalld service files. When user then runs engine-setup,
> > we always ask again "Configure firewall?", and if user says "no", we should
> > not touch these files (but I didn't verify that for ovn, although the code
> > looks ok to me).
> 
> Makes sense. Let's include it.

Comment 14 Marcin Mirecki 2019-04-11 14:08:51 UTC
(In reply to Yedidyah Bar David from comment #13)
> (In reply to Marcin Mirecki from comment #10)
> > (In reply to Yedidyah Bar David from comment #9)
> > > (In reply to Marcin Mirecki from comment #8)
> > > > (In reply to Yedidyah Bar David from comment #7)
> > > > > (In reply to Marcin Mirecki from comment #6)
> > > > > > ovirt-provider-ovn config:
> > > > > > /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf
> > > > > > /etc/ovirt-provider-ovn/conf.d/*
> > > > > 
> > > > > Any problem handling the entire directory /etc/ovirt-provider-ovn ?
> > > > 
> > > > Thinking about this a little more.
> > > > 
> > > > /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf  -- this should be the
> > > > default config, one not modified by the user. Any changes should be made in
> > > > the conf.d directory. Hence I'm not quite sure we should backup it (it could
> > > > break things if newer versions introduce new config values for example)
> > > 
> > > In the spec file, you have:
> > > 
> > > %config(noreplace) %{_sysconfdir}/ovirt-provider-ovn
> > > 
> > > I admit it's the first time I see such a thing - a directory (not a file)
> > > set as %config(noreplace). Searched a bit and couldn't find how this should
> > > behave. You might want to file a bug about this. If this behaves as
> > > "recursively handle everything underneath", then your assumption above
> > > (which might also be official documentation, didn't check) might break. In
> > > any case, if you indeed open a bug (and even if not), you should probably at
> > > least add a comment inside it saying that it should not be edited.
> > > 
> > > Considering the above, not sure what to do :-(. I think it's best to backup
> > > and restore everything. If we want to do that, we should commit to not
> > > change the file inside z-stream (as restore refuses to restore a backup
> > > taken with a different x.y version).
> > 
> > I don't think this was meant to work recursively.
> 
> Perhaps it was not "meant", but I now verified and it does:
> # rpm -qc ovirt-provider-ovn
> /etc/logrotate.d/ovirt-provider-ovn
> /etc/ovirt-provider-ovn/conf.d/README
> /etc/ovirt-provider-ovn/logger.conf
> /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf
> 
> I also edited ovirt-provider-ovn.conf and updated the package, and the file
> was not changed back.
> 
> I also pushed https://gerrit.ovirt.org/99001 to see what happens when you
> update the file in the new version, and updated from the jenkins result, and
> got:
> 
>   Updating   :
> ovirt-provider-ovn-1.2.21-0.20190328105938.git34d44b5.el7.noarch            
> 1/2 
> warning: /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf created as
> /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf.rpmnew
> 

I thought this would just overwrite it.
I assume the update proceeds fine when the file is not touched by the user?
Could we solve this by saying in the docs that changing this file is not supported,
and that changing this will make the update and backup of this file not work?


> > {_sysconfdir}/ovirt-provider-ovn/ovirt-provider-ovn.conf is meant to be
> > updated.
> 
> I guess you meant "meant to be updated by the rpm package, not by users".
> But nothing prevent users from updating it, and if they do, if you have
> 'config(noreplace)', rpm will not overwrite their changed file.

Yes, updated by rpm updates, NOT by users (sorry for being ambiguous)

> 
> > We do not expect to ever provide any files in the conf.d directory during
> > updates,
> > so they do not need to be protected. 
> > I think the only purpose of this entry is to remove the config dir on rpm
> > remove,
> > and it is not intended to protect any files inside the dir.
> 
> Whatever.
> 
> > 
> > > 
> > > > 
> > > > /etc/ovirt-provider-ovn/conf.d/*  -- contains all the custom config which
> > > > overrides the values defined in the default config. We only look at *.conf
> > > > files, and I don't really expect anything else to be present there.
> > > 
> > > Most likely, (some) people that update these files also keep there backups,
> > > and imo we should backup/restore these backups as well.
> > > 
> > > > We could backup /etc/ovirt-provider-ovn/conf.d/*.conf only.
> > > 
> > > See above. What do you think
> > 
> > Any extra files in the directory do not cause any harm (the provider
> > will just ignore additional files).
> > If there is any value in backing them up, let's have them.
> > 
> > 
> > > 
> > > IMO:
> > > 
> > > 1. Currently, backup/restore all of /etc/ovirt-provider-ovn
> > 
> > Maybe /etc/ovirt-provider-ovn/conf.d?
> > Do we want to support edits in
> > /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf?
> 
> Official support is one thing, actual code is anther.
> 
> Your code already supports editing this file by the user.

I'm not sure I get this. The user can just as well modify any source file,
but that does not mean it's supported.

> 
> Now we have to decide:
> 
> 1. What do we want in the future. Either document that this is not
> supported, and perhaps change the code to not support that as well, or to
> not update this file anymore and find some other solution (e.g. add new
> stuff inside conf.d, or whatever).

I would go with documenting that changing this is not supported.

> 
> 2. What do to for current bug :-), backup/restore.

I would use a fresh file from rpm, only restore the conf.d files.

> 
> > This could lead to situations where a backup is applied on a newer version,
> > but
> > we restore a file from a previous version. The provider will use default
> > values
> > for missing config, but still this would be as if we sanctioned changing
> > this file.
> 
> Exactly. But as I wrote above, you have the exact same problem also with
> normal update, without backup/restore.
> 
> So I still vote, for current bug, to backup/restore all of
> /etc/ovirt-provider-ovn , and later on decide about (1.) above.
> 
> > 
> > > 2. If needed, consider saving in the backup file the exact version of
> > > ovs/ovn used during backup, and refuse, or at least warn/prompt, if trying
> > > to restore with a different version. If you you/we want that, please open
> > > another bug.
> > 
> > Do we need this of ovs/ovn? Let me check with the ovn team again if replacing
> > the config with one of a different version could harm us.
> 
> Thanks. But note that it will complicate things for users, e.g. if they had
> non-latest version of package during backup, but when restoring had to use
> latest (or spend much more time finding the exact needed version they had).
> So it's best if we strictly keep compatible versions inside z-stream, so that
> users can safely backup at X.Y.Z and restore at X.Y.ZZ.
> 
> > 
> > > 3. Open another bug (or a few) about '%config(noreplace)
> > 
> > As stated above we only need this for cleanup during "rpm remove".
> > 
> > 
> > > %{_sysconfdir}/ovirt-provider-ovn' - check what it does, decide if that's ok
> > > for us (and then officially document that), and if not, do something (and
> > > then document the change).
> > > 4. If needed, open another bug to refine the list of files to backup/restore.
> > > 
> > > > 
> > > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > ovirt-provider-ovn is statless, so there is no data to backup. The state is
> > > > > > in ovs, so we should backup the ovs databases:
> > > > > > /var/lib/openvswitch/ovnnb_db.db
> > > > > > /var/lib/openvswitch/ovnsb_db.db
> > > > > > or maybe the whole dir: /var/lib/openvswitch/*?
> > > > > 
> > > > > Yes, is that ok?
> > > > 
> > > > I'll check with the ovs team if it's safe to overwrite the whole dir.
> > > 
> > > Thanks.
> > 
> > Overwriting the whole dir is ok (confirmed by nsiddique)
> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > service firewall (as above)
> > > > > 
> > > > > ok
> > > > 
> > > > We would only need this if the user does any customizations to it.
> > > > Otherwise it should be recreated on reinstalling.
> > > 
> > > Thinking about this, we indeed probably backup/restore the existing files
> > > mainly because we allow overriding some ports (although IIRC that's not
> > > documented):
> > > 
> > > packaging/firewalld 2430 $ grep -r '@' .
> > > ./vmconsole-proxy/ovirt-vmconsole-proxy.xml.in:  <port protocol="tcp"
> > > port="@VMCONSOLE_PROXY_PORT@"/>
> > > ./ovirt-common/ovirt-https.xml.in:  <port protocol="tcp"
> > > port="@HTTPS_PORT@"/>
> > > ./ovirt-common/ovirt-http.xml.in:  <port protocol="tcp" port="@HTTP_PORT@"/>
> > > ./websocket-proxy/ovirt-websocket-proxy.xml.in:  <port protocol="tcp"
> > > port="@WEBSOCKET_PROXY_PORT@"/>
> > > ./ovirt-engine/ovirt-jboss-https.xml.in:  <port protocol="tcp"
> > > port="@JBOSS_HTTPS_PORT@"/>
> > > ./ovirt-engine/ovirt-jboss-http.xml.in:  <port protocol="tcp"
> > > port="@JBOSS_HTTP_PORT@"/>
> > > 
> > > I think it would be best to backup/restore, even though engine-setup does
> > > not customize ovn firewalld service files. When user then runs engine-setup,
> > > we always ask again "Configure firewall?", and if user says "no", we should
> > > not touch these files (but I didn't verify that for ovn, although the code
> > > looks ok to me).
> > 
> > Makes sense. Let's include it.

Comment 15 Yedidyah Bar David 2019-04-18 07:03:22 UTC
(In reply to Marcin Mirecki from comment #14)
> (In reply to Yedidyah Bar David from comment #13)
> > (In reply to Marcin Mirecki from comment #10)
> > > (In reply to Yedidyah Bar David from comment #9)
> > > > (In reply to Marcin Mirecki from comment #8)
> > > > > (In reply to Yedidyah Bar David from comment #7)
> > > > > > (In reply to Marcin Mirecki from comment #6)
> > > > > > > ovirt-provider-ovn config:
> > > > > > > /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf
> > > > > > > /etc/ovirt-provider-ovn/conf.d/*
> > > > > > 
> > > > > > Any problem handling the entire directory /etc/ovirt-provider-ovn ?
> > > > > 
> > > > > Thinking about this a little more.
> > > > > 
> > > > > /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf  -- this should be the
> > > > > default config, one not modified by the user. Any changes should be made in
> > > > > the conf.d directory. Hence I'm not quite sure we should backup it (it could
> > > > > break things if newer versions introduce new config values for example)
> > > > 
> > > > In the spec file, you have:
> > > > 
> > > > %config(noreplace) %{_sysconfdir}/ovirt-provider-ovn
> > > > 
> > > > I admit it's the first time I see such a thing - a directory (not a file)
> > > > set as %config(noreplace). Searched a bit and couldn't find how this should
> > > > behave. You might want to file a bug about this. If this behaves as
> > > > "recursively handle everything underneath", then your assumption above
> > > > (which might also be official documentation, didn't check) might break. In
> > > > any case, if you indeed open a bug (and even if not), you should probably at
> > > > least add a comment inside it saying that it should not be edited.
> > > > 
> > > > Considering the above, not sure what to do :-(. I think it's best to backup
> > > > and restore everything. If we want to do that, we should commit to not
> > > > change the file inside z-stream (as restore refuses to restore a backup
> > > > taken with a different x.y version).
> > > 
> > > I don't think this was meant to work recursively.
> > 
> > Perhaps it was not "meant", but I now verified and it does:
> > # rpm -qc ovirt-provider-ovn
> > /etc/logrotate.d/ovirt-provider-ovn
> > /etc/ovirt-provider-ovn/conf.d/README
> > /etc/ovirt-provider-ovn/logger.conf
> > /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf
> > 
> > I also edited ovirt-provider-ovn.conf and updated the package, and the file
> > was not changed back.
> > 
> > I also pushed https://gerrit.ovirt.org/99001 to see what happens when you
> > update the file in the new version, and updated from the jenkins result, and
> > got:
> > 
> >   Updating   :
> > ovirt-provider-ovn-1.2.21-0.20190328105938.git34d44b5.el7.noarch            
> > 1/2 
> > warning: /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf created as
> > /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf.rpmnew
> > 
> 
> I thought this would just overwrite it.
> I assume the update proceeds fine when the file is not touched by the user?

Yes

> Could we solve this by saying in the docs that changing this file is not
> supported,
> and that changing this will make the update and backup of this file not work?

Yes, but this won't help people that already read the docs, once (that's
everyone), and then changed the file.

> 
> 
> > > {_sysconfdir}/ovirt-provider-ovn/ovirt-provider-ovn.conf is meant to be
> > > updated.
> > 
> > I guess you meant "meant to be updated by the rpm package, not by users".
> > But nothing prevent users from updating it, and if they do, if you have
> > 'config(noreplace)', rpm will not overwrite their changed file.
> 
> Yes, updated by rpm updates, NOT by users (sorry for being ambiguous)
> 
> > 
> > > We do not expect to ever provide any files in the conf.d directory during
> > > updates,
> > > so they do not need to be protected. 
> > > I think the only purpose of this entry is to remove the config dir on rpm
> > > remove,
> > > and it is not intended to protect any files inside the dir.
> > 
> > Whatever.
> > 
> > > 
> > > > 
> > > > > 
> > > > > /etc/ovirt-provider-ovn/conf.d/*  -- contains all the custom config which
> > > > > overrides the values defined in the default config. We only look at *.conf
> > > > > files, and I don't really expect anything else to be present there.
> > > > 
> > > > Most likely, (some) people that update these files also keep there backups,
> > > > and imo we should backup/restore these backups as well.
> > > > 
> > > > > We could backup /etc/ovirt-provider-ovn/conf.d/*.conf only.
> > > > 
> > > > See above. What do you think
> > > 
> > > Any extra files in the directory do not cause any harm (the provider
> > > will just ignore additional files).
> > > If there is any value in backing them up, let's have them.
> > > 
> > > 
> > > > 
> > > > IMO:
> > > > 
> > > > 1. Currently, backup/restore all of /etc/ovirt-provider-ovn
> > > 
> > > Maybe /etc/ovirt-provider-ovn/conf.d?
> > > Do we want to support edits in
> > > /etc/ovirt-provider-ovn/ovirt-provider-ovn.conf?
> > 
> > Official support is one thing, actual code is anther.
> > 
> > Your code already supports editing this file by the user.
> 
> I'm not sure I get this. The user can just as well modify any source file,
> but that does not mean it's supported.

In "your code", I referred to "%config(noreplace)". In writing this, you
officially state that users are allowed/supposed to be able to edit this
file, and updates to it in newer versions will not overwrite the file if
users changed it.

If users update any other file, 'yum update' will overwrite their changes.

> 
> > 
> > Now we have to decide:
> > 
> > 1. What do we want in the future. Either document that this is not
> > supported, and perhaps change the code to not support that as well, or to
> > not update this file anymore and find some other solution (e.g. add new
> > stuff inside conf.d, or whatever).
> 
> I would go with documenting that changing this is not supported.

Fine, but IMO we can still not ignore people that already changed it, unless
we have good reasons that these do not exist (or are very very few).

> 
> > 
> > 2. What do to for current bug :-), backup/restore.
> 
> I would use a fresh file from rpm, only restore the conf.d files.

I am going to do this now, mainly to get this bug going, but can't say I feel that good about this...

Also filed now bug 1701121, so that we can ignore it for now.

> 
> > 
> > > This could lead to situations where a backup is applied on a newer version,
> > > but
> > > we restore a file from a previous version. The provider will use default
> > > values
> > > for missing config, but still this would be as if we sanctioned changing
> > > this file.
> > 
> > Exactly. But as I wrote above, you have the exact same problem also with
> > normal update, without backup/restore.
> > 
> > So I still vote, for current bug, to backup/restore all of
> > /etc/ovirt-provider-ovn , and later on decide about (1.) above.
> > 
> > > 
> > > > 2. If needed, consider saving in the backup file the exact version of
> > > > ovs/ovn used during backup, and refuse, or at least warn/prompt, if trying
> > > > to restore with a different version. If you you/we want that, please open
> > > > another bug.
> > > 
> > > Do we need this of ovs/ovn? Let me check with the ovn team again if replacing
> > > the config with one of a different version could harm us.
> > 
> > Thanks. But note that it will complicate things for users, e.g. if they had
> > non-latest version of package during backup, but when restoring had to use
> > latest (or spend much more time finding the exact needed version they had).
> > So it's best if we strictly keep compatible versions inside z-stream, so that
> > users can safely backup at X.Y.Z and restore at X.Y.ZZ.
> > 
> > > 
> > > > 3. Open another bug (or a few) about '%config(noreplace)
> > > 
> > > As stated above we only need this for cleanup during "rpm remove".
> > > 
> > > 
> > > > %{_sysconfdir}/ovirt-provider-ovn' - check what it does, decide if that's ok
> > > > for us (and then officially document that), and if not, do something (and
> > > > then document the change).
> > > > 4. If needed, open another bug to refine the list of files to backup/restore.
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > ovirt-provider-ovn is statless, so there is no data to backup. The state is
> > > > > > > in ovs, so we should backup the ovs databases:
> > > > > > > /var/lib/openvswitch/ovnnb_db.db
> > > > > > > /var/lib/openvswitch/ovnsb_db.db
> > > > > > > or maybe the whole dir: /var/lib/openvswitch/*?
> > > > > > 
> > > > > > Yes, is that ok?
> > > > > 
> > > > > I'll check with the ovs team if it's safe to overwrite the whole dir.
> > > > 
> > > > Thanks.
> > > 
> > > Overwriting the whole dir is ok (confirmed by nsiddique)
> > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > service firewall (as above)
> > > > > > 
> > > > > > ok
> > > > > 
> > > > > We would only need this if the user does any customizations to it.
> > > > > Otherwise it should be recreated on reinstalling.
> > > > 
> > > > Thinking about this, we indeed probably backup/restore the existing files
> > > > mainly because we allow overriding some ports (although IIRC that's not
> > > > documented):
> > > > 
> > > > packaging/firewalld 2430 $ grep -r '@' .
> > > > ./vmconsole-proxy/ovirt-vmconsole-proxy.xml.in:  <port protocol="tcp"
> > > > port="@VMCONSOLE_PROXY_PORT@"/>
> > > > ./ovirt-common/ovirt-https.xml.in:  <port protocol="tcp"
> > > > port="@HTTPS_PORT@"/>
> > > > ./ovirt-common/ovirt-http.xml.in:  <port protocol="tcp" port="@HTTP_PORT@"/>
> > > > ./websocket-proxy/ovirt-websocket-proxy.xml.in:  <port protocol="tcp"
> > > > port="@WEBSOCKET_PROXY_PORT@"/>
> > > > ./ovirt-engine/ovirt-jboss-https.xml.in:  <port protocol="tcp"
> > > > port="@JBOSS_HTTPS_PORT@"/>
> > > > ./ovirt-engine/ovirt-jboss-http.xml.in:  <port protocol="tcp"
> > > > port="@JBOSS_HTTP_PORT@"/>
> > > > 
> > > > I think it would be best to backup/restore, even though engine-setup does
> > > > not customize ovn firewalld service files. When user then runs engine-setup,
> > > > we always ask again "Configure firewall?", and if user says "no", we should
> > > > not touch these files (but I didn't verify that for ovn, although the code
> > > > looks ok to me).
> > > 
> > > Makes sense. Let's include it.

Comment 17 Petr Matyáš 2019-06-19 07:42:19 UTC
Verified on ovirt-engine-4.3.5-0.1.el7.noarch

Comment 21 errata-xmlrpc 2019-08-12 11:53:27 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2019:2431


Note You need to log in before you can comment on or make changes to this bug.