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

kubefed v1.6 update #2899

Merged
merged 3 commits into from
Mar 21, 2017
Merged

Conversation

madhusudancs
Copy link
Contributor

@madhusudancs madhusudancs commented Mar 17, 2017

Ref: Issue #2909

This change is Reviewable

@madhusudancs madhusudancs added this to the 1.6 milestone Mar 17, 2017
@madhusudancs madhusudancs self-assigned this Mar 17, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 17, 2017
@chenopis chenopis changed the base branch from master to release-1.6 March 18, 2017 00:42
@madhusudancs madhusudancs changed the title [WIP] kubefed v1.6 update kubefed v1.6 update Mar 18, 2017
@madhusudancs
Copy link
Contributor Author

This is ready for review now.

@madhusudancs
Copy link
Contributor Author

@perotinus I can't put you in the Assignees list yet, but please review for technical accuracy.

@shashidharatd @irfanurrehman please review for technical accuracy too.

@madhusudancs
Copy link
Contributor Author

cc @kubernetes/sig-docs-maintainers

@madhusudancs
Copy link
Contributor Author

cc @kubernetes/docs

[`PersistentVolumeClaim`](/docs/user-guide/persistent-volumes/#persistentvolumeclaims)
to the federation control plane that it bootstraps. We are planning to
support this in a future version of `kubefed`.

Choose a reason for hiding this comment

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

@madhusudancs, in L235-L238, the documentation is slightly confusing. when --etcd-persistent-storage=false we disable persistent storage (either dynamic or statically provisioned pv will be disabled).

And for L248-L251, If user creates statically provisioned pv with exact same requirements of pvc, the current mechanism already supports binding to statically provisioned pv. only that the pvc configuration may not be know to user.

Copy link
Contributor Author

@madhusudancs madhusudancs Mar 18, 2017

Choose a reason for hiding this comment

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

@madhusudancs, in L235-L238, the documentation is slightly confusing. when --etcd-persistent-storage=false we disable persistent storage (either dynamic or statically provisioned pv will be disabled).

Ok, first of all I misunderstood this. Thanks for clarifying.

The reason why I misunderstood this was because we use the annotation "volume.alpha.kubernetes.io/storage-class": "yes" in our PVC and I thought it wasn't possible to create a PV manually with that storage class. So I thought one has to pass --etcd-persistent-storage=false and create a PV and a PVC themselves. Isn't that correct?

And for L248-L251, If user creates statically provisioned pv with exact same requirements of pvc, the current mechanism already supports binding to statically provisioned pv. only that the pvc configuration may not be know to user.

If it possible to just create a PV that our PVC can bind to, then we can document our PVC's storage class and access modes here so that people can create a PV for that configuration. Then we can ask people to set --etcd-persistent-storage=false only when they don't need persistence.

Choose a reason for hiding this comment

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

if --etcd-persistent-storage=false PVC is not created by kubefed, thereby disabling persistent storage altogether.

if user creates required PV statically beforehand matching the requirements of PVC created by kubefed, then PVC is able to bind to the Statically created PV. So definitely we should be documenting the PVC's storage class, access modes and size. This will enable users to create PV beforehand, if necessary.

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 understood this after your previous comment. But my question is, is it possible to create a PV manually with "volume.alpha.kubernetes.io/storage-class": "yes"?

Choose a reason for hiding this comment

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

I am not sure of this @madhusudancs. In my local setup i had created a PV without this storage-class statically and i was able to bind to the PVC created by kubefed.
So my guess is this storage-class applies to dynamic provisioning of PV, something meaningful for cloud-provider. Just a guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shashidharatd Oh I see. That's useful information. Thanks!

Documented the PVC configuration along with a description. PTAL.

@shashidharatd
Copy link

In section "Turning down the federation control plane", the last one, i saw kubefed is referenced as alpha release. we can search for keyword alpha in the documentation to replace it with beta.

```ini
[Global]
etcd-endpoints = http://etcd-cluster.ns:2379
zones = example.com

Choose a reason for hiding this comment

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

Do we need to add a trailing dot here to avoid inconsistencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, if a trailing dot does not affect anything. Does it?

Choose a reason for hiding this comment

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

Trailing dot does not seem to affect CoreDNS configuration. so probably we should use trailing dot consistent with other examples.

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 will fix it then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

--host-cluster-context=rivendell \
--dns-provider="google-clouddns" \
--dns-zone-name="example.com." \
--apiserver-arg-overrides="--anonymous-auth=false, --v=4" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Although it might work, I think it might not be a good idea to provide an example with space in it (I havent tested it though, but I am guessing this wont even work).
it can better be given as
--apiserver-arg-overrides="--anonymous-auth=false,--v=4" \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, fair enough. Fixed.

```shell
kubefed init fellowship \
--host-cluster-context=rivendell \
--dns-provider="coredns" \
Copy link
Contributor

Choose a reason for hiding this comment

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

For the sake of further clarity; do we also need to specify somewhere that dns-provider is a mandatory flag (or kubefed init wont work properly, if a known dns-provider is not specified) and then the dns-provider-conf becomes a mandatory option if the dns-provider (among the known providers) is coredns.

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 don't think it is necessary to have all sorts of details about the various flags. Some of these belong to the reference documentation for the command.

Since --dns-provider is mandatory, the command won't even pass the validation in the beginning. So people are going to figure that out sooner than later, i.e. before modifying anything in the host cluster. So I want to leave that detail out.

Also, CoreDNS config is not mandatory. As in, it is not enforced, so I don't want to put that in the documentation here. It should be probably mentioned in the CoreDNS doc though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough!

the `--api-server-service-type=NodePort` flag. You can also specify
the preferred address to advertise the federation API server by
passing the `--api-server-advertise-address=<IP-address>`
flag. Otherwise, one of the host cluster's node address is chosen as
Copy link
Contributor

Choose a reason for hiding this comment

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

do we also need to say, that one of the external IP's will be chosen and/or a catch, if a publicIP or a legacyIP is not available, the auto assignment of the IP will remain empty.

@@ -121,8 +283,19 @@ To use `kubefed join`, you'll need to provide the name of the cluster
you want to add to the federation, and the `--host-cluster-context`
for the federation control plane's host cluster.

> Note: The name that you provide to the `join` command is used as the
joining cluster's identity in federation. This name should adhere to
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a naming rules section below:
Does it make more sense to list all this detail there, and a one liner note here something like:
Note: The name that you provide to the `join` command is used as the joining cluster's identity in federation. This name needs to adhere to specific rules, some details of which are listed in naming rules section below:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This note is in response to the direct feedback I got from different people (issue #2353). It is not clear why you need to name a cluster while joining it and all the aliasing that happens between context names and cluster names in federation. That's why a note here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, Please leave it as it is now!

@irfanurrehman
Copy link
Contributor

adding one more comment, which is on a portion not part of the diff here:
comment on line 10.
Suggestion to update this line
Kubernetes version 1.5 includes a new command line tool called
to
Kubernetes from version 1.5 onwards includes a command line tool called

Copy link
Contributor

@devin-donnelly devin-donnelly left a comment

Choose a reason for hiding this comment

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

Just a couple of minor docs nits, otherwise LGTM.

@@ -16,7 +16,7 @@ existing federation control plane.
This guide explains how to administer a Kubernetes Cluster Federation
using `kubefed`.

> Note: `kubefed` is an alpha feature in Kubernetes 1.5.
> Note: `kubefed` is in beta as of Kubernetes 1.6.
Copy link
Contributor

Choose a reason for hiding this comment

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

"is a beta feature in Kubernetes 1.6." Please follow the established format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

--controllermanager-arg-overrides="--controllers=services=false"
```

### Configuring DNS provider
Copy link
Contributor

Choose a reason for hiding this comment

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

"Configuring a DNS provider"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


### Configuring DNS provider

Federated service controller programs a DNS provider to expose
Copy link
Contributor

Choose a reason for hiding this comment

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

"Federated service controller" -> "The Federated service controller"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@devin-donnelly
Copy link
Contributor

Once all comments are addressed, we can merge. Overall great work @madhusudancs !

Copy link
Contributor Author

@madhusudancs madhusudancs left a comment

Choose a reason for hiding this comment

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

@devin-donnelly PTAL again. Added some additional text based on the comments here.

@shashidharatd @irfanurrehman PTAL too.

@@ -16,7 +16,7 @@ existing federation control plane.
This guide explains how to administer a Kubernetes Cluster Federation
using `kubefed`.

> Note: `kubefed` is an alpha feature in Kubernetes 1.5.
> Note: `kubefed` is in beta as of Kubernetes 1.6.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

--host-cluster-context=rivendell \
--dns-provider="google-clouddns" \
--dns-zone-name="example.com." \
--apiserver-arg-overrides="--anonymous-auth=false, --v=4" \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, fair enough. Fixed.

--controllermanager-arg-overrides="--controllers=services=false"
```

### Configuring DNS provider
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


### Configuring DNS provider

Federated service controller programs a DNS provider to expose
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

```shell
kubefed init fellowship \
--host-cluster-context=rivendell \
--dns-provider="coredns" \
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 don't think it is necessary to have all sorts of details about the various flags. Some of these belong to the reference documentation for the command.

Since --dns-provider is mandatory, the command won't even pass the validation in the beginning. So people are going to figure that out sooner than later, i.e. before modifying anything in the host cluster. So I want to leave that detail out.

Also, CoreDNS config is not mandatory. As in, it is not enforced, so I don't want to put that in the documentation here. It should be probably mentioned in the CoreDNS doc though.

@@ -121,8 +283,19 @@ To use `kubefed join`, you'll need to provide the name of the cluster
you want to add to the federation, and the `--host-cluster-context`
for the federation control plane's host cluster.

> Note: The name that you provide to the `join` command is used as the
joining cluster's identity in federation. This name should adhere to
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This note is in response to the direct feedback I got from different people (issue #2353). It is not clear why you need to name a cluster while joining it and all the aliasing that happens between context names and cluster names in federation. That's why a note here.

[`PersistentVolumeClaim`](/docs/user-guide/persistent-volumes/#persistentvolumeclaims)
to the federation control plane that it bootstraps. We are planning to
support this in a future version of `kubefed`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shashidharatd Oh I see. That's useful information. Thanks!

Documented the PVC configuration along with a description. PTAL.

@madhusudancs
Copy link
Contributor Author

Once all comments are addressed, we can merge. Overall great work @madhusudancs !

@devin-donnelly thanks!

Copy link
Contributor

@perotinus perotinus left a comment

Choose a reason for hiding this comment

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

LGTM.

arguments to federation API server and federation controller manager.
Some of these arguments are derived from `kubefed init`'s flags.
However, you can override these command line arguments by passing
them via the appropriate override flags.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/them/a comma-separated list of arguments/

Or, if not here, can you make it clear somewhere other than the example that this should be a comma-separated list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"override these command line arguments by passing a comma-separated list of arguments" sounds a little awkward. It also probably redundant. So leaving this as is.

label.
The cluster name you supply to `kubefed join` must be a valid
[RFC 1035](https://www.ietf.org/rfc/rfc1035.txt) label and are
enumerated in the [Identifiers doc](/docs/user-guide/identifiers/#names).
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 quite sure I follow what you're saying here. Is it that the identifier must be a valid RFC 1035 label and adhere to the requirements enumerated in the Identifiers page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. And they are the same. Identifiers doc enumerates RFC 1035 rules.

Any suggestions for improving this or to make this sound better?


`kube-dns` configuration must be updated in each joining cluster to
enable federated service discovery. If the joining Kubernetes cluster
is version 1.5 or newer and your `kubefed` version 1.6 or later, then
Copy link
Contributor

Choose a reason for hiding this comment

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

s/later/newer/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


`kube-dns` configuration must be updated in each joining cluster to
enable federated service discovery. If the joining Kubernetes cluster
is version 1.5 or newer and your `kubefed` version 1.6 or later, then
Copy link
Contributor

Choose a reason for hiding this comment

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

kubefed is version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor Author

@madhusudancs madhusudancs left a comment

Choose a reason for hiding this comment

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

@perotinus PTAL.

@devin-donnelly
Copy link
Contributor

devin-donnelly commented Mar 21, 2017

Docs LGTM. @perotinus , waiting for your Tech LGTM before merging.

Never mind, I see it.

@uiansol
Copy link

uiansol commented Apr 3, 2017

Is there a doc proposal to this feature where I can study it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants