Add support to manage ovn-ic services with systemd

[odivlad]

This PR adds support to manage ovn-ic daemons with systemd.
Two new .service units are added: ovn-ic.service and ovsdb-server-ic.service.

• ovn-ic.service systemd-unit manages ovn-ic daemon
• ovn-ic-db.service systemd-unit manages OVN_IC_Northbound and OVN_IC_Southbound OVSDB server processes.

[numansiddique]

Thanks for the PR. This is definitely acceptable.

I will take a look hopefully soon.

Thanks

[numansiddique]

I've submitted this PR as patch series in the mailing list for further review - https://patchwork.ozlabs.org/project/ovn/list/?series=220521

[numansiddique]

Since ovn-ic needs to connect to OVN NB and SB DBs, user needs to define the connection parms to these DBs using the system file - /etc/systemd/system/ovn-ic.d/local.conf (OVN_IC_OPTS) if ovn-northd service is not runnng in the same machine right ?

[odivlad]

Yes, that's correct. Additionally user may supply OVN_IC_OPTS via /etc/sysconfig/ovn or /etc/sysconfig/ovn-ic.

[numansiddique]

@odivlad I've been thinking about the systemd service file you added to ovn-ic.

Did you consider making ovn-ic a sub package ? I checked with some of the fedora maintainers.
Although its not a hard rule, but having a subpackage for a service is better.

Would you mind reworking a bit to have a separate ovn-ic sub package ?

Thanks
Numan

[odivlad]

@numansiddique, sure I can do this.
But one thing: if we move ovn-ic and related stuff to sub-package, existing deployments should install ovn-ic package explicitly on upgrade. Is it okay, or, maybe ovn-central package should require ovn-ic for backward compatibility?

[odivlad]

And a second question, is it required to add firewalld xml for ovn ic? I can write config, by I don't use firewalld in my setup, so I can't test it in my env.

[odivlad]

@numansiddique, I've updated PR with requested changes.

[odivlad]

@numansiddique, please note that

if we move ovn-ic and related stuff to sub-package, existing deployments should install ovn-ic package explicitly on upgrade. Otherwise ovn-ic will be removed from the host on upgrade. Is it okay, or, maybe ovn-central package should require ovn-ic for backward compatibility?

I see, that commit is already in tree, but just want to double-check before those changes are released.
In my understanding most deployments will install ovn-ic on the central nodes. And technically they can run ovsdb-server-ic on one-two central nodes of each AZ in multi-AZ deployments. Even if they want to deploy global ovsdb on a separate nodes, it's easy to install ovn-central.rpm.
Do we really need a new subpackage?

[numansiddique]

@numansiddique, please note that

if we move ovn-ic and related stuff to sub-package, existing deployments should install ovn-ic package explicitly on upgrade. Otherwise ovn-ic will be removed from the host on upgrade. Is it okay, or, maybe ovn-central package should require ovn-ic for backward compatibility?

I see, that commit is already in tree, but just want to double-check before those changes are released.
In my understanding most deployments will install ovn-ic on the central nodes. And technically they can run ovsdb-server-ic on one-two central nodes of each AZ in multi-AZ deployments. Even if they want to deploy global ovsdb on a separate nodes, it's easy to install ovn-central.rpm.
Do we really need a new subpackage?

I think we decided on the sub package based on the comment on 17th Dec.

[numansiddique]

@odivlad I've been thinking about the systemd service file you added to ovn-ic.

Did you consider making ovn-ic a sub package ? I checked with some of the fedora maintainers.
Although its not a hard rule, but having a subpackage for a service is better.

Would you mind reworking a bit to have a separate ovn-ic sub package ?

Thanks
Numan

This one.

[hzhou8]

@odivlad I've been thinking about the systemd service file you added to ovn-ic.
Did you consider making ovn-ic a sub package ? I checked with some of the fedora maintainers.
Although its not a hard rule, but having a subpackage for a service is better.
Would you mind reworking a bit to have a separate ovn-ic sub package ?
Thanks
Numan

This one.

@numansiddique So do you think the ovsdb-server-ic (suggested to rename as ovn-ic-db) should be a submodule as well? It requires same components as regular ovsdb but just with IC-DB schemas.

[numansiddique]

@odivlad I've been thinking about the systemd service file you added to ovn-ic.
Did you consider making ovn-ic a sub package ? I checked with some of the fedora maintainers.
Although its not a hard rule, but having a subpackage for a service is better.
Would you mind reworking a bit to have a separate ovn-ic sub package ?
Thanks
Numan

This one.

