Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws: conditionally open either ovn-kubernetes or openshift-sdn ports #1905

Closed
wants to merge 1 commit into from

Conversation

danwinship
Copy link
Contributor

alternative to #1563; this makes the behavior conditional on networkType (and adds a bunch of notes pointing out that this is just for 4.2; for 4.3 the plan is that cluster-network-operator will do this). It works, though the implementation may not be ideal; I was figuring out terraform as I went.

/cc @dcbw @squeed

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 26, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: danwinship
To complete the pull request process, please assign wking
You can assign the PR to them by writing /assign @wking in a comment when ready.

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

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
Copy link
Contributor

@danwinship: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 be67154 link /test e2e-aws-scaleup-rhel7

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@squeed
Copy link
Contributor

squeed commented Jun 26, 2019

A bit of background for this: OVN is Tech Preview for 4.2, and there just aren't enough cycles to get port opening in to the CNO. When it goes GA in 4.3, we can rip this all out (and support other platforms, naturally).

@dcbw
Copy link
Contributor

dcbw commented Jun 26, 2019

@squeed @danwinship does this obsolete SDN-461 "OVN: move database ports to local port range (9000-9999)" ?

@squeed
Copy link
Contributor

squeed commented Jun 26, 2019

/assign wking

@abhinavdahiya
Copy link
Contributor

/unassign @wking

/assgin @abhinavdahiya

I think we need to have a bigger discussion on how to manage or setup cloud networking for k8s networking.

@abhinavdahiya abhinavdahiya assigned abhinavdahiya and unassigned wking Jun 26, 2019
@squeed
Copy link
Contributor

squeed commented Jun 26, 2019

I think we need to have a bigger discussion on how to manage or setup cloud networking for k8s networking.

For sure. We've chatted about it with various folks, and IMHO the way forward is pretty clear - the CNO should manage network resources as much as reasonably possible. The Openstack team has lead the way in this regard, and set the pattern for other providers to follow suit. We now have CNO code that gets a OpenStack credential and reconciles the desired networking state. I'm pretty pleased, honestly.

Our desire is to bring other platforms to this standard, but it's just a matter of resources + time + prioritization (only 3 dimensions! that seems solvable...). For 4.3, we will be able to revert this PR. But we need to get OVN in the hands of partners + customers for 4.2 for evaluation.

The open question is where to draw the line between the installer and CNO. Obvious challenges are chicken-egg bootstrapping and terraform having person-decades of bugfixes we don't want to discover ourselves.

@danwinship
Copy link
Contributor Author

danwinship commented Jun 26, 2019

We've chatted about it with various folks

Much of the discussion is in #1218

For 4.3, we will be able to revert this PR.

More than "revert". We'd be removing both the new OVN port stuff and the old VXLAN port stuff.

The open question is where to draw the line between the installer and CNO.

For openshift-sdn and ovn-kubernetes, I don't think there's much of an open question; the installer sets up node-to-node networking and CNO modifies that setup to support pod networking as well. However, as seen later on in #1218, it may be more complicated for some other plugins, which may need to change some attributes of the masters before they're created.

@squeed
Copy link
Contributor

squeed commented Jun 27, 2019

The installer sets up node-to-node networking and CNO modifies that setup to support pod networking as well.

As a starting-point, yes. But I could see us wanting to adopt the apiserver load balancer. OTOH that could be the domain of an expanded cloud-operator.

@danwinship
Copy link
Contributor Author

Casey says the installer team would rather go with the non-conditional version.

@danwinship danwinship closed this Jul 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants