kubefed v1.6 update #2899
kubefed v1.6 update #2899
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
madhusudancs
force-pushed
the
kubefed-v16
branch
from
9c9596e
to
cab5ab0
Compare
madhusudancs
force-pushed
the
kubefed-v16
branch
from
cab5ab0
to
6c54d16
Compare
|
This is ready for review now. |
|
@perotinus I can't put you in the @shashidharatd @irfanurrehman please review for technical accuracy too. |
|
cc @kubernetes/sig-docs-maintainers |
|
cc @kubernetes/docs |
ghost
mentioned this pull request
| [`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`. | ||
|
|
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@shashidharatd Oh I see. That's useful information. Thanks!
Documented the PVC configuration along with a description. PTAL.
|
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 |
There was a problem hiding this comment.
Do we need to add a trailing dot here to avoid inconsistencies?
There was a problem hiding this comment.
Sure, if a trailing dot does not affect anything. Does it?
There was a problem hiding this comment.
Trailing dot does not seem to affect CoreDNS configuration. so probably we should use trailing dot consistent with other examples.
There was a problem hiding this comment.
Ok, I will fix it then.
| --host-cluster-context=rivendell \ | ||
| --dns-provider="google-clouddns" \ | ||
| --dns-zone-name="example.com." \ | ||
| --apiserver-arg-overrides="--anonymous-auth=false, --v=4" \ |
There was a problem hiding this comment.
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" \
There was a problem hiding this comment.
Alright, fair enough. Fixed.
| ```shell | ||
| kubefed init fellowship \ | ||
| --host-cluster-context=rivendell \ | ||
| --dns-provider="coredns" \ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, Please leave it as it is now!
|
adding one more comment, which is on a portion not part of the diff here: |
| @@ -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. | |||
There was a problem hiding this comment.
"is a beta feature in Kubernetes 1.6." Please follow the established format.
| --controllermanager-arg-overrides="--controllers=services=false" | ||
| ``` | ||
|
|
||
| ### Configuring DNS provider |
There was a problem hiding this comment.
"Configuring a DNS provider"
|
|
||
| ### Configuring DNS provider | ||
|
|
||
| Federated service controller programs a DNS provider to expose |
There was a problem hiding this comment.
"Federated service controller" -> "The Federated service controller"
|
Once all comments are addressed, we can merge. Overall great work @madhusudancs ! |
There was a problem hiding this comment.
@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. | |||
| --host-cluster-context=rivendell \ | ||
| --dns-provider="google-clouddns" \ | ||
| --dns-zone-name="example.com." \ | ||
| --apiserver-arg-overrides="--anonymous-auth=false, --v=4" \ |
There was a problem hiding this comment.
Alright, fair enough. Fixed.
| --controllermanager-arg-overrides="--controllers=services=false" | ||
| ``` | ||
|
|
||
| ### Configuring DNS provider |
|
|
||
| ### Configuring DNS provider | ||
|
|
||
| Federated service controller programs a DNS provider to expose |
| ```shell | ||
| kubefed init fellowship \ | ||
| --host-cluster-context=rivendell \ | ||
| --dns-provider="coredns" \ |
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
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`. | ||
|
|
There was a problem hiding this comment.
@shashidharatd Oh I see. That's useful information. Thanks!
Documented the PVC configuration along with a description. PTAL.
@devin-donnelly thanks! |
| 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
"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). |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
|
|
||
| `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 |
madhusudancs
force-pushed
the
kubefed-v16
branch
from
b1c5265
to
b4ffa83
Compare
|
Docs LGTM. @perotinus , waiting for your Tech LGTM before merging. Never mind, I see it. |
|
Is there a doc proposal to this feature where I can study it? |
Ref: Issue #2909
This change is