feat: revive CoreOS support #892
Conversation
alexeldeib
changed the title
alexeldeib
changed the title
|
I got rpc-statd to work on the master nodes by adding a I need a deeper understanding of flexvolume implementation/kubernetes storage and dependencies, probably. The cluster looks pretty okay otherwise. Needs a LOT of testing. |
|
Also, anything customized like the SGX drivers or GPU drivers will almost certainly not work with coreos as-is. |
| ExecStart=/usr/bin/dockerd -H fd:// --storage-driver=overlay2 --bip={{WrapAsParameter "dockerBridgeCidr"}} | ||
| {{end}} |
There was a problem hiding this comment.
nit: let's keep the existing whitespace/indentation usage pattern
There was a problem hiding this comment.
to clarify, anything that's not whitespace sensitive should be fully left-justified?
There was a problem hiding this comment.
I wasn't sure if the stuff inside content should be indented with the text or misaligned to make it more pronounced
There was a problem hiding this comment.
Well, you're correct that the template layer tokens that open and close block/lexical scope (e.g., {{) aren't whitespace-sensitive, but for readability/maintainability we like to align them.
E.g.,
{{if foo}}
bar baz...
{{end}}
So in this case let's add 4 spaces to the {{end}} on L150 so it's left-aligned w/ the {{if .IsCoreOS}}.
|
Generally lgtm, let's get rid of those Thanks for adding this! |
|
@alexeldeib I just made your life more difficult by merging this: That said, I think this PR is mergeable, so feel free to get rid of the |
|
@jackfrancis I don't think flexvolumes work, and the default model includes blobfuse and keyvault flexvolumes. I was hoping to make those work but it's turning out to be a bit tricky. Would you prefer I write tests for the basic functionality and disable the flexvolumes (+ possibly validation for addons/customization which will not work with coreos) or wait to merge this until I get at least flexvols working? I'll take a look at the merge. |
|
I haven't looked deeply at the e2e tests yet, but I would at least like to manually validate some normal scenarios, e.g. deploy nginx-ingress and try to hit some running pods to make sure things work as expected. Looks like e2e supports something like that. I'm not well versed in PVs/kube storage but if flexvols don't work there could be some wider storage issues (although, I didn't see any issues loading the other volume plugins and e.g. mounting secrets). |
|
@alexeldeib Because CoreOS is still definitely a feature "edge case", we definitely don't have to require functional parity w/ Ubuntu before merging. Let's make sure the docs aren't obviously misleading in places if it makes sense to suggests (CoreOS support coming soon! or something), and yeah, make it so that we don't deliver addon specs that just result in failed pods at cluster creation time. In terms of E2E tests, I'd like to throw the whole kitchen sink at a CoreOS-backed cluster and see what fails. Again, it's an experimental scenario, let's merge it when it's minimally functional, and then iterate towards functional parity. |
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov Report
@@ Coverage Diff @@
## master #892 +/- ##
==========================================
- Coverage 74.51% 74.51% -0.01%
==========================================
Files 131 131
Lines 17926 17963 +37
==========================================
+ Hits 13358 13385 +27
- Misses 3824 3829 +5
- Partials 744 749 +5 |
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
alexeldeib
changed the title
|
Still need to update model validation to prevent deploying busted pods and update tests on CSE script to reflect coreos. e2e looking surprisingly good! |
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@jackfrancis @CecileRobertMichon think I managed the rebase, also added a simple validation test on addons. Hopefully this is good to merge! |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeldeib, jackfrancis 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 |
Reason for Change:
Fixes #584
Issue Fixed:
Requirements:
Notes:
I took the absolute dead simplest approach I could to get this working. Right now the masters and agents successfully form a cluster. Seems like there are some issues with rpc-statd and possibly volume mounts. Got some running pods though!