Add sctp service support #1137

trozet · 2020-03-13T21:29:41Z

No description provided.

trozet · 2020-03-13T21:32:05Z

Adding WIP tag while I test it.

go-controller/pkg/node/gateway_localnet.go

go-controller/pkg/ovn/endpoints.go

go-controller/pkg/ovn/service.go

trozet · 2020-03-20T17:12:00Z

Removing WIP tag. This patch is ready for review. It will fail CI until corresponding OVN patch is merged and in f31.

trozet · 2020-03-20T17:12:20Z

@girishmg @dcbw @danwinship PTAL

danwinship · 2020-03-20T20:14:23Z

lgtm but needs to be rebased

girishmg · 2020-03-20T21:01:07Z

@trozet if CI is going to fail, then after this integrates into master will all the subsequent PRs then fail?

trozet · 2020-03-20T21:26:30Z

@trozet if CI is going to fail, then after this integrates into master will all the subsequent PRs then fail?

@girishmg I don't want it to be merged, I just want reviews so that when OVN part is ready, this is ready to go.

trozet · 2020-03-31T17:19:39Z

@dcbw @girishmg PTAL

The control-plane e2e tests are failing everhywere because the VM runs out of disk space during the go mod download. Has nothing to do with this patch. I just verified pod to pod SCTP, cluster IP service SCTP, and nodePort SCTP service on my local setup.

dist/images/Dockerfile.fedora

dcbw · 2020-04-01T03:58:15Z

/lgtm

trozet · 2020-04-01T13:13:27Z

@girishmg can you PTAL ASAP?

girishmg · 2020-04-01T14:12:17Z

@trozet yes will take a look

girishmg

@trozet @dcbw how is this going to work on deployments running OVN-20-03 without SCTP support. The cluster will not come up with checks like this:

	if oc.SCTPLoadBalancerUUID == "" {
		return fmt.Errorf("SCTP cluster load balancer not created")
	}

With this PR, we are pretty much saying that ovn-kubernetes is only supported on post-split OVN and not even on the initial OVN-20-03 release

Should we add a feature check sctpProtocolSupported before doing SCTP related stuff?

go-controller/pkg/util/gateway-init.go

go-controller/pkg/util/gateway-cleanup.go

go-controller/pkg/ovn/master.go

girishmg · 2020-04-01T17:48:54Z

@trozet @dcbw i stopped reviewing this PR since we need an answer for this #1137 (review)

trozet · 2020-04-01T17:53:01Z

@trozet @dcbw how is this going to work on deployments running OVN-20-03 without SCTP support. The cluster will not come up with checks like this:
	if oc.SCTPLoadBalancerUUID == "" {
		return fmt.Errorf("SCTP cluster load balancer not created")
	}
With this PR, we are pretty much saying that ovn-kubernetes is only supported on post-split OVN and not even on the initial OVN-20-03 release

@girishmg this kind of triggers in my mind a larger discussion around having real releases/versioning in this repo, and having master always work with latest master OVN. Otherwise we are going to be carrying a lot backwards compatibility/maintenance in the future.

Should we add a feature check sctpProtocolSupported before doing SCTP related stuff?

@girishmg hmm, I could maybe add a way to check if the cluster has sctp feature enabled? what do you think?

danwinship · 2020-04-01T18:43:02Z

Does this PR make ovn-kubernetes require super-recent OVN just to start, or is it just that it requires super-recent OVN if you want SCTP load balancers to actually work? The latter seems fine to me.

trozet · 2020-04-01T18:46:00Z

Does this PR make ovn-kubernetes require super-recent OVN just to start, or is it just that it requires super-recent OVN if you want SCTP load balancers to actually work? The latter seems fine to me.

Requires it to even start. We make the load balancers when ovn-k8s starts, so if we cannot make the one for each protocol we will throw an error.

trozet · 2020-04-02T14:54:07Z

For an OVN version that doesnt support SCTP. The svc will get marked with an event like this:
[trozet@trozet ~]$ kubectl describe svc my-service
Name: my-service
Namespace: default
Labels:
Annotations:
Selector: role=sctpserver
Type: NodePort
IP: 10.96.34.129
Port: 62324/SCTP
TargetPort: 62324/SCTP
NodePort: 32173/SCTP
Endpoints: 10.244.1.4:62324
Session Affinity: None
External Traffic Policy: Cluster
Events:
Type Reason Age From Message

Warning Unsupported protocol error 17s controlplane SCTP protocol is unsupported by this version of OVN

trozet · 2020-04-02T14:54:44Z

@dcbw @danwinship @girishmg PTAL If the fixup looks ok then I will squash it and rebase my patch for final approval.

dcbw · 2020-04-02T21:41:30Z

go-controller/pkg/ovn/master.go

@@ -107,6 +107,34 @@ func (oc *Controller) SetupMaster(masterNodeName string) error {
 		return err
 	}

+	// Determine SCTP support


@trozet could we put this in a function of it's own? like oc.SCTPSupport, err = detectSCTPSupport() or something like that

yeah I will do that when i squash/rebase. Will just wait for @girishmg review before I do that.

dcbw · 2020-04-02T21:47:45Z

@trozet looks good to me, just one comment about pulling the "do we have SCTP support" into a separate function

girishmg · 2020-04-03T02:07:05Z

@trozet LGTM. Can you please collapse the commit 1 and 3 into 1, so that all the SCTP related changes are in one change and the fedora.dev changes can be a separate commit.

trozet · 2020-04-03T14:45:42Z

@girishmg @dcbw PTAL I just squashed the fix up, and moved the detect sctp support to its own util function

trozet · 2020-04-03T15:19:11Z

hmm hold while I look into this 1 failure

trozet · 2020-04-03T16:31:58Z

Looking at the logs it looks like we never got the add endpoint event for 10.108.245.242 cluster ip session-affinity-service. Unfortunately we do not have great debug logs in endpoints code so I can't be 100% certain. I'm going to add another commit to improve debug logging there to this PR and see if we hit the failure again. I wonder if this has to do with our recent updates to use newer kapi and KIND still on the old kubernetes version.

github-actions · 2020-04-03T16:57:22Z

Coverage increased (+0.2%) to 39.256% when pulling cc74254 on trozet:add_sctp_service_support into 2c50f13 on ovn-org:master.

dcbw · 2020-04-03T17:07:16Z

I wonder if this has to do with our recent updates to use newer kapi and KIND still on the old kubernetes version.

@trozet the client-go update should fix ovnkube itself for missing events on relist, but would have no effect on the kubernetes version that KIND is using. The fix in question should also only have an effect on relist, it doesn't change DeltaFIFO at all for normal processing.

trozet · 2020-04-03T17:23:08Z

I wonder if this has to do with our recent updates to use newer kapi and KIND still on the old kubernetes version.

@trozet the client-go update should fix ovnkube itself for missing events on relist, but would have no effect on the kubernetes version that KIND is using. The fix in question should also only have an effect on relist, it doesn't change DeltaFIFO at all for normal processing.

I meant more along the lines that we updated all the kapi versions:
#1199

So of course now the test passes when I add the logging :)

@dcbw we can either A) merge this and if we hit this again in CI we will have the improved logging to make more sense of it, or B) run manually retrigger these CI tests a few more times today and see if we hit it. What do you think?

This patch will detect if the current OVN version supports SCTP and then enable it for K8S services if so. If no support is detected and a user creates an SCTP service in k8s, a warning event will be recorded for the resource in k8s. Also cleans up and consolidates a bunch of duplicate logic for each protocol. Additionally, removes using self-defined constants for protocols and uses kapi versions for services. Closes: ovn-org#1120 Closes: ovn-org#1124 Signed-off-by: Tim Rozet <trozet@redhat.com>

Signed-off-by: Tim Rozet <trozet@redhat.com>

Adds debug logging for EPs. Signed-off-by: Tim Rozet <trozet@redhat.com>

trozet · 2020-04-06T13:43:34Z

@dcbw as not able to reproduce the service failure again. Let's merge this and if we see it again in CI we can debug it further then.

dcbw · 2020-04-06T14:09:40Z

@dcbw as not able to reproduce the service failure again. Let's merge this and if we see it again in CI we can debug it further then.

+1

…ter_annot_48 [release-4.8] Bug 2097018: Duplicated IPs can be assigned to multiple Pods

trozet force-pushed the add_sctp_service_support branch from 8311a55 to 6e0d764 Compare March 13, 2020 21:31

trozet changed the title ~~Add sctp service support~~ [WIP] Add sctp service support Mar 13, 2020

danwinship reviewed Mar 17, 2020

View reviewed changes

go-controller/pkg/node/gateway_localnet.go Outdated Show resolved Hide resolved

go-controller/pkg/ovn/endpoints.go Outdated Show resolved Hide resolved

go-controller/pkg/ovn/service.go Outdated Show resolved Hide resolved

trozet force-pushed the add_sctp_service_support branch from 6e0d764 to 03d7205 Compare March 20, 2020 17:11

trozet changed the title ~~[WIP] Add sctp service support~~ Add sctp service support Mar 20, 2020

trozet force-pushed the add_sctp_service_support branch 2 times, most recently from 965a7c1 to 4f5ed91 Compare March 31, 2020 16:38

dcbw reviewed Mar 31, 2020

View reviewed changes

dist/images/Dockerfile.fedora Show resolved Hide resolved

girishmg reviewed Apr 1, 2020

View reviewed changes

dcbw reviewed Apr 2, 2020

View reviewed changes

trozet force-pushed the add_sctp_service_support branch from 9fa2ec4 to 02321fe Compare April 3, 2020 14:36

trozet force-pushed the add_sctp_service_support branch 2 times, most recently from de0c777 to b3aa5a9 Compare April 3, 2020 17:34

trozet added 2 commits April 3, 2020 16:50

Ensure we pick up OVN with SCTP support for F31 CI

5fee58a

Signed-off-by: Tim Rozet <trozet@redhat.com>

trozet force-pushed the add_sctp_service_support branch 7 times, most recently from 0a90c32 to ab0378d Compare April 4, 2020 04:20

Improve debug logging for Endpoint events

c878a37

Adds debug logging for EPs. Signed-off-by: Tim Rozet <trozet@redhat.com>

trozet force-pushed the add_sctp_service_support branch from ab0378d to c878a37 Compare April 6, 2020 13:07

dcbw merged commit 8dd8f5c into ovn-org:master Apr 6, 2020

kyrtapz pushed a commit to kyrtapz/ovn-kubernetes that referenced this pull request Nov 21, 2022

Merge pull request ovn-org#1137 from martinkennelly/ensure_network_af…

345e744

…ter_annot_48 [release-4.8] Bug 2097018: Duplicated IPs can be assigned to multiple Pods

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sctp service support #1137

Add sctp service support #1137

trozet commented Mar 13, 2020

trozet commented Mar 13, 2020

trozet commented Mar 20, 2020

trozet commented Mar 20, 2020

danwinship commented Mar 20, 2020

girishmg commented Mar 20, 2020 •

edited

trozet commented Mar 20, 2020

trozet commented Mar 31, 2020

dcbw commented Apr 1, 2020

trozet commented Apr 1, 2020

girishmg commented Apr 1, 2020

girishmg left a comment

girishmg commented Apr 1, 2020 •

edited

trozet commented Apr 1, 2020 •

edited

danwinship commented Apr 1, 2020

trozet commented Apr 1, 2020

trozet commented Apr 2, 2020

trozet commented Apr 2, 2020

dcbw Apr 2, 2020

trozet Apr 3, 2020

dcbw commented Apr 2, 2020

girishmg commented Apr 3, 2020

trozet commented Apr 3, 2020

trozet commented Apr 3, 2020

trozet commented Apr 3, 2020

github-actions bot commented Apr 3, 2020

dcbw commented Apr 3, 2020

trozet commented Apr 3, 2020

trozet commented Apr 6, 2020

dcbw commented Apr 6, 2020

Add sctp service support #1137

Add sctp service support #1137

Conversation

trozet commented Mar 13, 2020

trozet commented Mar 13, 2020

trozet commented Mar 20, 2020

trozet commented Mar 20, 2020

danwinship commented Mar 20, 2020

girishmg commented Mar 20, 2020 • edited

trozet commented Mar 20, 2020

trozet commented Mar 31, 2020

dcbw commented Apr 1, 2020

trozet commented Apr 1, 2020

girishmg commented Apr 1, 2020

girishmg left a comment

Choose a reason for hiding this comment

girishmg commented Apr 1, 2020 • edited

trozet commented Apr 1, 2020 • edited

danwinship commented Apr 1, 2020

trozet commented Apr 1, 2020

trozet commented Apr 2, 2020

trozet commented Apr 2, 2020

dcbw Apr 2, 2020

Choose a reason for hiding this comment

trozet Apr 3, 2020

Choose a reason for hiding this comment

dcbw commented Apr 2, 2020

girishmg commented Apr 3, 2020

trozet commented Apr 3, 2020

trozet commented Apr 3, 2020

trozet commented Apr 3, 2020

github-actions bot commented Apr 3, 2020

dcbw commented Apr 3, 2020

trozet commented Apr 3, 2020

trozet commented Apr 6, 2020

dcbw commented Apr 6, 2020

girishmg commented Mar 20, 2020 •

edited

girishmg commented Apr 1, 2020 •

edited

trozet commented Apr 1, 2020 •

edited