Skip to content

Enable SCTP feature gate - #504

Merged
openshift-merge-robot merged 1 commit into
openshift:masterfrom
dougbtv:sctp-feature-gate
Nov 14, 2019
Merged

openshift-merge-robot merged 1 commit into
openshift:masterfrom
dougbtv:sctp-feature-gate

Conversation

@dougbtv

@dougbtv dougbtv commented Oct 29, 2019

Copy link
Copy Markdown
Contributor

This adds the SCTPSupport to the enabled-by-default feature gate flags.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 29, 2019
@dougbtv

dougbtv commented Oct 29, 2019

Copy link
Copy Markdown
Contributor Author

/cc @squeed

@squeed

squeed commented Oct 30, 2019

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2019
@squeed

squeed commented Oct 30, 2019

Copy link
Copy Markdown
Contributor

@eparis observes this is an alpha feature. We do give users the ability to manage this themselves - see https://github.com/williamcaban/ocp4-sctp/blob/master/00-featuregate.yaml

/lgtm cancel

@fepan

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2019
@danwinship

Copy link
Copy Markdown
Contributor

@eparis observes this is an alpha feature.

It's an alpha feature, but it's not an alpha feature, you know?

From an API stability point of view, the effect of enabling the feature gate is just that SCTP becomes an allowed value for the various port Protocol fields in Service, Pod, and NetworkPolicy, and there is basically a 0% chance that this behavior will change as the feature moves to beta and GA upstream. There's no real danger that we're going to get stuck supporting some dropped or deprecated API as a result of enabling this feature gate now.

From a code-untestedness point of view, the effect of enabling the feature gate is also very small; for one, if you don't actually define any SCTP pods/services/policies, then enabling the feature gate has no effect at all. (The "implementing SCTP" code is already enabled whether the feature gate is enabled or not; it's only the "allowing resources that use SCTP to be created" code that's gated.) If you do create SCTP pods/services/policies, then the only new code that gets hit is code for writing the string "sctp" into iptables/OVS/OVN rather than "tcp" or "udp". The SCTP feature is not getting a ton of use/testing upstream, so if there are bugs in iptables/OVS/OVN's handling of SCTP then they aren't going to get discovered until we actually let people use the feature...

@eparis

eparis commented Nov 14, 2019

Copy link
Copy Markdown
Member

If the network/sdn makes 2 commitments I think we should merge this.
#1 you agree to work upstream to get it beta
#2 you agree that you will support this API even if upstream changes it.
so it's up to Dan/Casey, but I lean towards letting this merge.

@danwinship

Copy link
Copy Markdown
Contributor

yes, i'm working on moving upstream to beta: kubernetes/enhancements#1250
It missed 1.17 but it should happen for 1.18

@danwinship

Copy link
Copy Markdown
Contributor

and yes, we already have all the code, so continuing to implement the feature as-is even if it changes upstream is no problem

@eparis

eparis commented Nov 14, 2019

Copy link
Copy Markdown
Member

sounds like we should rebase and merge this

This adds the SCTPSupport to the enabled-by-default feature gate flags.
@eparis

eparis commented Nov 14, 2019

Copy link
Copy Markdown
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2019
@openshift-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dougbtv, eparis

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:

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2019
@openshift-merge-robot
openshift-merge-robot merged commit 4efd1a5 into openshift:master Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants