OCPBUGS-1318 [Downstream Merge 23-09-2022]

[andreaskaris]

- What this PR does and why is it needed

- Special notes for reviewers
Some conflicts due to hybrid overlay downstream stuff. I hope I got this right.

[akaris@linux go-controller (merge_9_23_22_b)]$ git show f4ddad73ac9d971e0181c26003e0ad10622fa72f | cat
commit f4ddad73ac9d971e0181c26003e0ad10622fa72f
Merge: e1306251b 1c4421162
Author: Andreas Karis <ak.karis@gmail.com>
Date:   Mon Sep 26 15:55:12 2022 +0200

    Merge remote-tracking branch 'upstream/master' into merge_9_23_22_b
    
    Conflicts due to hybrid overlay OCP hacks:
    Kept some hack sections and added markers "OCP_HACK" for anyone hitting
    this in the future.
    Moved hybrid overlay add/update hacks from WatchNamespaces into obj_retry.go
    updateResource and addResource factories.
    
    Conflicts:
            go-controller/pkg/node/gateway_localnet.go
            go-controller/pkg/ovn/namespace.go
            go-controller/pkg/ovn/ovn.go
            go-controller/pkg/ovn/obj_retry.go
    Signed-off-by: Andreas Karis <ak.karis@gmail.com>

diff --cc go-controller/pkg/node/gateway_localnet.go
index ad7867a65,28ac41293..8b8615784
--- a/go-controller/pkg/node/gateway_localnet.go
+++ b/go-controller/pkg/node/gateway_localnet.go
@@@ -48,27 -47,6 +48,19 @@@ func newLocalGateway(nodeName string, h
  		return nil, err
  	}
  
- 	if config.OvnKubeNode.Mode == types.NodeModeFull {
- 		// add masquerade subnet route to avoid zeroconf routes
- 		err = addMasqueradeRoute(gwBridge.bridgeName, gwNextHops)
- 		if err != nil {
- 			return nil, err
- 		}
- 	}
- 
 +	// OCP HACK -- block MCS ports https://github.com/openshift/ovn-kubernetes/pull/170
 +	rules := []iptRule{}
 +	if config.IPv4Mode {
 +		generateBlockMCSRules(&rules, iptables.ProtocolIPv4)
 +	}
 +	if config.IPv6Mode {
 +		generateBlockMCSRules(&rules, iptables.ProtocolIPv6)
 +	}
 +	if err := addIptRules(rules); err != nil {
 +		return nil, fmt.Errorf("failed to setup MCS-blocking rules: %w", err)
 +	}
 +	// END OCP HACK
 +
  	if exGwBridge != nil {
  		gw.readyFunc = func() (bool, error) {
  			ready, err := gatewayReady(gwBridge.patchPort)
diff --cc go-controller/pkg/ovn/namespace.go
index c0ff8eae1,4b23cf4c9..0ac1603af
--- a/go-controller/pkg/ovn/namespace.go
+++ b/go-controller/pkg/ovn/namespace.go
@@@ -186,32 -187,12 +188,33 @@@ func (oc *Controller) AddNamespace(ns *
  		klog.Infof("[%s] adding namespace took %v", ns.Name, time.Since(start))
  	}()
  
 -	_, nsUnlock, err := oc.ensureNamespaceLocked(ns.Name, false, ns)
 +	nsInfo, nsUnlock, err := oc.ensureNamespaceLocked(ns.Name, false, ns)
  	if err != nil {
- 		klog.Errorf("Failed to ensure namespace locked: %v", err)
- 		return
+ 		return fmt.Errorf("failed to ensure namespace locked: %v", err)
  	}
- 
  	defer nsUnlock()
 +
++	// OCP HACK -- hybrid overlay
 +	annotation := ns.Annotations[hotypes.HybridOverlayExternalGw]
 +	if annotation != "" {
 +		parsedAnnotation := net.ParseIP(annotation)
 +		if parsedAnnotation == nil {
 +			klog.Errorf("Could not parse hybrid overlay external gw annotation")
 +		} else {
 +			nsInfo.hybridOverlayExternalGW = parsedAnnotation
 +		}
 +	}
 +	annotation = ns.Annotations[hotypes.HybridOverlayVTEP]
 +	if annotation != "" {
 +		parsedAnnotation := net.ParseIP(annotation)
 +		if parsedAnnotation == nil {
 +			klog.Errorf("Could not parse hybrid overlay VTEP annotation")
 +		} else {
 +			nsInfo.hybridOverlayVTEP = parsedAnnotation
 +		}
 +	}
++	// END OCP HACK
+ 	return nil
  }
  
  // configureNamespace ensures internal structures are updated based on namespace
@@@ -364,32 -350,13 +372,37 @@@ func (oc *Controller) updateNamespace(o
  		}
  	}
  
