Bug 1874696: Add ovs-notification.service #2102
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: juanluisvaladas The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
juanluisvaladas
changed the title
openshift-ci-robot
added
do-not-merge/work-in-progress
|
@juanluisvaladas: This pull request references Bugzilla bug 1874696, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
lgtm but we should get some more review @abhat @mccv1r0 @danwinship @trozet |
juanluisvaladas
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
juanluisvaladas
force-pushed
the
add-ovs-notification
branch
from
6d918fa to
8f78baa
Compare
|
Tested it manually everything looks fine in my manual testing. |
Was going to say the same thing as they have the expertise to review this PR as they've been working on this 👍 |
|
@juanluisvaladas when you confirm that a cluster-bot multi-PR run does the right thing, please note that in this PR. Thanks! |
| contents: | | ||
| [Unit] | ||
| Description=Creates a file to let the OVS pod know OVS is running on systemd. | ||
| # Don't write the notification before OVS starts |
There was a problem hiding this comment.
Why not? The goal of the service is to tell the sdn-ovs pod that system OVS is enabled. If this service is running at all, then system OVS is enabled, so it doesn't matter if this runs before or after OVS starts
There was a problem hiding this comment.
Is the goal to have a hostPath type File constraint in ovn-kubernetes/006-ovs-node.yaml to keep ovs-node from starting until it sees this file?
There was a problem hiding this comment.
@danwinship yes I guess that's reasonable.
@mccv1r0 yes
| Requires=openvswitch.service | ||
| Wants=NetworkManager-wait-online.service | ||
| After=NetworkManager-wait-online.service openvswitch.service | ||
| Before=network-online.target kubelet.service crio.service node-valid-hostname.service |
There was a problem hiding this comment.
Why should we care about most of those services? If we run before kubelet then we run before the sdn-ovs pod and we're good, right?
There was a problem hiding this comment.
Did that to be consistent with the ovs-configuration.service.yaml, but you're right.
I'm pretty sure we don't need the node-valid-hostname.service and crio. Regarding the network-online.target, I think it's necessary so that the network-online.target isn't complete until this is finished.
| StandardError=journal+console | ||
|
|
||
| [Install] | ||
| WantedBy=network-online.target |
There was a problem hiding this comment.
This is part of the network so I'd say we can't consider the network completely setup until this unit is complete. I would also make sense in the network.target but I think network-online.target is the most logical place.
Anyway I don't feel strongly enough about this to block the PR while we discuss where it should go, so if you want it somewhere else I'll move it.
There was a problem hiding this comment.
This isn't right to say wanted by network online target, but then in runtime this is running before network online target
Yes, I confirm that |
In ovnkubernetes and openshift-sdn we're unable to reliably identify whether the openvswitch.service is enabled or not though systemctl or the systemd's DBus API. To avoid issues we have decided to create a unit that launches before the SDN and creates an empty file that we can check easily and reliably.
juanluisvaladas
force-pushed
the
add-ovs-notification
branch
from
8f78baa to
c2abaae
Compare
|
@danwinship resolved some of your concerns, but not entirely. Please let me know if you want it outside the network-online.target where should it go. network.target and I'll change that? |
|
@juanluisvaladas: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
trozet
left a comment
trozet
left a comment
There was a problem hiding this comment.
I dont really see why this PR is necessary if we already have ovs-confguration service. You can just mount that path and check that the file exists. As long as the service is part of a target, its fine to check the target file path. I think we already do this in ovn: https://github.com/openshift/cluster-network-operator/blob/master/bindata/network/ovn-kubernetes/ovnkube-node.yaml#L229
| StandardError=journal+console | ||
|
|
||
| [Install] | ||
| WantedBy=network-online.target |
There was a problem hiding this comment.
This isn't right to say wanted by network online target, but then in runtime this is running before network online target
Yeah... we don't need to answer the question "Is OVS actually running as a system service on this node?", we just need to answer the question "Is this node running a version of OCP which would configure OVS to run as a system service if OpenShiftSDN was the configured network plugin?" which is a fact that is either true or false at install/upgrade time, not something that needs to be computed at any particular time during boot.
We could check for one of the If we can't check for those, then we can add a new file to |
|
@danwinship I think ovs-configuration exists even in openshift-sdn deployments; it just is not enabled as a service. Peng was changing this though in another PR to also enable it for openshift-sdn I think: #2066 |
|
Ah... ok. And actually "it's possible that nmstate or something would result in that all being done differently in the future" doesn't matter because this is only needed for the 4.6 cycle anyway right? (The 4.6 sdn-ovs pod needs to be able to DTRT when started on a 4.5 node, but the 4.7 sdn-ovs pod will only ever be started on 4.6 or 4.7 nodes and so can assume system OVS is always enabled.) |
@pliurh Can this be merged before code freeze? If it will be merged the best thing we can do is closing this PR and add a line with |
I think we have to get that merge before code freeze. The keepalived issue has been fixed. I'm testing #2066 with the latest build. Could you add a comment in #2066 where you want to add this line? |
No, you don't need to test if the service has run, you only need to test if it's there. |
hmm, good point; if we do this, we're not yet sure if OVS is actually running, so we'll need to remove all the log tailing stuff from the OVS container or maybe have it use |
|
/close |
|
@juanluisvaladas: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@juanluisvaladas: This pull request references Bugzilla bug 1874696. The bug has been updated to no longer refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
9 participants
In ovnkubernetes and openshift-sdn we're unable to reliably identify
whether the openvswitch.service is enabled or not though systemctl or
the systemd's DBus API.
To avoid issues we have decided to create a unit that launches before
the SDN and creates an empty file that we can check easily and reliably.
This is part of a fix for BZ#1874696