-
Notifications
You must be signed in to change notification settings - Fork 563
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
base for image stamping #25
Conversation
Thoughts on purging the wardroom name in favor of something more descriptive like 'kubernetes-image', 'kubeadm-image', or 'cluster-api-provider-aws-image'? |
images/README.md
Outdated
| Variable | Default | Description | | ||
|----------|---------|-------------| | ||
| build_version | unset | A unique build version for the image | | ||
| kubernetes_version | 1.9.5-00 | Kubernetes Version to install | |
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.
We should probably default to the latest available stable version here, same with kubernetes_cni_version.
images/README.md
Outdated
| kubernetes_version | 1.9.5-00 | Kubernetes Version to install | | ||
| kubernetes_cni_version | 0.6.0-00 | CNI Version to install | | ||
|
||
For exmaple, to build all images for use with Kubernetes 1.8.9 for build version 1: |
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.
comment doesn't align with the snippet below.
images/README.md
Outdated
|
||
- A Google Cloud account | ||
- The gcloud CLI installed and configured | ||
- A precreated service account json file |
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.
We should remove any references to GCP image building here.
images/ansible/playbook.yml
Outdated
- name: install python | ||
raw: bash -c "test -e /usr/bin/python || (apt-get -qqy update && apt-get install -qqy python python-pip)" | ||
register: output | ||
changed_when: output.stdout != "" |
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 a bit of an ugly hack that we can most likely avoid by using a shell provisioner script in packer instead.
@@ -0,0 +1,9 @@ | |||
--- | |||
kubernetes_version: "{{ kubernetes.version if kubernetes_version_defined else '1.9.3-00' }}" |
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.
Kill all the backwards compat for kubernetes.*
variables here and switch to simple defaults.
Default for versions should probably be an empty string or undefined, and the role should default to latest available release version if not specified.
At some point we may want to consider a way to handle alpha/beta/rc releases as well, but that isn't a priority for the MVP.
docker_image: | ||
name: "{{ item }}" | ||
with_items: kubernetes_cached_images | ||
when: kubernetes_enable_cached_images | bool |
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.
Lets kill the cached image handling for now, especially since it is docker specific.
RestartSec=10 | ||
|
||
[Install] | ||
WantedBy=multi-user.target |
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.
We shouldn't need this systemd unit file, it should be managed by the kubelet/kubeadm packages.
Environment="KUBELET_NETWORK_ARGS=--network-plugin=cni --cni-conf-dir=/etc/cni/net.d --cni-bin-dir=/opt/cni/bin" | ||
Environment="KUBELET_DNS_ARGS=--cluster-dns=10.96.0.10 --cluster-domain=cluster.local" | ||
ExecStart= | ||
ExecStart=/usr/bin/kubelet $KUBELET_KUBECONFIG_ARGS $KUBELET_SYSTEM_PODS_ARGS $KUBELET_NETWORK_ARGS $KUBELET_DNS_ARGS $KUBELET_EXTRA_ARGS |
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.
For kubernetes v1.11+, we should be setting values in /var/lib/kubelet/kubeadm-flags.env
insetad of overriding the unit files here.
kubernetes_apt_key_url_defined: "{{ kubernetes is defined and 'apt_key_url' in kubernetes }}" | ||
kubernetes_apt_repo_string_defined: "{{ kubernetes is defined and 'apt_repo_string' in kubernetes }}" | ||
kubernetes_yum_baseurl_defined: "{{ kubernetes is defined and 'yum_baseurl' in kubernetes }}" | ||
kubernetes_yum_gpgkey_defined: "{{ kubernetes is defined and 'yum_gpgkey' in kubernetes }}" |
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 all backward compat crud.
Nice, I think that would work for the general user use case, but we will still probably want some tooling for the community AMI building. The wardroom tool for example will sync to all available regions in the client config and ensure the images are public. |
@@ -0,0 +1,3 @@ | |||
--- | |||
kubernetes_version: "" | |||
kubernetes_cni_version: "" |
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.
Does this behave correctly with the defaults here? It appears this may fail, since we are passing the value into the kube_platform_version filter.
images/README.md
Outdated
} | ||
] | ||
} | ||
``` |
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.
Have you been able to test using these limited permissions? I'm wondering if the use of ami_groups
or ami_regions
adds additional requirements.
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've updated the permissions to match the official packer suggested ones. Which include permissions to modify images attributes, needed for ami_groups to function. The ami_regions
should be good too.
images/packer/packer.json
Outdated
"ssh_username": "centos", | ||
"tags": { | ||
"build_version": "{{user `build_version`}}", | ||
"source_ami": "{{user `centos_7_4_ami`}}", |
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 wondering if we can use source_ami_filter
here instead to be able to automatically choose the newest base image rather than having to hard code it into a variable file like we are currently doing.
Thoughts?
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.
Yeah. I was actually going to change it to use the official versions. The reason I left it untouched is that with the current behavior it's easier for folks to provide their own base image, which might contain different drivers/tweaks/security tools.
@detiber Fixed / replied to the other comments. I think this is ready to be merged in. 😊 |
Do these files need the licensing stanza at the top? |
All done! Thanks @chuckha |
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.
Lets get the licensing wrapped up and these commits squashed and get this merged :)
@@ -1,3 +1,17 @@ | |||
# Copyright 2018 Heptio Inc. |
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.
s/Heptio/The Kubernetes Authors/
We should probably have this on all the Ansible yaml files as well.
Signed-off-by: Vince Prignano <vince@vincepri.com>
@detiber License notices added, squashed commits. |
/ok-to-test |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: detiber, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…same-ami Make all examples use ami-060f14ef82deddfc6
This is an initial port from wardroom adding support for containerd in Ubuntu (RHEL to follow).
Fixes #12