OCPBUGS-28920: Update ingressconfig_controller to use field Manager #2259

npinaeva · 2024-02-07T12:09:46Z

Update ingressconfig_controller to use subcontroller "ingress_controller"
for field management on Patch calls. It used to apply changes with
deprecated cluster-network-operator field manager, that was overridden
and removed by ApplyObject. Thus, labels applied to
openshift-host-network namespace were periodically removed.

On upgrade, ingress-owned labels will be merged with "cluster-network-operator/operconfig" Apply call here https://github.com/openshift/cluster-network-operator/blob/master/pkg/apply/apply.go#L139-L150 and removed as the new owner didn't specify them. On the next reconcile loop, ingress controller will re-apply the labels with the new fieldManager and everything should work fine ever after.

For some reason, ingress controller was last this that doesn't use apply logic, but Patches instead.

Should end up with something like this

  managedFields:
  - apiVersion: v1
    fieldsType: FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          f:openshift.io/description: {}
          f:workload.openshift.io/allowed: {}
        f:labels:
          f:policy-group.network.openshift.io/host-network: {}
        f:ownerReferences:
          k:{"uid":"eae5d3b2-cf31-4bfa-940a-fa631adce87d"}: {}
    manager: cluster-network-operator/operconfig
    operation: Apply
    time: "2024-02-07T11:38:45Z"
  - apiVersion: v1
    fieldsType: FieldsV1
    fieldsV1:
      f:metadata:
        f:labels:
          f:network.openshift.io/policy-group: {}
          f:policy-group.network.openshift.io/ingress: {}
      f:spec: {}
    manager: cluster-network-operator/ingress_controller
    operation: Apply
    time: "2024-02-07T11:41:11Z"
  - apiVersion: v1
    fieldsType: FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          f:openshift.io/sa.scc.mcs: {}
          f:openshift.io/sa.scc.supplemental-groups: {}
          f:openshift.io/sa.scc.uid-range: {}
    manager: cluster-policy-controller
    operation: Update
    time: "2024-02-07T08:49:53Z"

openshift-ci-robot · 2024-02-07T12:09:52Z

@npinaeva: This pull request references Jira Issue OCPBUGS-28920, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.16.0) matches configured target version for branch (4.16.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @anuragthehatter

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

with subcontroller "ingress_controller". It used to apply changes with deprecated cluster-network-operator field manager, that was overridden and removed by ApplyObject. Thus, labels applied to openshift-host-network namespace were periodically removed.

On upgrade, ingress-owned labels will be merged with "cluster-network-operator/operconfig" Apply call here https://github.com/openshift/cluster-network-operator/blob/master/pkg/apply/apply.go#L139-L150 and removed as the new owner didn't specify them. On the next reconcile loop, ingress controller will re-apply the labels with the new fieldManager and everything should work fine ever after.

Should end up with something like this
 managedFields:
 - apiVersion: v1
   fieldsType: FieldsV1
   fieldsV1:
     f:metadata:
       f:annotations:
         f:openshift.io/description: {}
         f:workload.openshift.io/allowed: {}
       f:labels:
         f:policy-group.network.openshift.io/host-network: {}
       f:ownerReferences:
         k:{"uid":"eae5d3b2-cf31-4bfa-940a-fa631adce87d"}: {}
   manager: cluster-network-operator/operconfig
   operation: Apply
   time: "2024-02-07T11:38:45Z"
 - apiVersion: v1
   fieldsType: FieldsV1
   fieldsV1:
     f:metadata:
       f:labels:
         f:network.openshift.io/policy-group: {}
         f:policy-group.network.openshift.io/ingress: {}
     f:spec: {}
   manager: cluster-network-operator/ingress_controller
   operation: Apply
   time: "2024-02-07T11:41:11Z"
 - apiVersion: v1
   fieldsType: FieldsV1
   fieldsV1:
     f:metadata:
       f:annotations:
         f:openshift.io/sa.scc.mcs: {}
         f:openshift.io/sa.scc.supplemental-groups: {}
         f:openshift.io/sa.scc.uid-range: {}
   manager: cluster-policy-controller
   operation: Update
   time: "2024-02-07T08:49:53Z"

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 · 2024-02-07T12:22:15Z

@npinaeva: This pull request references Jira Issue OCPBUGS-28920, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.16.0) matches configured target version for branch (4.16.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @anuragthehatter

In response to this:

with subcontroller "ingress_controller". It used to apply changes with deprecated cluster-network-operator field manager, that was overridden and removed by ApplyObject. Thus, labels applied to openshift-host-network namespace were periodically removed.

On upgrade, ingress-owned labels will be merged with "cluster-network-operator/operconfig" Apply call here https://github.com/openshift/cluster-network-operator/blob/master/pkg/apply/apply.go#L139-L150 and removed as the new owner didn't specify them. On the next reconcile loop, ingress controller will re-apply the labels with the new fieldManager and everything should work fine ever after.

For some reason, ingress controller was last this that doesn't use apply logic, but Patches instead.

Should end up with something like this
 managedFields:
 - apiVersion: v1
   fieldsType: FieldsV1
   fieldsV1:
     f:metadata:
       f:annotations:
         f:openshift.io/description: {}
         f:workload.openshift.io/allowed: {}
       f:labels:
         f:policy-group.network.openshift.io/host-network: {}
       f:ownerReferences:
         k:{"uid":"eae5d3b2-cf31-4bfa-940a-fa631adce87d"}: {}
   manager: cluster-network-operator/operconfig
   operation: Apply
   time: "2024-02-07T11:38:45Z"
 - apiVersion: v1
   fieldsType: FieldsV1
   fieldsV1:
     f:metadata:
       f:labels:
         f:network.openshift.io/policy-group: {}
         f:policy-group.network.openshift.io/ingress: {}
     f:spec: {}
   manager: cluster-network-operator/ingress_controller
   operation: Apply
   time: "2024-02-07T11:41:11Z"
 - apiVersion: v1
   fieldsType: FieldsV1
   fieldsV1:
     f:metadata:
       f:annotations:
         f:openshift.io/sa.scc.mcs: {}
         f:openshift.io/sa.scc.supplemental-groups: {}
         f:openshift.io/sa.scc.uid-range: {}
   manager: cluster-policy-controller
   operation: Update
   time: "2024-02-07T08:49:53Z"

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.

kyrtapz

Overall looks good.
We should probably also address the same issue here

cluster-network-operator/pkg/controller/statusmanager/kube.go

Line 61 in 422456d

    
           return status.client.ClientFor(apply.GetClusterName(obj)).CRClient().Patch(ctx, obj, crclient.RawPatch(types.MergePatchType, patchData))

kyrtapz · 2024-02-08T09:21:00Z

pkg/controller/ingressconfig/ingressconfig_controller.go

@@ -64,7 +63,8 @@ var _ reconcile.Reconciler = &ReconcileIngressConfigs{}
 // ReconcileIngressConfigs watches for updates to ingress controller configuration
 // and sets the network policy related labels on the openshift-host-network namespace
 type ReconcileIngressConfigs struct {
-	client crclient.Client
+	client cnoclient.Client
+	mgr    manager.Manager


Instead of adding the manager to the controller you can extract the client from cnoclient.Client with client.Default().CRCClient()

kyrtapz · 2024-02-08T09:25:03Z

pkg/controller/ingressconfig/ingressconfig_controller.go

-		// The right way to address this would be to not even spawn the ingress
-		// controller if we are running in the context of a third party plugin
-		if apierrors.IsNotFound(err) {
-			return nil


Removing this changes the behavior of the function, if the namespace doesn't exist it will create it with the apply.ApplyObject. Is that intentional?

openshift-ci-robot · 2024-02-09T10:27:49Z

@npinaeva: This pull request references Jira Issue OCPBUGS-28920, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.16.0) matches configured target version for branch (4.16.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @anuragthehatter

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Update ingressconfig_controller to use subcontroller "ingress_controller"
for field management on Patch calls. It used to apply changes with
deprecated cluster-network-operator field manager, that was overridden
and removed by ApplyObject. Thus, labels applied to
openshift-host-network namespace were periodically removed.

On upgrade, ingress-owned labels will be merged with "cluster-network-operator/operconfig" Apply call here https://github.com/openshift/cluster-network-operator/blob/master/pkg/apply/apply.go#L139-L150 and removed as the new owner didn't specify them. On the next reconcile loop, ingress controller will re-apply the labels with the new fieldManager and everything should work fine ever after.

For some reason, ingress controller was last this that doesn't use apply logic, but Patches instead.

Should end up with something like this
 managedFields:
 - apiVersion: v1
   fieldsType: FieldsV1
   fieldsV1:
     f:metadata:
       f:annotations:
         f:openshift.io/description: {}
         f:workload.openshift.io/allowed: {}
       f:labels:
         f:policy-group.network.openshift.io/host-network: {}
       f:ownerReferences:
         k:{"uid":"eae5d3b2-cf31-4bfa-940a-fa631adce87d"}: {}
   manager: cluster-network-operator/operconfig
   operation: Apply
   time: "2024-02-07T11:38:45Z"
 - apiVersion: v1
   fieldsType: FieldsV1
   fieldsV1:
     f:metadata:
       f:labels:
         f:network.openshift.io/policy-group: {}
         f:policy-group.network.openshift.io/ingress: {}
     f:spec: {}
   manager: cluster-network-operator/ingress_controller
   operation: Apply
   time: "2024-02-07T11:41:11Z"
 - apiVersion: v1
   fieldsType: FieldsV1
   fieldsV1:
     f:metadata:
       f:annotations:
         f:openshift.io/sa.scc.mcs: {}
         f:openshift.io/sa.scc.supplemental-groups: {}
         f:openshift.io/sa.scc.uid-range: {}
   manager: cluster-policy-controller
   operation: Update
   time: "2024-02-07T08:49:53Z"

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.

…ler" for field management on Patch calls. It used to apply changes with deprecated cluster-network-operator field manager, that was overridden and removed by ApplyObject. Thus, labels applied to openshift-host-network namespace were periodically removed. Signed-off-by: Nadia Pinaeva <npinaeva@redhat.com>

kyrtapz · 2024-02-09T11:44:57Z

/lgtm

openshift-ci · 2024-02-09T11:45:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kyrtapz, npinaeva

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [kyrtapz]

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

kyrtapz · 2024-02-09T12:21:05Z

Overall looks good. We should probably also address the same issue here

We've decided to do a follow up tracked here: https://issues.redhat.com/browse/OCPBUGS-29288

openshift-ci-robot · 2024-02-09T12:35:04Z

/retest-required

Remaining retests: 0 against base HEAD d415d80 and 2 for PR HEAD 5231b51 in total

kyrtapz · 2024-02-09T15:25:33Z

/retest-required

openshift-ci · 2024-02-09T18:20:35Z

@npinaeva: 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/e2e-aws-ovn-single-node	`5231b51`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/e2e-vsphere-ovn-dualstack-primaryv6	`5231b51`	link	false	`/test e2e-vsphere-ovn-dualstack-primaryv6`
ci/prow/e2e-vsphere-ovn-dualstack	`5231b51`	link	false	`/test e2e-vsphere-ovn-dualstack`
ci/prow/e2e-ovn-ipsec-step-registry	`5231b51`	link	false	`/test e2e-ovn-ipsec-step-registry`
ci/prow/security	`5231b51`	link	false	`/test security`

Full PR test history. Your PR dashboard.

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

openshift-ci-robot · 2024-02-09T18:24:23Z

@npinaeva: Jira Issue OCPBUGS-28920: All pull requests linked via external trackers have merged:

openshift/cluster-network-operator#2259

Jira Issue OCPBUGS-28920 has been moved to the MODIFIED state.

In response to this:

Update ingressconfig_controller to use subcontroller "ingress_controller"
for field management on Patch calls. It used to apply changes with
deprecated cluster-network-operator field manager, that was overridden
and removed by ApplyObject. Thus, labels applied to
openshift-host-network namespace were periodically removed.

On upgrade, ingress-owned labels will be merged with "cluster-network-operator/operconfig" Apply call here https://github.com/openshift/cluster-network-operator/blob/master/pkg/apply/apply.go#L139-L150 and removed as the new owner didn't specify them. On the next reconcile loop, ingress controller will re-apply the labels with the new fieldManager and everything should work fine ever after.

For some reason, ingress controller was last this that doesn't use apply logic, but Patches instead.

Should end up with something like this
 managedFields:
 - apiVersion: v1
   fieldsType: FieldsV1
   fieldsV1:
     f:metadata:
       f:annotations:
         f:openshift.io/description: {}
         f:workload.openshift.io/allowed: {}
       f:labels:
         f:policy-group.network.openshift.io/host-network: {}
       f:ownerReferences:
         k:{"uid":"eae5d3b2-cf31-4bfa-940a-fa631adce87d"}: {}
   manager: cluster-network-operator/operconfig
   operation: Apply
   time: "2024-02-07T11:38:45Z"
 - apiVersion: v1
   fieldsType: FieldsV1
   fieldsV1:
     f:metadata:
       f:labels:
         f:network.openshift.io/policy-group: {}
         f:policy-group.network.openshift.io/ingress: {}
     f:spec: {}
   manager: cluster-network-operator/ingress_controller
   operation: Apply
   time: "2024-02-07T11:41:11Z"
 - apiVersion: v1
   fieldsType: FieldsV1
   fieldsV1:
     f:metadata:
       f:annotations:
         f:openshift.io/sa.scc.mcs: {}
         f:openshift.io/sa.scc.supplemental-groups: {}
         f:openshift.io/sa.scc.uid-range: {}
   manager: cluster-policy-controller
   operation: Update
   time: "2024-02-07T08:49:53Z"

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-bot · 2024-02-09T23:01:20Z

[ART PR BUILD NOTIFIER]

This PR has been included in build cluster-network-operator-container-v4.16.0-202402092208.p0.gcaa06bb.assembly.stream.el9 for distgit cluster-network-operator.
All builds following this will include this PR.

pperiyasamy · 2024-02-15T11:24:54Z

pkg/controller/ingressconfig/ingressconfig_controller.go

@@ -142,5 +142,7 @@ func (r *ReconcileIngressConfigs) updatePolicyGroupLabelOnNamespace(ctx context.

 	newNamespace.SetLabels(existingLabels)

-	return r.client.Patch(context.TODO(), newNamespace, crclient.MergeFrom(namespace))
+	return r.client.Patch(context.TODO(), newNamespace, crclient.MergeFrom(namespace), &crclient.PatchOptions{


@npinaeva @kyrtapz This seems to be using MergePatchType, shouldn't this be changed to use ApplyPatchType for SSA to kick in ?

openshift-ci bot requested review from anuragthehatter, kyrtapz and tssurya February 7, 2024 12:10

kyrtapz reviewed Feb 8, 2024

View reviewed changes

npinaeva force-pushed the ingress-controller-apply branch 2 times, most recently from b87efdb to e6248e3 Compare February 9, 2024 10:26

npinaeva changed the title ~~OCPBUGS-28920: Update ingressconfig_controller to use CNO Apply logic~~ OCPBUGS-28920: Update ingressconfig_controller to use field Manager Feb 9, 2024

npinaeva force-pushed the ingress-controller-apply branch from e6248e3 to 5231b51 Compare February 9, 2024 10:30

openshift-ci bot assigned kyrtapz Feb 9, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2024

npinaeva mentioned this pull request Feb 9, 2024

[release-4.15] OCPBUGS-29299: Update ingressconfig_controller to use field Manager #2265

Merged

openshift-merge-bot bot merged commit caa06bb into openshift:master Feb 9, 2024
37 of 42 checks passed

npinaeva deleted the ingress-controller-apply branch February 12, 2024 11:01

pperiyasamy reviewed Feb 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-28920: Update ingressconfig_controller to use field Manager #2259

OCPBUGS-28920: Update ingressconfig_controller to use field Manager #2259

npinaeva commented Feb 7, 2024 •

edited

openshift-ci-robot commented Feb 7, 2024

openshift-ci-robot commented Feb 7, 2024

kyrtapz left a comment

kyrtapz Feb 8, 2024

kyrtapz Feb 8, 2024

openshift-ci-robot commented Feb 9, 2024

kyrtapz commented Feb 9, 2024

openshift-ci bot commented Feb 9, 2024

kyrtapz commented Feb 9, 2024

openshift-ci-robot commented Feb 9, 2024

kyrtapz commented Feb 9, 2024

openshift-ci bot commented Feb 9, 2024

openshift-ci-robot commented Feb 9, 2024

openshift-bot commented Feb 9, 2024

pperiyasamy Feb 15, 2024

OCPBUGS-28920: Update ingressconfig_controller to use field Manager #2259

OCPBUGS-28920: Update ingressconfig_controller to use field Manager #2259

Conversation

npinaeva commented Feb 7, 2024 • edited

openshift-ci-robot commented Feb 7, 2024

openshift-ci-robot commented Feb 7, 2024

kyrtapz left a comment

Choose a reason for hiding this comment

kyrtapz Feb 8, 2024

Choose a reason for hiding this comment

kyrtapz Feb 8, 2024

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 9, 2024

kyrtapz commented Feb 9, 2024

openshift-ci bot commented Feb 9, 2024

kyrtapz commented Feb 9, 2024

openshift-ci-robot commented Feb 9, 2024

kyrtapz commented Feb 9, 2024

openshift-ci bot commented Feb 9, 2024

openshift-ci-robot commented Feb 9, 2024

openshift-bot commented Feb 9, 2024

pperiyasamy Feb 15, 2024

Choose a reason for hiding this comment

npinaeva commented Feb 7, 2024 •

edited