++	// OCP HACK -- hybrid overlay
 +	annotation := newer.Annotations[hotypes.HybridOverlayExternalGw]
 +	if annotation != "" {
 +		parsedAnnotation := net.ParseIP(annotation)
 +		if parsedAnnotation == nil {
 +			klog.Errorf("Could not parse hybrid overlay external gw annotation")
 +		} else {
 +			nsInfo.hybridOverlayExternalGW = parsedAnnotation
 +		}
 +	} else {
 +		nsInfo.hybridOverlayExternalGW = nil
 +	}
 +	annotation = newer.Annotations[hotypes.HybridOverlayVTEP]
 +	if annotation != "" {
 +		parsedAnnotation := net.ParseIP(annotation)
 +		if parsedAnnotation == nil {
 +			klog.Errorf("Could not parse hybrid overlay VTEP annotation")
 +		} else {
 +			nsInfo.hybridOverlayVTEP = parsedAnnotation
 +		}
 +	} else {
 +		nsInfo.hybridOverlayVTEP = nil
 +	}
- 	oc.multicastUpdateNamespace(newer, nsInfo)
++	// END OCP HACK
+ 	if err := oc.multicastUpdateNamespace(newer, nsInfo); err != nil {
+ 		errors = append(errors, err)
+ 	}
+ 	return kerrors.NewAggregate(errors)
  }
  
