Skip to content
This repository has been archived by the owner on May 6, 2020. It is now read-only.

Update to latest kubernetes version #58

Merged
merged 4 commits into from
Sep 9, 2019
Merged

Update to latest kubernetes version #58

merged 4 commits into from
Sep 9, 2019

Conversation

surajssd
Copy link
Contributor

@surajssd surajssd commented Aug 30, 2019 •
edited by pothos
Loading

Note: Before merging, make sure that the bootkube PR is merged and then use the resulting merge commit ID here in github.com/kinvolk/terraform-render-bootkube//?ref=.

@surajssd surajssd force-pushed the surajssd/update-to-latest-k8s branch 2 times, most recently from 96e2707 to 68e39de Compare August 30, 2019 14:21
@surajssd
Copy link
Contributor Author

surajssd commented Aug 30, 2019 •
edited
Loading

This is working for Packet, AWS, KVM Libvirt !

Testing in progress Google Cloud and Azure!

@surajssd surajssd force-pushed the surajssd/update-to-latest-k8s branch from 68e39de to c84e5eb Compare September 4, 2019 07:32
@pothos pothos mentioned this pull request Sep 4, 2019
Merged
@surajssd surajssd force-pushed the surajssd/update-to-latest-k8s branch from c84e5eb to c043dac Compare September 4, 2019 13:15
@surajssd surajssd changed the title [WIP] Update to latest kubernetes version Update to latest kubernetes version Sep 4, 2019
@surajssd surajssd force-pushed the surajssd/update-to-latest-k8s branch from c043dac to 5b74be4 Compare September 4, 2019 13:18
@surajssd surajssd mentioned this pull request Sep 4, 2019
Merged
@pothos
Copy link
Contributor

pothos commented Sep 4, 2019

@surajssd Please rebase and then change the ci/…/ files to reflect the new versions.

@surajssd surajssd force-pushed the surajssd/update-to-latest-k8s branch from 5b74be4 to 64274dc Compare September 4, 2019 13:32
@pothos
Copy link
Contributor

pothos commented Sep 4, 2019

Why is there a new option to set the cgroup driver while at the same time it is set by autodetecting the docker default (/etc/kubernetes/configure-kubelet-cgroup-driver writes to /etc/kubernetes/kubelet.config)? I think this new option has to be removed again. Did you add it as workaround for the bug in fixed in #62?

@pothos pothos force-pushed the surajssd/update-to-latest-k8s branch from 64274dc to 9a7ca81 Compare September 4, 2019 15:06
@pothos
Copy link
Contributor

pothos commented Sep 4, 2019 •
edited
Loading

Fixed a merge bug where an old version was chosen for the libvirt provider.

rata
rata reviewed Sep 4, 2019
View reviewed changes
Copy link
Contributor

@rata rata left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot for the patch!

I don't follow the reasoning on why so many changes are needed. Can you please explain?

I guess some are from following typhoon? And the rest is really needed for the upgrade or are changes that you made along the way but not related to the upgrade? I think it will be very useful for reviewing to clarify why the changes are done (either on the PR description, or different commits with a commit message that explains the reasonisn, etc.).

IMHO, if possible, let's do the changes to upgrade kubernetes in one PR and the other changes, with their reasoning, etc. can be explained in another PR if those are not related to the upgrade . If these changes come from typhoon or something and makes sense to do them in the upgrade, fine, but in that case it would be great if there is any way (links to typhoon PRs?) to understand the changes done here.

Thanks again!
Rodrigo

aws/flatcar-linux/kubernetes/cl/controller.yaml.tmpl Show resolved Hide resolved
aws/flatcar-linux/kubernetes/cl/controller.yaml.tmpl Outdated Show resolved Hide resolved
aws/flatcar-linux/kubernetes/controllers.tf Show resolved Hide resolved
aws/flatcar-linux/kubernetes/controllers.tf Outdated Show resolved Hide resolved
aws/flatcar-linux/kubernetes/security.tf Show resolved Hide resolved
aws/flatcar-linux/kubernetes/security.tf Show resolved Hide resolved
azure/flatcar-linux/kubernetes/bootkube.tf Show resolved Hide resolved
digital-ocean/container-linux/kubernetes/bootkube.tf Show resolved Hide resolved
@dongsupark
Copy link
Contributor

I've just tested this PR on Packet with Flatcar stable. It worked fine.

@pothos
Copy link
Contributor

pothos commented Sep 4, 2019

FYI from the CI:

Getting source "github.com/kinvolk/terraform-render-bootkube?ref=f5e41c83ec01e00187cb5c50cb7e9ec8275bd43e"
Error downloading modules: Error loading modules: error downloading 'https://github.com/kinvolk/terraform-render-bootkube.git?ref=f5e41c83ec01e00187cb5c50cb7e9ec8275bd43e': /usr/bin/git exited with 128: fatal: reference is not a tree: f5e41c83ec01e00187cb5c50cb7e9ec8275bd43e

@surajssd surajssd force-pushed the surajssd/update-to-latest-k8s branch from ae65f63 to 697754e Compare September 5, 2019 07:11
@surajssd
Copy link
Contributor Author

surajssd commented Sep 5, 2019

Why is there a new option to set the cgroup driver while at the same time it is set by autodetecting the docker default (/etc/kubernetes/configure-kubelet-cgroup-driver writes to /etc/kubernetes/kubelet.config)? I think this new option has to be removed again. Did you add it as workaround for the bug in fixed in #62?

@pothos I didn't notice your PR. The changes added in this PR are ported from upstream, do you suggest removal of that explicit checks? Removed those changes.

@pothos
Copy link
Contributor

pothos commented Sep 5, 2019

Maybe this PR should be called import upstream changes. Good if it helps us to sync better in the future. If there are formatting changes from squashed upstream changes, it's nice to have a separate commit but it's also more work to do so I wouldn't require it and rather have the process more often to reduce the change size.

@surajssd surajssd force-pushed the surajssd/update-to-latest-k8s branch from 697754e to 5358eed Compare September 5, 2019 13:07
@surajssd
Copy link
Contributor Author

surajssd commented Sep 5, 2019

Upstream has moved to 0.12.* of terraform and just rebasing isn't easy on that. I can call it import upstream no problem with it.

@surajssd surajssd force-pushed the surajssd/update-to-latest-k8s branch from 548e751 to d25dd3d Compare September 5, 2019 14:02
@rata
Copy link
Contributor

rata commented Sep 6, 2019

@surajssd Ohh, I understand now. This PR is not only about upgrading kubernetes to latest, but also trying to backport changes from typhoon that is using another terraform version. Sorry I didn't notice before, now I see why there are so many changes here.

For the next time, I think we can either just update kubernetes (no sync with typhoon) or be very clear on which changes have been backported and probably have a 1:1 mapping (1 commit here for each commit upstream). That way, the backported commit can reference the original commit and is more easy to review.

Will try to review with this new scenario in mind now. Sorry for the noise :)

@surajssd
Copy link
Contributor Author

surajssd commented Sep 6, 2019

Yep once we have moved to 0.12 we dont need to worry about where the changes are coming from and why!

rata
rata approved these changes Sep 6, 2019
View reviewed changes
Copy link
Contributor

@rata rata left a comment

Choose a reason for hiding this comment

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

I've continued to look at this and tested this in Packet (with flatcar stable) and this LGTM!

@surajssd
Copy link
Contributor Author

surajssd commented Sep 6, 2019

I will wait for one more approval and then we are good to go I think. Thanks @rata @dongsupark @pothos for testing.

@rata
Copy link
Contributor

rata commented Sep 6, 2019

@surajssd thank you!

@pothos Not sure if your comment was an approval and forgot to approve, or just a comment :)

@pothos
Copy link
Contributor

pothos commented Sep 6, 2019 •
edited
Loading

Should 7e237ffa21fd85f76ddf2a215073aa7cd6ef2476 (the latest master commit) be used for bootkube?
Asking because I don't know if the referenced commit comes from the PR branch and if that was rebased on master before merging.

After that: Please merge ;) I've tested libvirt again.

@rata
Copy link
Contributor

rata commented Sep 6, 2019

Should 7e237ffa21fd85f76ddf2a215073aa7cd6ef2476 be used for bootkube?

Yes, exactly :)

@kosyfrances
Copy link
Contributor

Testing in progress Google Cloud and Azure!

Was this done 🤔
cc @surajssd

@rata
Copy link
Contributor

rata commented Sep 9, 2019

@pothos if the bootkube commit is updated, can we merge this? :)

@surajssd surajssd force-pushed the surajssd/update-to-latest-k8s branch from d25dd3d to 38f261e Compare September 9, 2019 13:26
@surajssd
Update Lokomotive to Kubernetes 1.15
d209acf 
This commit pulls in code from upstream Typhoon with terrafrom 0.11.*
supported.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
@surajssd
CI: Update Lokomotive CI provider version
03d7b02 
Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
@surajssd
Cgroup Driver: Remove manual selection
5ff9bff 
Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
@surajssd
Update provider versions
cedb1be 
Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
@surajssd surajssd force-pushed the surajssd/update-to-latest-k8s branch from 38f261e to cedb1be Compare September 9, 2019 13:39
@surajssd
Copy link
Contributor Author

surajssd commented Sep 9, 2019 •
edited
Loading

@rata @pothos rebased and updated the PR also the tests pass! Can we merge this now?

@surajssd surajssd merged commit c31d850 into master Sep 9, 2019
@surajssd surajssd deleted the surajssd/update-to-latest-k8s branch September 9, 2019 14:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dongsupark dongsupark dongsupark left review comments

@rata rata rata approved these changes

@pothos pothos pothos approved these changes

@schu schu Awaiting requested review from schu

@invidian invidian Awaiting requested review from invidian

@johananl johananl Awaiting requested review from johananl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@surajssd @pothos @dongsupark @rata @kosyfrances