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

Follow up to PR 5360 #5705

Merged
merged 4 commits into from Oct 20, 2017
Merged

Follow up to PR 5360 #5705

merged 4 commits into from Oct 20, 2017

Conversation

mburke5678
Copy link
Contributor

See #5360

@mburke5678
Copy link
Contributor Author

@e-minguez Can you take a look to make sure I haven't changed the meaning of anything?

Also, should the Flannel iptables rules section be in the To enable flannel procedure above it?

be disabled on the ports that carry flannel traffic.
different from that on the port itself. Flannel creates virtual MACs and IP
addresses and must send and receive packets on the port, so port security must
be disabled on the ports that carry Flannel traffic.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flannel vs flannel


+
When creating the {product-title} instances, disable both port security and security
groups in the ports where the container network flannel interface will be:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flannel vs Flannel

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the flannel docs, they use flannel with lower case f except at the beginning of sentences where they use Flannel. I tried to match that.

the time of writing this paper.
[NOTE]
====
Custom networking CIDR for pods and services will be supported in a future release.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Custom networking CIDR for pods and services using flannel will be supported in a future release.

====
The `iptables-save` command saves all the current _in memory_ iptables rules.
However, because Docker, Kubernetes and {product-title} create a high number of iptables rules
(services, etc.) not designed to be persisted, saving these rules can result in a large
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say in a "bad" file instead large, because the problem is if you persist the k8s services, docker, etc. rules, you will be screwed 🗡

@e-minguez
Copy link
Contributor

@mburke5678 I've added a few comments, otherwise LGTM

@mburke5678
Copy link
Contributor Author

@e-minguez should the Flannel iptables rules section be in the To enable flannel procedure above it? Is that a required process to enable flannel, it seems that it should be in the steps.

@e-minguez
Copy link
Contributor

Yes, it is required, but it should be performed after the OCP installation, so it is ok to be moved into the enable flannel procedure as a subsection or something similar.
Thanks

When creating the OpenStack instances, disable both port-security and security
groups in the ports where the container network Flannel interface will be:

. When creating the {product-title} instances, disable both port security and security
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be changed? Our product title is not OpenStack?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe:
"When creating the {product-title} instances in OpenStack,..."

@mburke5678
Copy link
Contributor Author

@e-minguez If the user needs to configure the Ansible playbook before installing OpenShift, we should probably add these steps to the Prerequisites or Host Preparation files before the install?
Set the following variables in your Ansible inventory file before running the
installation

https://docs.openshift.com/container-platform/3.6/install_config/install/host_preparation.html

Please let me know what you think so that we can close this PR!

@e-minguez
Copy link
Contributor

Currently, the "openshift_use_flannel" ansible variable documentation has a link to the Flannel section on the SDN chapter[1] which I think it is enough.

[1] https://docs.openshift.com/container-platform/3.6/install_config/install/advanced_install.html#configuring-cluster-variables

@mburke5678
Copy link
Contributor Author

Beautiful. Without further objection, I will merge!

@mburke5678 mburke5678 merged commit 77897ff into openshift:master Oct 20, 2017
@mburke5678 mburke5678 deleted the follow-up-5360 branch October 20, 2017 17:41
@mburke5678
Copy link
Contributor Author

No rev history needed.

@vikram-redhat
Copy link
Contributor

@mburke5678 - 4 commits here which should have been squashed into one. Also, this didn't get picked into the right branches once merged into master or the labels applied. Finally, the original dev PR needs to get the to_followup label removed.

@vikram-redhat
Copy link
Contributor

I am reverting this as this is causing issues when doing a weekly release due to the issues mentioned in the previous comment from me. @mburke5678 will do a follow up to fix these issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants