-
Notifications
You must be signed in to change notification settings - Fork 7
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
Move to Kubernetes 1.11 #77
Conversation
54bfbb1
to
10bd01d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks Daniel!
10bd01d
to
f52daee
Compare
f52daee
to
6ff55eb
Compare
f4db4e7
to
6a3fadc
Compare
Do not silently ignore errors
5d53b2d
to
9bc7ffe
Compare
- Accept configuration file in nodeadm join - Use kubeadm config to override hostname - Do not write nodeadm kubelet systemd drop-in; no longer necessary, as values are fetched by kubeadm. See https://kubernetes.io/docs/setup/independent/kubelet-integration/#propagating-cluster-level-configuration-to-each-kubelet - Remove unused Kubelet and Networking fields from JoinConfiguration - Remove default IP and routerID for VIPConfiguration
CoreDNS is the default cluster DNS provider as of kubeadm 1.11
kubeadm 1.11 expects the arch-agnostic image. See kubernetes/kubernetes@0745b25
e20c719
to
c0d8d88
Compare
Co-Authored-By: dlipovetsky <dlipovetsky@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Daniel, thanks a lot for all the changes, had a few questions/comments ptal, thanks!
} | ||
if netConfig.DNSDomain == "" { | ||
netConfig.DNSDomain = constants.DefaultDNSDomain | ||
if p.ExistsP("api.bindPort") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just adding a comment as per our discussion during code walk through. There is a tradeoff between knowing the type or using master configuration as just a blob of map[string]{}
. Not knowing the type reemphasizes (and rightly so) the fact that nodeadm should just act as pass through of kubeadm's master configuration. However there might be cases like these where nodeadm would still have to be aware of partial if not entire schema of upstream type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only case, and it's due to nodeadm configuring keepalived. I'd like to remove even this case in the future, if possible.
func logAllWithPrefix(prefix string, r io.Reader) { | ||
s := bufio.NewScanner(r) | ||
for s.Scan() { | ||
log.Infof("%v: %v", prefix, s.Text()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Daniel, just to understand, this means that even with the piped output the verbosity set at nodeadm
will be honored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes 😄
If nodeadm is configured to omit Info-level messages, kubeadm output won't be seen.
DefaultPodNetwork = "10.244.0.0/16" | ||
DefaultDNSIP = "10.96.0.10" | ||
DefaultServiceSubnet = "10.96.0.0/12" | ||
DefaultDNSDomain = "cluster.local" | ||
DefaultRouterID = 42 | ||
KubeadmConfig = "/tmp/kubeadm.yaml" | ||
KubeDNSVersion = "1.14.8" | ||
CoreDNSVersion = "1.1.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Daniel, sorry forgot to ask during code walk through, for upgrade scenarios do we need an explicit kubectl delete
for kubedns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but that will be done in cctl.
Fixes #76