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
kubeadm system container #96
Conversation
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.
Good stuff, but a few updates are needed.
kubeadm/Dockerfile
Outdated
ENV container=docker | ||
|
||
ENV NAME=kubeadm VERSION=0 RELEASE=0 ARCH=x86_64 | ||
LABEL BZComponent="$NAME" \ |
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.
Labels should be lower case.
kubeadm/Dockerfile
Outdated
Architecture="$ARCH" \ | ||
atomic.type='system' | ||
|
||
RUN dnf install -y docker iproute kubernetes-kubeadm kubernetes-node kubernetes-client containernetworking-cni ethtool ebtables && dnf clean all |
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.
--setopt=tsflags=nodocs
should be used to help keep the size down.
kubeadm/Dockerfile
Outdated
FROM registry.fedoraproject.org/fedora:rawhide | ||
MAINTAINER "Jason Brooks" <jbrooks@redhat.com> | ||
|
||
ENV container=docker |
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.
I don't see a reference to this used in the system container itself. It may be worth putting a comment above it explaining what it's used for.
kubeadm/launch.sh
Outdated
export KUBELET_KUBECONFIG_ARGS="--kubeconfig=/etc/kubernetes/kubelet.conf --require-kubeconfig=true" | ||
export KUBELET_SYSTEM_PODS_ARGS='--pod-manifest-path=/etc/kubernetes/manifests --allow-privileged=true --cgroup-driver=systemd --cgroups-per-qos=false --enforce-node-allocatable=' | ||
export KUBELET_NETWORK_ARGS="--network-plugin=cni --cni-conf-dir=/etc/cni/net.d --cni-bin-dir=/usr/libexec/cni" | ||
export KUBELET_DNS_ARGS="--cluster-dns=10.96.0.10 --cluster-domain=cluster.local" |
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.
should this value be hardcoded?
038b680
to
59e8819
Compare
get env values from config file if there's a crio drop-in, use it add back temp_kubelet_args place kubelet vars in an editable location, look for crio env vars if available fix dockerfile tweaks
852da79
to
8aff4d5
Compare
|
kubeadm/config.json.template
Outdated
], | ||
"devices": null, | ||
"apparmorProfile": "", | ||
"selinuxProcessLabel": "" |
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 line doesn't look valid.
] | ||
}, | ||
{ | ||
"type": "bind", |
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 checking, is this mount used?
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.
I'm not sure if it's strictly needed. The service part of this container is the kubelet, and I based the container on the kubelet system container. That one includes this mount.
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.
I won't block on it, but if it isn't needed it would be nice to remove it later.
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.
I'll do some tests
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.
Looks much better. A few questions and one thing that needs to be fixed.
I think it's really the extra coma:
|
fabc528
to
7a4e0e5
Compare
⚡ Test exempted: merge already tested. |
Closes: #96 Approved by: ashcrow
A system container for kubeadm, as blogged about at http://www.projectatomic.io/blog/2017/05/testing-system-containerized-kubeadm/, but with fedora containers.