- func (oc *Controller) deleteNamespace(ns *kapi.Namespace) {
+ func (oc *Controller) deleteNamespace(ns *kapi.Namespace) error {
  	klog.Infof("[%s] deleting namespace", ns.Name)
  
  	nsInfo := oc.deleteNamespaceLocked(ns.Name)
diff --cc go-controller/pkg/ovn/obj_retry.go
index 16f275ec0,0c2cb0590..af6f52e59
--- a/go-controller/pkg/ovn/obj_retry.go
+++ b/go-controller/pkg/ovn/obj_retry.go
@@@ -10,6 -9,6 +9,8 @@@ import 
  	"sync"
  	"time"
  
++	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
++
  	ocpcloudnetworkapi "github.com/openshift/api/cloudnetwork/v1"
  
  	kapi "k8s.io/api/core/v1"
@@@ -746,6 -747,13 +754,21 @@@ func (oc *Controller) addResource(objec
  		cloudPrivateIPConfig := obj.(*ocpcloudnetworkapi.CloudPrivateIPConfig)
  		return oc.reconcileCloudPrivateIPConfig(nil, cloudPrivateIPConfig)
  
+ 	case factory.NamespaceType:
+ 		ns, ok := obj.(*kapi.Namespace)
+ 		if !ok {
+ 			return fmt.Errorf("could not cast %T object to *kapi.Namespace", obj)
+ 		}
++		// OCP HACK -- required for hybrid overlay
++		if config.HybridOverlay.Enabled && hasHybridAnnotation(ns.ObjectMeta) {
++			if err := oc.addNamespaceICNIv1(ns); err != nil {
++				klog.Errorf("Unable to handle legacy ICNIv1 check for namespace %q add, error: %v",
++					ns.Name, err)
++			}
++		}
++		// END OCP HACK
+ 		return oc.AddNamespace(ns)
+ 
  	default:
  		return fmt.Errorf("no add function for object type %s", objectsToRetry.oType)
  	}
@@@ -885,6 -889,10 +908,18 @@@ func (oc *Controller) updateResource(ob
  		oldCloudPrivateIPConfig := oldObj.(*ocpcloudnetworkapi.CloudPrivateIPConfig)
  		newCloudPrivateIPConfig := newObj.(*ocpcloudnetworkapi.CloudPrivateIPConfig)
  		return oc.reconcileCloudPrivateIPConfig(oldCloudPrivateIPConfig, newCloudPrivateIPConfig)
+ 
+ 	case factory.NamespaceType:
+ 		oldNs, newNs := oldObj.(*kapi.Namespace), newObj.(*kapi.Namespace)
++		// OCP HACK -- required for hybrid overlay
++		if config.HybridOverlay.Enabled && nsHybridAnnotationChanged(oldNs, newNs) {
++			if err := oc.addNamespaceICNIv1(newNs); err != nil {
++				klog.Errorf("Unable to handle legacy ICNIv1 check for namespace %q during update, error: %v",
++					newNs.Name, err)
++			}
++		}
++		// END OCP HACK
+ 		return oc.updateNamespace(oldNs, newNs)
  	}
  
  	return fmt.Errorf("no update function for object type %s", objectsToRetry.oType)

There was also a second merge conflict during the second merge:

[akaris@linux go-controller (merge_9_23_22_b)]$ git show | cat
commit 8d4f50b371a105501a66b54647ce93cadabfb7a0
Merge: f4ddad73a 4577c2613
Author: Andreas Karis <ak.karis@gmail.com>
Date:   Wed Sep 28 16:05:56 2022 +0200

    Merge remote-tracking branch 'upstream/master' into merge_9_23_22_b
    
    Conflicts due to hybrid overlay OCP HACK
    
    Signed-off-by: Andreas Karis <ak.karis@gmail.com>
    
    Conflicts:
            go-controller/hybrid-overlay/pkg/types/types.go
            go-controller/pkg/ovn/pods_test.go

diff --cc go-controller/hybrid-overlay/pkg/types/types.go
index 3b71212f8,9793bbf48..74e437ee6
--- a/go-controller/hybrid-overlay/pkg/types/types.go
+++ b/go-controller/hybrid-overlay/pkg/types/types.go
@@@ -11,10 -11,8 +11,14 @@@ const 
  	HybridOverlayNodeSubnet = HybridOverlayAnnotationBase + "node-subnet"
  	// HybridOverlayDRMAC holds the MAC address of the Distributed Router/gateway
  	HybridOverlayDRMAC = HybridOverlayAnnotationBase + "distributed-router-gateway-mac"
++	// OCP HACK --- required for hybrid overlay
 +	// HybridOverlayExternalGw is a pod and namespace annotation. It holds the IP address of the external gateway to route default traffic
 +	HybridOverlayExternalGw = HybridOverlayAnnotationBase + "external-gw"
 +	// HybridOverlayVTEP is a pod and namespace annotation that holds the IP address of the VTEP
 +	HybridOverlayVTEP = HybridOverlayAnnotationBase + "vtep"
++	// END OCP HACK
+ 	// HybridOverlayDRIP holds the port address to redirect traffic to get to the hybrid overlay
+ 	HybridOverlayDRIP = HybridOverlayAnnotationBase + "distributed-router-gateway-ip"
  	// HybridOverlayVNI is the VNI for VXLAN tunnels between nodes/endpoints
  	HybridOverlayVNI = 4097
  )
diff --cc go-controller/pkg/ovn/pods_test.go
index ef493a59f,9a41a098a..fab378e50
--- a/go-controller/pkg/ovn/pods_test.go
+++ b/go-controller/pkg/ovn/pods_test.go
(...)

In pods_test.go, I had to remove this line as it showed up 2x:

-       hotypes "github.com/ovn-org/ovn-kubernetes/go-controller/hybrid-overlay/pkg/types"

[trozet]

conflict resolution looks good

[andreaskaris]

/retest-required

[andreaskaris]

/retest-required

[trozet]

/retest-required

[tssurya]

/retest-required

[tssurya]

#1281 merged but hoping the merge CI is smart enough to ignore that commit here.

[tssurya]

/retitle OCPBUGS-1318 [Downstream Merge 23-09-2022]

[tssurya]

/lgtm

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD 337a94c and 2 for PR HEAD 8d4f50b in total

[trozet]

/lgtm

[trozet]

/retest-required

[flavio-fernandes]

/retest-required

[flavio-fernandes]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreaskaris, flavio-fernandes, trozet, tssurya

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 [trozet]

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

[trozet]

@andreaskaris I've seen this fail multiple times now:
: [sig-network] [Feature:IPv6DualStack] Granular Checks: Services Secondary IP Family [LinuxOnly] should function for service endpoints using hostNetwork

Perhaps worth launching a cluster bot and running this test manually to see if it passes.

[trozet]

/retest-required

[andreaskaris]

/retest-required

[flavio-fernandes]

/retest-required

[trozet]

dualstack is now passing on my clone PR here:
#1289

with this fix: ovn-org/ovn-kubernetes#3201

[andreaskaris]

/retest-required

[openshift-ci]

@andreaskaris: 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-vsphere-windows	8d4f50b	link	false	/test e2e-vsphere-windows
ci/prow/okd-e2e-gcp-ovn	8d4f50b	link	false	/test okd-e2e-gcp-ovn
ci/prow/e2e-ovn-hybrid-step-registry	8d4f50b	link	false	/test e2e-ovn-hybrid-step-registry
ci/prow/e2e-openstack-ovn	8d4f50b	link	false	/test e2e-openstack-ovn
ci/prow/e2e-hypershift	8d4f50b	link	false	/test e2e-hypershift
ci/prow/e2e-aws-ovn-windows	8d4f50b	link	false	/test e2e-aws-ovn-windows
ci/prow/e2e-metal-ipi-ovn-dualstack	8d4f50b	link	true	/test e2e-metal-ipi-ovn-dualstack

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.