SDN-5330: Keep IPsec in OVN enabled during the upgrade - #2621
Conversation
pperiyasamy
changed the title
|
@pperiyasamy: This pull request references SDN-5330 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
/test e2e-aws-ovn-ipsec-upgrade |
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
| // We render OVN IPsec as long as East-West IPsec is enabled. | ||
| renderIPsecOVN = mode == operv1.IPsecModeFull |
There was a problem hiding this comment.
Why not
renderIPsecOVN = (renderIPsecHostDaemonSet || renderIPsecContainerizedDaemonSet || isOVNIPsecActive) && mode == operv1.IPsecModeFull
Why don't we need to wait anymore for the NS IPSec to be deployed before activating OVN IPSec?
Also, it's best if the comment explains why we are doing what we are doing, not just what we are doing.
There was a problem hiding this comment.
Why don't we need to wait anymore for the NS IPSec to be deployed before activating OVN IPSec?
activating OVN IPSec at this case won't trigger anything because ovs-monitor-ipsec script is not started yet. but still your suggestion on checking isOVNIPsecActive makes more sense as it enables IPsec OVN only when needed. changed it. added some info about explaining 'why', hope that's ok now.
There was a problem hiding this comment.
Thanks @jcaamano for your time and patience, it was a great long discussion to update IPsec state machine with up to date information. Updated the PR as we discussed.
e1059c0 to
4d1f23a
Compare
|
@pperiyasamy: This pull request references SDN-5330 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
The following change on the machine pool `status.MachineCount == status.UpdatedMachineCount && hasSourceInMachineConfigStatus(status, machineConfigs)` is introduced with PR openshift#2349 which ensures IPsec machine config is always installed on all the nodes in the cluster, So this is deleting the IPsec daemonset as per the CNO state machine for IPsec when the condition is not met. But this is also accidentally disabling IPsec in OVN which is not an expected behavior. This causes ovs-monitor-ipsec to refresh existing ipsec connections unnecessarily when IPsec pod comes up as it is not able to find remote_name from the tunnel. This may also trigger deleting IPsec connection entries from openshift.conf file if ovs-monitor-ipsec is not killed timely when ipsec daemonset is removed. So this commit enables ovn ipsec option as long as the API is set with Full mode. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
4d1f23a to
4e57dcd
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcaamano, pperiyasamy The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
1 similar comment
|
@pperiyasamy: 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-sigs/prow repository. I understand the commands that are listed here. |
|
[ART PR BUILD NOTIFIER] Distgit: cluster-network-operator |
6 participants
The following change on the machine pool:
status.MachineCount == status.UpdatedMachineCount && hasSourceInMachineConfigStatus(status, machineConfigs)is introduced with PR #2349 which ensures IPsec machine config is always installed on all the nodes in the cluster,
So this is deleting the IPsec daemonset as per the CNO state machine for IPsec when the condition is not met. But this is also accidentally disabling IPsec in OVN which is not an expected behavior. This causes ovs-monitor-ipsec to refresh existing ipsec connections unnecessarily when IPsec pod comes up as it is not able to find remote_name from the tunnel. This may also trigger deleting IPsec connection entries from openshift.conf file if ovs-monitor-ipsec is not killed timely when ipsec daemonset is removed. So this PR enables ovn ipsec option as long as the API is set with Full mode.