@numansiddique So do you think the ovsdb-server-ic (suggested to rename as ovn-ic-db) should be a submodule as well? It requires same components as regular ovsdb but just with IC-DB schemas.

Before the commit 8abe98f ovn spec file was generating the below ovn packages

• ovn
  • ovn-central
  • ovn-host
  • ovn-vtep

ovn is the main package and others are sub packages. And now with this commit, it generates
- ovn
- ovn-central
- ovn-host
- ovn-vtep
- ovn-ic

ovn-ic was split from ovn-central. I think this is fine.

[numansiddique]

@numansiddique, sure I can do this.
But one thing: if we move ovn-ic and related stuff to sub-package, existing deployments should install ovn-ic package explicitly on upgrade. Is it okay, or, maybe ovn-central package should require ovn-ic for backward compatibility?

I thought about it when I suggested the change and I also tried out your patch. I think it should be fine as I don't see many users.

Are you ok with this ?

[numansiddique]

I think having separate package for ic would be better.
User needs to use the systemd service provided by the ovn-ic package on start ovn-ic on central node.
And the user can use the service - ovn-ic-db (added in this PR) to start ovsdb-servers on another node.

[odivlad]

@numansiddique, @hzhou8, thanks for the review. If I correctly understood all comments, I'm doing next:

1. Rename ovsdb-server-ic.service to ovn-ic-db.service
2. Leave ovn-ic-db.service systemd-unit incide ovn-ic subpackage.
3. The Numan's suggestion was that we don't add any special requirements for ovn-ic in ovn-central subpackage. Users who upgrade from previous versions to new code and who run OVN Interconnection, must install manually ovn-ic subpackage on nodes where they run ovn-ic daemon and global ovsdb interconnection servers. Should this be mentioned in NEWS file?
   Honestly, my opinion is that such small change can bring an unneeded headache to users. Are there any cons in adding ovn-ic as a Requires to ovn-central package? This can give backward compatibility, and in the same time users can install ovn-ic on any other then central node. What do you think about it?

[odivlad]

I've added fixup commit (with added requires to ovn-central). Let me know if it looks good, I'll squash it and reword with correct commit message.

[numansiddique]

@numansiddique, @hzhou8, thanks for the review. If I correctly understood all comments, I'm doing next:

1. Rename ovsdb-server-ic.service to ovn-ic-db.service
2. Leave ovn-ic-db.service systemd-unit incide ovn-ic subpackage.
3. The Numan's suggestion was that we don't add any special requirements for ovn-ic in ovn-central subpackage. Users who upgrade from previous versions to new code and who run OVN Interconnection, must install manually ovn-ic subpackage on nodes where they run ovn-ic daemon and global ovsdb interconnection servers. Should this be mentioned in NEWS file?
   Honestly, my opinion is that such small change can bring an unneeded headache to users. Are there any cons in adding ovn-ic as a Requires to ovn-central package? This can give backward compatibility, and in the same time users can install ovn-ic on any other then central node. What do you think about it?

I agree with you that its not a small change. I was just wondering if there are any users who build their own packages.
Normally from what I know, people would consume from either ubuntu, centos, fedora repos.

I looked into the instructions here - https://fedoraproject.org/wiki/Upgrade_paths_%E2%80%94_renaming_or_splitting_packages

I think we need to create compact packages when splitting the packages. I totally missed it earlier. Sorry. I normally study about packaging and the guidelines as and when required.

I'd suggest

• Revert the patch 8abe98f by moving the ovn-ic related to ovn-central package.
• Add the systemd files related to ovn-ic and make it part of ovn-central package.

I think we can revisit it later if we really need to split ovn-ic into its own package. What do you think ?

Thanks

[odivlad]

Agree, I'll update PR.

[odivlad]

@numansiddique, PR is updated.

[numansiddique]

Thanks. I'll take a look.

Numan

[hzhou8]

LGTM (but I didn't test it)

@numansiddique what's our process for github PR review (v.s. mailing-list patches)

[numansiddique]

rocess for githu

I normally post the patches myself for review to the ML.

I'm doing it now.

[numansiddique]

@odivlad Can you please update the first commit with your signed-off-by tag ?

Meanwhile I will add your tag myself and post it to the ovs-dev ML. I hope its fine.

Thanks
Numan

[numansiddique]

Patches posted here - https://patchwork.ozlabs.org/project/ovn/list/?series=227739

[odivlad]

Meanwhile I will add your tag myself and post it to the ovs-dev ML. I hope its fine.

That's okay, thanks :)

[numansiddique]

Thanks for the PR. The patches are applied to the master branch.