Update the kubelet with the ability to register itself with the master.#2435
Update the kubelet with the ability to register itself with the master.#2435brendandburns wants to merge 1 commit intokubernetes:masterfrom
Conversation
96040d8 to
ef9a057
Compare
There was a problem hiding this comment.
I like a single flag that tells the path to a kubernetes_auth file better than calling BindClientConfigFlags because it allows clients to be portable across clusters with different auth setups, and facilitates rotating credentials. Consider replacing client.BindClientConfigFlags with:
import "github.com/GoogleCloudPlatform/kubernetes/pkg/clientauth"
...
vars(
authPath = flag.String("auth_path", "", "Path to .kubernetes_auth file, specifying how to authenticate to API server.")
apiServer = flag.String("api_server", "Kubernetes API server (ip:port)")
)
...
func getApiserverClient() (*client.Client, error) {
authInfo, err := clientauth.LoadFromFile(*authPath)
if err != nil {
return nil, err
}
clientConfig, err := authInfo.MergeWithConfig(client.Config{})
if err != nil {
return nil, err
}
clientConfig.Host = apiServer
if c, err := client.New(&clientConfig); err != nil {
return nil, err
} else {
return c, nil
}
}
...
apiClient, err := getApiserverClient()There was a problem hiding this comment.
#2437 is intended to make this easier and simpler to do.
There was a problem hiding this comment.
Suggest removing this line and waiting for a different PR to discuss my and Clayton's comments above. Should still be quite possible to make a client.
|
why is it better to have kubelets send the join than to have the master or a minion controller add kubelets? in the case where self-registration of kubelets is enabled, by what process are they de-registered? |
|
What mode of authentication do you plan to use with the kubelet's api client? |
There was a problem hiding this comment.
Couldn't these be part of the same flag? Seems like they always go together (don't need both)
|
I'd like to suggest an alternate auth model for this:
Note that this flow is compatible with the thinking in #2303. |
|
As it stands, there is nothing in this PR that relates to auth. The kubelet uses the apiserver API just like anything else. I would like to suggest that we divide the problem into two problems:
I would really rather get this PR in, rather than block on the auth model. (I'll fix the bugs in the code noted by @erictune shortly) |
I feel like we never all got on the same page about this being our model. Should kubelets self-register? Or should our node-manager thingy register them, or an admin, all of the above, or none of the above? I don't think we have a coherent vision for this yet. |
|
|
100% all of the above. I think that some people will want to lock it down completely, some people will want to self-manage their nodes (especially at first) so they can add specific instances with SSD or whatever. But again, this policy seems to be a property of the apiserver which is the gate keeper for deciding if they join or not, not the kubelet which simply discover's and asks to join. We should enable the expression of these policies, but not in this PR. As it stands right now, I can dynamically add minions via the API server. This PR simply streamlines that process for the kubelet. |
|
What problem are you trying to solve? Submitting PRs with no comments, no description, and no issue reference is not very helpful. |
ef9a057 to
c8e4cfc
Compare
|
I added some documentation (clustering.md) about the three different models I think that we should support. Let's get some agreement there. --brendan |
c8e4cfc to
d7988ab
Compare
|
@jbeda I'm happy to add your suggestions there, or we can merge and iterate forward. |
There was a problem hiding this comment.
Thanks for adding this doc. It explains the different options well. Would you mind either:
- moving it to docs/design/clustering.md until we have complete support for all the options; OR
- calling out which options are actually supported, and which aren't, as of the time of this PR being merged.
Ideally, all descriptions in the top level docs folder would be current rather than aspirational.
There was a problem hiding this comment.
I'm willing to wait for another PR for this comment.
|
I'm now happy with the overall direction that @brendanburns and @jbeda are suggesting. I do still have some comments on the code |
There was a problem hiding this comment.
You said that you wanted to tackle registration in this PR, and authentication in another one. That's fine by me. In that case, how about deleting line 80 above, and line 89. You can still test out this code --auth_path flag and the --api_servers flag. And then we can revisit those lines in a later PR.
There was a problem hiding this comment.
Yes, could we please not Fatalf here? This will be a problem in production. It would be much better to produce an error, sleep, and retry until it succeeds, while surfacing information for debugging/monitoring.
This is a case where it would be great to have the equivalent of DFATAL - die if a test, and error if production.
|
Can one of the admins verify this patch? |
|
Where does clustering.md live now? I couldn't find it in the repo. |
|
@davidopp clustering.md hasn't been added yet, since this PR hasn't been merged. |
|
I'll work with @brendandburns this week to move this forward. This is a key part of our "easy clustering" story. |
|
#2793 will go nicely with this PR. |
This is related to kubernetes#2303 and steals from kubernetes#2435.
|
@brendandburns Are you planning to pick this up again? |
|
ping @brendanburns to close PR or state plan for it |
I think that might be a bad ping. @brendanburns vs @brendandburns |
|
Discussed w/ @brendandburns. We're planning to do this, but not using this exact approach. Closing. |
This is related to kubernetes#2303 and steals from kubernetes#2435.
…silimits 4.16: OCPBUGS-60872: UPSTREAM: 127757: scheduler: Improve CSILimits plugin accuracy by using VolumeAttachments
See #2303