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

flannel-docs: Updating flannel Documents files. #679

Merged
merged 1 commit into from
May 4, 2017

Conversation

zbwright
Copy link
Contributor

Please add content as necessary. In particular, there's some placeholder text in running - "Zero-downtime restarts - moves to an upgrade section (or standalone doc) with extra k8s content"

@zbwright zbwright requested review from tomdee and joshix April 13, 2017 23:05
@zbwright zbwright changed the title flannel-docs: Updating running.md and backends.md flannel-docs: Updating flannel Documents files. Apr 18, 2017
@@ -1,34 +1,36 @@
## Configuration
# Configuration
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to add a note here that flannel ships with etcd2 - for etcd3 go to xxx.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand this comment? We should have info on setting kubernetes configuration, but etcd3 support isn't something we have at the moment,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok - I was overly worried about the etcd2 > 3 stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

by way of explanation, @zbwright, etcd clients like flannel can use the etcd v3 or v2 apis when communicating with an etcd v3 service. So and etcd v3 binary can serve either the v2 http api and the v3 grpc api to a client. The two apis cannot be used interchangeably and they have separate data stores.

(No change required for this comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 So I think this comment is resolved right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. resolved.

@tomdee
Copy link
Contributor

tomdee commented Apr 26, 2017

@zbwright It would be better to split this into smaller PRs (if the changes don't overlap) as that makes it much easier for me to review and merge. You could even go as far as one PR per file/document.

@tomdee
Copy link
Contributor

tomdee commented Apr 26, 2017

So far I've looked at Documentation/alicloud-vpc-backend.md and Documentation/aws-vpc-backend.md and both look fine

@zbwright
Copy link
Contributor Author

@tomdee sorry about that - still learning how this works. I will certainly do smaller PRs next time. Would you like me to figure out how to do it to this one as well?

Copy link
Contributor

@tomdee tomdee left a comment

Choose a reason for hiding this comment

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

This is a great incremental improvement, thanks for making these changes. There are a few comments that should be addressed and a conflict that needs to be resolved.

Note: Currently, AliVPC limit the number of entries per route table to 50.
# Backends

Flannel may be paired with several different backends. Once set, the backend cannot be changed at runtime.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it should say "should not" instead of "cannot". People can change it and if they really know what they're doing it might be OK, but we don't support or recommend it.


Flannel may be paired with several different backends. Once set, the backend cannot be changed at runtime.

VXLAN is the default choice. host-gw is recommended for more experienced users who want the performance improvement. UDP is suggested for debugging only.
Copy link
Contributor

Choose a reason for hiding this comment

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

This recommendation is really important to get right so apologies if this seems nit-picky!

  • In the code, UDP is still the default (for historical and maybe compatibility reasons).
  • host-gw should mention the infrastructure restriction too.

So, maybe something like this

VXLAN is the recommended choice. host-gw is recommended for more experienced users who want the performance improvement and who's infrastructure support it (typically it can't be used in cloud environments). UDP is suggested for debugging only or for very old kernels that don't support vxlan.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit picky and precise is good.


Use host-gw to create IP routes to subnets via remote machine IPs. Requires direct layer2 connectivity between hosts running flannel.

host-gw provides good performance, with few dependencies, and easy set up. http://machinezone.github.io/research/networking-solutions-for-kubernetes/
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably not include this machinezone link as it doesn't give a very clear view of current vxlan performance

* `Type` (string): `udp`
* `Port` (number): UDP port to use for sending encapsulated packets. Defaults to 8285.

### AliVPC
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be moved under experimental too

@@ -1,34 +1,36 @@
## Configuration
# Configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand this comment? We should have info on setting kubernetes configuration, but etcd3 support isn't something we have at the moment,


* `Backend` (dictionary): Type of backend to use and specific configurations for that backend.
The list of available backends and the keys that can be put into the this dictionary are listed below.
Defaults to "udp" backend.
Defaults to `udp` backend.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth going through the docs and making sure that backends are consistently backticked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I've done it, but will do again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I don't know why, but I cannot reply to your previous comment. The etcd comment must have been original to the doc. Where can I find info on setting up k8s config?

@@ -0,0 +1,36 @@
# Reporting bugs
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks for adding this.

Please be aware of the following flannel runtime limitations.
* The datastore type cannot be changed.
* The backend type cannot be changed. (It can be changed if you stop all workloads and restart all flannel daemons.)
* You can change the the subnetlen/subnetmin/subnetmax with a daemon restart. (Subnets can be changed with caution. If pods are already using IP addresses outside the new range they will stop working.)
Copy link
Contributor

Choose a reason for hiding this comment

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

the the


If you're running on CoreOS, use `cloud-config` to set `coreos.flannel.interface` to `$public_ipv4`.

## Zero-downtime restarts - moves to an upgrade section (or standalone doc) with extra k8s content
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooops, this shouldn't be in the title

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing comment for now - we can address later.

@@ -0,0 +1,36 @@
# Reporting bugs

If any part of the flannel project has bugs or documentation mistakes, please let us know by [opening an issue][flannel-issue]. We treat bugs and mistakes very seriously and believe no issue is too small. Before creating a bug report, please check that an issue reporting the same problem does not already exist.
Copy link
Contributor

Choose a reason for hiding this comment

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

strike second sentence

@@ -1,34 +1,36 @@
## Configuration
# Configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

by way of explanation, @zbwright, etcd clients like flannel can use the etcd v3 or v2 apis when communicating with an etcd v3 service. So and etcd v3 binary can serve either the v2 http api and the v3 grpc api to a client. The two apis cannot be used interchangeably and they have separate data stores.

(No change required for this comment)

Also, to avoid interruptions during restart, the configuration must not be changed (e.g. VNI, --iface values).


[coreos-etcd]: https://github.com/coreos/etcd/releases/latest
Copy link
Contributor

Choose a reason for hiding this comment

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

….md. TOC rearranging for ReadMe.

Adding updated Flannel info.

Editing for format.

Edits to building, config, and running. Adding reporting_bugs.md.

Edited for etcd3 accuracy.

minor changes

minor edit. link changed.
@tomdee tomdee merged commit 9eecece into flannel-io:master May 4, 2017
@zbwright zbwright deleted the flannel-reorg branch May 5, 2017 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants