search

kinvolk-archives / lokomotive-kubernetes

Lokomotive is a 100% open-source Kubernetes distribution from the folks at Kinvolk
https://kinvolk.io
MIT License
144 stars 20 forks source link

Migrate to Terraform 0.12 #119

Closed mauriciovasquezbernal closed 4 years ago

mauriciovasquezbernal commented 4 years ago

Description

This PR is based and supersedes https://github.com/kinvolk/lokomotive-kubernetes/pull/45. It migrates the different providers and updates the documentation accordingly.

I would need some help here to test the providers I already tested and also to test the ones I wasn't able to test yet.

The commits are organized as follows:

Status

Testing

Given that it is a quite large PR, I'm adding this table to check that all plaforms are tested. Please add your name (so we can blame you later on [not true :rofl: ]) when you test a platform.

Platform Tester 1 Tester 2 Tester 3?
AWS @mauriciovasquezbernal
Azure @mauriciovasquezbernal
Bare Metal
Google-Cloud @mauriciovasquezbernal
KVM-libvirt @ipochi
Packet @mauriciovasquezbernal @johananl

TODO

ipochi commented 4 years ago

tested on kvm-libvirt, works fine

mauriciovasquezbernal commented 4 years ago

@rata I had saved a backup branch with more granular commits, I clean those up and pushed here. I hope this new version is easier to review.

mauriciovasquezbernal commented 4 years ago

@rata I think having a working tree on each commit and having small commits could be complicated. For instance, after running terraform 0.12upgrade it is needed to fix some issues by hand, if I do that in a single commit then it'll be not possible to understand what was changed by the tool and what was fixed manually.

I like the idea of having a working tree on all commits, but I'm don't know if it's worth to do so.

mauriciovasquezbernal commented 4 years ago

I think the overall diff looks good to me. Added some small comments. Thanks for tackling such a big PR! :)

One thing I'm confused is that it seems you created other PRs for fixes included here. The idea is to merge those and then adapt this?

Yes, while working on this I found some problems in Azure, since those are not related to this PR I opened a separated PR for that https://github.com/kinvolk/lokomotive-kubernetes/pull/116. That's now merged and I'll rebase it to include those changes.

On a side note, I still think we can improve the way commits are split, to be easy to review while still having a working tree in all commits (for easy bisect, etc.), but that seems out of scope for this PR and we don't have any rules on this yet.

The problem is that in order to do that I'll have to squash the commits where terraform 0.12upgrade and the fixes for that are done, if that's not a problem I can do that.

I think the following layout could work:

Thoughts @rata, @invidian , @johananl ?

rata commented 4 years ago

I think the overall diff looks good to me. Added some small comments. Thanks for tackling such a big PR! :) One thing I'm confused is that it seems you created other PRs for fixes included here. The idea is to merge those and then adapt this?

Yes, while working on this I found some problems in Azure, since those are not related to this PR I opened a separated PR for that #116. That's now merged and I'll rebase it to include those changes.

Cool! :)

On a side note, I still think we can improve the way commits are split, to be easy to review while still having a working tree in all commits (for easy bisect, etc.), but that seems out of scope for this PR and we don't have any rules on this yet.

The problem is that in order to do that I'll have to squash the commits where terraform 0.12upgrade and the fixes for that are done, if that's not a problem I can do that.

I think the following layout could work:

  • preparatory commits: update provider versions, rename count to worker_count
  • one commit per provider: run terraform 0.12upgrade, fixes problems, point bootkube to right place, update doc syntax
  • use array notation
  • use correct datatypes

Thoughts @rata, @invidian , @johananl ?

That sound good to me, if it doesn't take too much time for you. It is, in a way, what I was trying to say here (haven't realized about the "after" changes, haven't review those probably at that time): https://github.com/kinvolk/lokomotive-kubernetes/pull/119#pullrequestreview-336819228. Sorry I wasn't clear there :-/

But I'll let others decide on this :)

invidian commented 4 years ago

preparatory commits: update provider versions, rename count to worker_count

It can be separated PR to minimize the impact of this PR (it's already large).

one commit per provider: run terraform 0.12upgrade, fixes problems, point bootkube to right place, update doc syntax

IMO all providers at a time is fine. If we decide to commit per provider, we should be consistent around that with further changes. Given that it's unlikely that this commits will be reverted, I think it should be fine to have one commit for all providers.

Also the reasoning etc is the same for all providers, which is another argument for doing that together.

invidian commented 4 years ago

Rebased on latest master and pushed a commit, which changes CI to use Terraform 0.12.

invidian commented 4 years ago

Had to cherry-pick some commits from #112 to resolve version skew between kube-apiserver and kubelets.

pothos commented 4 years ago

I cherry-picked one more commit from https://github.com/kinvolk/lokomotive-kubernetes/pull/112 since it seems to be related to the bootkube failure. I adapted it to apply to baremetal as well.

invidian commented 4 years ago

Let's merge #112 first, what do you think?

pothos commented 4 years ago

Yes, so that we can drop the commits here again instead of trying to keep them in sync.

invidian commented 4 years ago

Given that it took us so much time to get there, I'll merge it now.