Skip to content
This repository has been archived by the owner on Jul 14, 2019. It is now read-only.

[CMS-272] Use kubeadm.k8s.io/v1alpha1 and cleanup before installation. #43

Merged
merged 4 commits into from
Aug 7, 2018

Conversation

davidewatson
Copy link
Contributor

No description provided.

@davidewatson davidewatson changed the title [CMS-272] Use kubeadm.k8s.io/v1alpha1 since v1alpha2 requires k8s 1.11. [CMS-272] Use kubeadm.k8s.io/v1alpha1 since v1alpha2 requires kubeadm 1.11. Aug 7, 2018
@joejulian
Copy link
Contributor

Do we want to wrap this in some sort of logic to we can upgrade versions?

@alejandroEsc
Copy link
Contributor

alejandroEsc commented Aug 7, 2018

The change is acceptable though i havent had this issue, even with new prestine deployments. Could you verify that the new apiversion works and consistently deploys?

Copy link
Contributor

@oneilcin oneilcin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@alejandroEsc
Copy link
Contributor

@davidewatson i think i got your verification on slack. going to merge.

@joejulian
Copy link
Contributor

joejulian commented Aug 7, 2018

Additionally, v1alpha1 read support exists in v1.11, but will be removed in v1.12:
https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG-1.11.md#new-deprecations
kubernetes/kubernetes#63788

@@ -63,7 +63,7 @@

# Set up kubeadm config file to pass parameters to kubeadm init.
cat << EOF | sudo tee /etc/kubernetes/kubeadm_config.yaml
apiVersion: kubeadm.k8s.io/v1alpha2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment line explaining when we will need to switch to v1alpha2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be backwards compatible? if not yeah big deal. Good point!

@davidewatson
Copy link
Contributor Author

davidewatson commented Aug 7, 2018

@joejulian: I can add the requested logic. We'll need it later. The purpose of this PR is to ensure everyone is on the same page. I was going to submit another temporary PR along the lines of:

          kubeadm reset || true
          apt-get purge kubeadm kubectl kubelet kubernetes-cni kube* -y || true
          rm -f /etc/systemd/system/kubelet.service.d/* || true
          rm -f /etc/systemd/system/kubelet.d/* || true
          rm -rf /var/lib/etcd || true

I think this is even worse than the current PR. :) Is it okay if we defer this change for when we rewrite these scripts? The are many problems (e.g. unpinned and untested docker releases, etc.) with these temporary scripts...

@alejandroEsc
Copy link
Contributor

alejandroEsc commented Aug 7, 2018

@davidewatson I am missing context here. What are you suggestion you do with that kubeadm reset || true? I hope you are NOT suggesting we run that as part of the provisioning script.

@davidewatson
Copy link
Contributor Author

@alejandroEsc: That is correct.

@davidewatson davidewatson changed the title [CMS-272] Use kubeadm.k8s.io/v1alpha1 since v1alpha2 requires kubeadm 1.11. [CMS-272] Use kubeadm.k8s.io/v1alpha1 and cleanup before installation. Aug 7, 2018
@alejandroEsc
Copy link
Contributor

@davidewatson i think that brings forth a lot of assumptions that we should not make. First we assume that machines are Pristine. Making sure that is true or fixing any state issues with existing machines should not be our concern. We should make those assumptions clear and known. As for ourselves and our testing, we should do the cleaning we can and make sure things are pristine before each test cycle. That is on us. Honestly I dont want to support every possible combination of things to look for or clean, or even worry about supporting such code.

@@ -6,6 +6,13 @@
# Run in noninteractive mode to avoid erroring when a config file preexists.
export DEBIAN_FRONTEND=noninteractive

# This a temporary hack for development purposes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please DO NOT INCLUDE THIS even with a TODO. Please create a new master template file and call it _DEV if you wish.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even further, this should be in a separate PR. Note you had to use the word and in your PR subject.

@davidewatson
Copy link
Contributor Author

davidewatson commented Aug 7, 2018

@alejandroEsc, @joejulian: I've removed the hack.

Again, the intent of the PR was to simplify things while over half of our team can't seem to deploy. Let's focus on that issue please.

@joejulian joejulian merged commit d5c0092 into samsung-cnct:master Aug 7, 2018
@davidewatson davidewatson deleted the scripts branch August 7, 2018 20:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants