SDN-4829: Add machine config watchers to report IPsec status by pperiyasamy · Pull Request #2383 · openshift/cluster-network-operator

pperiyasamy · 2024-05-23T17:05:04Z

This PR enhances the status manager to react to the network operator's machine configs rendering, listens to machine config and machine config pool events during the lifetime of the machine configs rendered by the network operator and reflects machine config pool(s) progressing/degraded status while rolling out changes. It uses a new network.operator.openshift.io/rendered-machineconfig-state annotation as dynamic cache on the network cluster operator object to stash rendered machine config state so that network operator pod can rely on it to build its own internal state upon its pod restart caused by machine config triggered node reboot.

openshift-ci-robot · 2024-05-27T13:22:04Z

@pperiyasamy: This pull request references SDN-4829 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.17.0" version, but no target version was set.

Details

In response to this:

This enhances status manager to watch for machine config and machine config pool objects, retrieves machine config pool status when ipsec is enabled and when any one of the machine config pool goes into degraded state, then makes network operator also into degraded state.

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.

pperiyasamy · 2024-05-27T13:23:14Z

/assign @jcaamano @kyrtapz

pperiyasamy · 2024-05-29T11:49:29Z

/retest

pperiyasamy · 2024-08-06T08:48:07Z

/retest

pperiyasamy · 2024-08-06T08:48:13Z

/retest

jcaamano

Some generic comments first.

With this, I also think we should no longer return progressing from the render method. That was a trick we should not have used.

And would also like @kyrtapz to take a look eventually.

pperiyasamy · 2024-12-04T13:53:42Z

/retest

pperiyasamy · 2025-02-28T11:40:48Z

/retest-required

pperiyasamy · 2025-02-28T11:42:35Z

/test e2e-aws-ovn-ipsec-upgrade
/test e2e-aws-ovn-ipsec-serial

some ipsec jobs are failing due to olm deployment failures which happening very recently, not related to PR change. triggering it agin.

This enhances status manager to watch for machine config and machine config pool objects, retrieves machine config pool status when ipsec is enabled and when any one of the machine config pool goes into degraded state, then makes network operator also into degraded state. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>

This commit also provides progressing status about ipsec machine config roll out, it notifies user about ongoing progress, otherwise network co gets updated only at the end with degrade status if there is any failure with ipsec rollout. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>

The status manager should not be tied to only ipsec machine config, network operator may own other machine configs in future, so this commit processes every machine configs that contains network operator as owner reference. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>

This reverts commit 763c565.

This replaces informer with source.Kind for machine config watcher which helps to make use of same cache used for the manager. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>

This commit captures machine config delete event, watches machine config pool while removing machine config from the cluster and reflect machine config pool status conditions into network status. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>

When network operator owned machine config is deleted, though mco is still deleting the rendered machine config from the machine config pool, there is no way to lookup owner reference on the machine config object upon network operator pod reboot caused by node reboot. This commit writes machine config rendering status onto network.operator.openshift.io/last-seen-machineconfig-state network co annotation and this is being looked up to report status accordingly. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>

This commit removes unnecessary initialization of machine config cache while handling events, cleans up cache entry at the right time, better handling of variable names, errors and adds few doc comments. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>

pperiyasamy · 2025-02-28T14:03:58Z

/test e2e-aws-ovn-ipsec-upgrade
/test e2e-aws-ovn-ipsec-serial

pperiyasamy · 2025-02-28T16:52:33Z

The commit b99b606 is updated with following change to fix an issue (Network status is still reflecting machine config pool status even after IPsec is disabled) found from @huiran0826's testing.

$ git diff
diff --git a/pkg/controller/statusmanager/machineconfig_status.go b/pkg/controller/statusmanager/machineconfig_status.go
index fa61f2dff..858293c22 100644
--- a/pkg/controller/statusmanager/machineconfig_status.go
+++ b/pkg/controller/statusmanager/machineconfig_status.go
@@ -157,7 +157,16 @@ func (status *StatusManager) SetFromMachineConfigPool(mcPools []mcfgv1.MachineCo
                                        removed = mcutil.AreMachineConfigsRemovedFromPool(pool.Status, mcSet)
                                        if removed {
                                                status.machineConfigsBeingRemoved[role].Delete(machineConfig)
+                                               // Delete map entry from status cache if role doesn't have machine configs. By deleting the entry,
+                                               // there won't be any unnecessary processing of pools in the reconcile loop when it's not dealing
+                                               // with network operator machine configs anymore.
+                                               if status.machineConfigsBeingRemoved[role].Len() == 0 {
+                                                       delete(status.machineConfigsBeingRemoved, role)
+                                               }
                                                status.renderedMachineConfigs[role].Delete(machineConfig)
+                                               if status.renderedMachineConfigs[role].Len() == 0 {
+                                                       delete(status.renderedMachineConfigs, role)
+                                               }
                                                if err := status.setLastRenderedMachineConfigState(status.renderedMachineConfigs); err != nil {
                                                        return fmt.Errorf("failed to update rendered machine config state: %v", err)
                                                }
diff --git a/pkg/controller/statusmanager/status_manager_test.go b/pkg/controller/statusmanager/status_manager_test.go
index 8a1930dcf..60ce0fe84 100644
--- a/pkg/controller/statusmanager/status_manager_test.go
+++ b/pkg/controller/statusmanager/status_manager_test.go
@@ -737,6 +737,39 @@ func TestStatusManagerSetFromIPsecConfigs(t *testing.T) {
        if len(co.Status.Versions) > 0 {
                t.Fatalf("Status.Versions unexpectedly already set: %#v", co.Status.Versions)
        }
+
+       // Set master mcp into degraded state, check network operator status condition is
+       // doesn't go into degraded state.
+       masterIPsecmachineConfigPool.Status = mcfgv1.MachineConfigPoolStatus{Conditions: []mcfgv1.MachineConfigPoolCondition{{Type: mcfgv1.MachineConfigPoolDegraded,
+               Status: v1.ConditionTrue}}}
+       err = status.SetFromMachineConfigPool([]mcfgv1.MachineConfigPool{masterIPsecmachineConfigPool,
+               workerIPsecMachineConfigPool})
+       if err != nil {
+               t.Fatalf("error processing machine config pools: %v", err)
+       }
+       co, oc, err = getStatuses(client, "testing")
+       if err != nil {
+               t.Fatalf("error getting ClusterOperator: %v", err)
+       }
+       if !conditionsInclude(oc.Status.Conditions, []operv1.OperatorCondition{
+               {
+                       Type:   operv1.OperatorStatusTypeDegraded,
+                       Status: operv1.ConditionFalse,
+               },
+       }) {
+               t.Fatalf("unexpected network Status.Conditions: %#v", oc.Status.Conditions)
+       }
+       if !conditionsInclude(oc.Status.Conditions, []operv1.OperatorCondition{
+               {
+                       Type:   operv1.OperatorStatusTypeProgressing,
+                       Status: operv1.ConditionFalse,
+               },
+       }) {
+               t.Fatalf("unexpected Status.Conditions: %#v", oc.Status.Conditions)
+       }

Did few tests with changes, The issue is also solved with the fix.

pperiyasamy · 2025-02-28T16:53:24Z

/hold cancel

pperiyasamy · 2025-02-28T18:14:52Z

/retest-required

jcaamano · 2025-03-03T10:11:28Z

/lgtm

openshift-ci · 2025-03-03T10:12:24Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [jcaamano]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2025-03-03T13:06:51Z

/retest-required

Remaining retests: 0 against base HEAD c2322a9 and 2 for PR HEAD b99b606 in total

openshift-ci · 2025-03-03T15:18:00Z

@pperiyasamy: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/4.18-upgrade-from-stable-4.17-e2e-azure-ovn-upgrade	`bf88169`	link	false	`/test 4.18-upgrade-from-stable-4.17-e2e-azure-ovn-upgrade`
ci/prow/4.18-upgrade-from-stable-4.17-e2e-gcp-ovn-upgrade	`bf88169`	link	false	`/test 4.18-upgrade-from-stable-4.17-e2e-gcp-ovn-upgrade`
ci/prow/e2e-vsphere-ovn-dualstack-primaryv6	`b99b606`	link	false	`/test e2e-vsphere-ovn-dualstack-primaryv6`
ci/prow/e2e-aws-ovn-ipsec-upgrade	`b99b606`	link	false	`/test e2e-aws-ovn-ipsec-upgrade`
ci/prow/e2e-vsphere-ovn	`b99b606`	link	false	`/test e2e-vsphere-ovn`
ci/prow/security	`b99b606`	link	false	`/test security`
ci/prow/e2e-aws-hypershift-ovn-kubevirt	`b99b606`	link	false	`/test e2e-aws-hypershift-ovn-kubevirt`
ci/prow/4.19-upgrade-from-stable-4.18-e2e-gcp-ovn-upgrade	`b99b606`	link	false	`/test 4.19-upgrade-from-stable-4.18-e2e-gcp-ovn-upgrade`
ci/prow/4.19-upgrade-from-stable-4.18-e2e-azure-ovn-upgrade	`b99b606`	link	false	`/test 4.19-upgrade-from-stable-4.18-e2e-azure-ovn-upgrade`
ci/prow/e2e-aws-ovn-ipsec-serial	`b99b606`	link	false	`/test e2e-aws-ovn-ipsec-serial`

Full PR test history. Your PR dashboard.

Details

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-sigs/prow repository. I understand the commands that are listed here.

pperiyasamy · 2025-03-03T15:29:43Z

/retest-required

openshift-bot · 2025-03-03T22:57:38Z

[ART PR BUILD NOTIFIER]

Distgit: cluster-network-operator
This PR has been included in build cluster-network-operator-container-v4.19.0-202503032209.p0.g972dea1.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci Bot requested review from JacobTanenbaum and tssurya May 23, 2024 17:07

pperiyasamy marked this pull request as draft May 23, 2024 17:10

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 23, 2024

pperiyasamy force-pushed the mcp-watcher branch from b75108d to 04be42f Compare May 27, 2024 13:10

pperiyasamy marked this pull request as ready for review May 27, 2024 13:12

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2024

openshift-ci Bot requested a review from kyrtapz May 27, 2024 13:14

pperiyasamy changed the title ~~Add machine config pool watcher for status manager~~ SDN-4829: Add machine config watchers to report IPsec status May 27, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 27, 2024

openshift-ci Bot assigned jcaamano and kyrtapz May 27, 2024

pperiyasamy force-pushed the mcp-watcher branch from 04be42f to 3522040 Compare May 27, 2024 16:20

pperiyasamy force-pushed the mcp-watcher branch from 3522040 to 5f6efe5 Compare August 5, 2024 08:53

pperiyasamy force-pushed the mcp-watcher branch from 5f6efe5 to 0ad8ac9 Compare October 18, 2024 09:15

pperiyasamy mentioned this pull request Oct 18, 2024

OCPBUGS-52280: Move to use newer IPsec DaemonSets irrespective of MCP state #2454

Merged

jcaamano suggested changes Oct 21, 2024

View reviewed changes

Comment thread pkg/controller/statusmanager/machineconfig_watcher.go Outdated

Comment thread pkg/controller/statusmanager/status_manager.go Outdated

Comment thread pkg/controller/statusmanager/ipsec_status.go Outdated

jcaamano suggested changes Oct 22, 2024

View reviewed changes

Comment thread pkg/controller/statusmanager/machineconfig_watcher.go Outdated

pperiyasamy force-pushed the mcp-watcher branch from 66d2074 to 09e1092 Compare October 23, 2024 09:00

pperiyasamy force-pushed the mcp-watcher branch from 09e1092 to 6abf798 Compare November 11, 2024 12:59

jcaamano suggested changes Nov 29, 2024

View reviewed changes

pperiyasamy force-pushed the mcp-watcher branch 3 times, most recently from a66e86d to bca0e79 Compare December 3, 2024 14:37

openshift-ci Bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 28, 2025

pperiyasamy and others added 8 commits February 28, 2025 14:45

Revert "Update network operator status for IPsec"

cab04d3

This reverts commit 763c565.

Use source kind for machine config watcher

97309ac

This replaces informer with source.Kind for machine config watcher which helps to make use of same cache used for the manager. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>

pperiyasamy force-pushed the mcp-watcher branch from 82d19db to b99b606 Compare February 28, 2025 13:45

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2025

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2025

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2025

openshift-merge-bot Bot merged commit 972dea1 into openshift:master Mar 3, 2025

This was referenced Mar 6, 2025

[Release 4.15] OCPBUGS-51208: Use applyconfigurations for updating network.oprerator status #2653

Merged

[release-4.18] OCPBUGS-52949, OCPBUGS-50582: Unexpected Behavior During Cluster Upgrade for the ovn-ipsec-host pods #2644

Merged

Uh oh!

Conversation

pperiyasamy commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented May 27, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pperiyasamy commented May 27, 2024

Uh oh!

pperiyasamy commented May 29, 2024

Uh oh!

pperiyasamy commented Aug 6, 2024

Uh oh!

pperiyasamy commented Aug 6, 2024

Uh oh!

jcaamano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pperiyasamy commented Dec 4, 2024

Uh oh!

pperiyasamy commented Feb 28, 2025

Uh oh!

pperiyasamy commented Feb 28, 2025

Uh oh!

pperiyasamy commented Feb 28, 2025

Uh oh!

pperiyasamy commented Feb 28, 2025

Uh oh!

pperiyasamy commented Feb 28, 2025

Uh oh!

pperiyasamy commented Feb 28, 2025

Uh oh!

jcaamano commented Mar 3, 2025

Uh oh!

openshift-ci Bot commented Mar 3, 2025

Uh oh!

openshift-ci-robot commented Mar 3, 2025

Uh oh!

openshift-ci Bot commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pperiyasamy commented Mar 3, 2025

Uh oh!

openshift-bot commented Mar 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pperiyasamy commented May 23, 2024 •

edited

Loading

openshift-ci-robot commented May 27, 2024 •

edited by openshift-ci Bot

Loading

openshift-ci Bot commented Mar 3, 2025 •

edited

Loading