Skip to content
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

WIP Add fcos flag which transpiles ignition files from spec2 to spec3 #2146

Closed
wants to merge 2 commits into from

Conversation

vrutkovs
Copy link
Member

@vrutkovs vrutkovs commented Aug 2, 2019

This PR would add a new --fcos flag to installer, which would quickly patch ignition files to be compatible with spec3

TODO:

  • Convert configs in IPI mode
  • Make sure default FCOS bootstrap image boots with this config
  • Fix unit tests

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 2, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vrutkovs
To complete the pull request process, please assign wking
You can assign the PR to them by writing /assign @wking in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 2, 2019
@abhinavdahiya
Copy link
Contributor

It's fine for testing, but we wouldn't be able to accept any new flags like fcos.

@LorbusChris
Copy link
Member

awesome! I just had a first quick glance.

Can we call the flag spec3 or something not bound to one specific distro? I imagine v3 will be the default in the future, when RHCOS moves over to it.

@LorbusChris
Copy link
Member

LorbusChris commented Aug 2, 2019

@abhinavdahiya in MCO we'll put spec3 changes in the master-4.3 branch for now. General directive is not to diverge too much with okd, and I think this way we can make sure to re-merge.

@vrutkovs
Copy link
Member Author

vrutkovs commented Aug 2, 2019

It's fine for testing, but we wouldn't be able to accept any new flags like fcos.

Perhaps this should be an install-config.yaml option instead? It would make IPI mode support easier

@wking
Copy link
Member

wking commented Aug 2, 2019

Perhaps this should be an install-config.yaml option instead? It would make IPI mode support easier

The installer is designed to launch a single release image. If other release images are close enough that the installer can launch them too, then great, but I don't think we want a bunch of settings (in any location) to say "slot in the logic for launching this other class of release images". If folks want to test the spec3 format, just create a spec3 branch of the installer and use that. Eventually (when RHCOS moves to spec3), the mainline installer will also move to spec3 (e.g. with a revived #1468). But I think life gets unnecessarily complicated if we try to track both a given RHCOS series and a given FCOS series in the same installer branch, when we can get the same result by branching the installer (or just waiting on FCOS-backed clusters until RHCOS catches up).

@LorbusChris
Copy link
Member

How will installer go without some sort of dual spec2/spec3 support before we are able to update bootimages (openshift/os#381)? Or are we waiting for that to be possible before moving to spec3 with RHCOS?

@cgwalters
Copy link
Member

If folks want to test the spec3 format, just create a spec3 branch of the installer and use that.

Hmm. Maybe, though we may need more conditional code to support FCOS than just spec 2 vs 3.

Anyways, I'm aware the installer team didn't get CC'd on the plan - we can reconsider things here, but we need a cohesive architecture to pull off FCOS-for-OKD.

Maybe instead of a git branch, it's a build-time option? That's what we want ultimately I think, is for the openshift-installer one gets from the OKD buildsystem to output one that works with OKD.

@vrutkovs
Copy link
Member Author

vrutkovs commented Aug 2, 2019

Eventually (when RHCOS moves to spec3), the mainline installer will also move to spec3

I don't mind making a fork, but KNI team had kept its fork for ~3 month and it was pretty painful to merge it back. The situation where community has to use a fork until RHCOS supports spec3 doesn't look good to me

@wking
Copy link
Member

wking commented Aug 2, 2019

Maybe, though we may need more conditional code to support FCOS than just spec 2 vs 3.

Your doc mentions dropping the bootstrap machine's kubelet dependency as a possible step. If that turns out to be feasible, I don't see an issue with doing that in our master branch; no need for a knob to toggle it. Are there other changes you expect where FCOS would have a long-running divergence from RHCOS?

I don't mind making a fork, but KNI team had kept its fork for ~3 month...

What's blocking RHCOS moving to spec3? I'd guess the issue is how we manage upgrades from clusters with spec2 bootimages, and bootimage changes at all are still unclear (openshift/os#381). Would the MCO serve spec2 and spec3 content depending on how it was asked during the transition? But from the installer side, serving spec3 content to the stock RHCOS bootimages seems pretty straightforward (e.g. #1468), so I don't see us holding this up.

The only changes you'd need in a spec3 fork are the pkg/asset/asset.go bit about convertSpec2ToSpec3. That doesn't seem like it's going to be an invasive change with complicated rebasing if you do decide to maintain a fork.

What's the pressure for FCOS support? If maintaining a spec3 fork for too long seems arduous, how much does it hurt to just wait until spec3 RHCOS is here (or at least closer)?

@cgwalters
Copy link
Member

The MCO side of bootimage handling and transitioning is in openshift/machine-config-operator#492

@DanyC97
Copy link
Contributor

DanyC97 commented Aug 9, 2019

how much does it hurt to just wait until spec3 RHCOS is here (or at least closer)?

has this even been planned for the very near future @wking @cgwalters ?

@openshift-ci-robot
Copy link
Contributor

@vrutkovs: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-openstack 48dc4a3 link /test e2e-openstack
ci/prow/e2e-aws-scaleup-rhel7 48dc4a3 link /test e2e-aws-scaleup-rhel7
ci/prow/e2e-aws-disruptive 48dc4a3 link /test e2e-aws-disruptive

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@abhinavdahiya
Copy link
Contributor

/close

closing without prejudice due to age. Please consider opening an issue or enhancement to discuss and bring consensus on the fix, or if you think this is still important in current state feel free to reopen.

@openshift-ci-robot
Copy link
Contributor

@abhinavdahiya: Closed this PR.

In response to this:

/close

closing without prejudice due to age. Please consider opening an issue or enhancement to discuss and bring consensus on the fix, or if you think this is still important in current state feel free to reopen.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants