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.

Sign up for GitHub

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

Jump to bottom

Abstract Ignition operations in a single file #3248

Closed
wants to merge 9 commits into from
Closed

Abstract Ignition operations in a single file #3248

wants to merge 9 commits into from

Conversation

vrutkovs
Copy link
Member

@vrutkovs vrutkovs commented Mar 5, 2020 •
edited
Loading

This PR moves all functions which create or modify Ignition in a single file and abstract Ignition config in IgnConfig.

The change would enable us to replace Ignition v2 implementation with Ignition v3 using build tags (or any other way on compile-time).

This ensures machineconfigs are created with Ignition 2.2.0 and Openstack shim is created using Ignition v2.4.0.

/cc @LorbusChris @cgwalters @openshift/installer

@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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 5, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign smarterclayton
You can assign the PR to them by writing /assign @smarterclayton 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

@vrutkovs
Copy link
Member Author

vrutkovs commented Mar 5, 2020

/cc @smarterclayton @abhinavdahiya

@abhinavdahiya
Copy link
Contributor

this is too big a commit to review. can you break it apart into new functions and then moving existing code to it.

also ignConfig isn't a great name.
adding abstractions for dropin, unit, file etc is a little too much for when the installer should actually just move to v3 in one go and the server (here the igntion on the host) is the one that should understand both.

@vrutkovs
Copy link
Member Author

vrutkovs commented Mar 5, 2020

this is too big a commit to review. can you break it apart into new functions and then moving existing code to it.

Okay, will do

also ignConfig isn't a great name.

It can be addressed later.

adding abstractions for dropin, unit, file etc is a little too much for when the installer should actually just move to v3 in one go and the server (here the igntion on the host) is the one that should understand both.

That can be done in approximately 4.6 time, when RHCOS switches to Ignition v3. OKD installer would like to do that earlier.

@LorbusChris
Copy link
Member

/cc @openshift/openshift-team-mco-maintainers

LorbusChris
LorbusChris reviewed Mar 6, 2020
View reviewed changes
pkg/asset/ignition/ignition_v2.go
// self-signed certificate, we have to provide it a valid custom ca bundle.
// To do so we generate a small ignition config that contains just Security section with the bundle
// and later append it to the main ignition config.
// We can't do it directly in Terraform, because Ignition provider suppors only 2.1 version, but
Copy link
Member

Choose a reason for hiding this comment

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

I opened a PR a month ago to support Spec 2.4 in the provider: hashicorp/terraform-provider-ignition#67

Copy link
Member

Choose a reason for hiding this comment

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

this may not be needed after all. If you don't have to use 2.4 everywhere, I'd say let's not in order to keep changes to a mininum.

@LorbusChris
Copy link
Member

MCO should probably be updated to natively use Ignition Spec 2.4 before this can go in?
/cc @runcom

@LorbusChris
Copy link
Member

openshift/machine-config-operator#1543 adds support for Spec 2.4,
but it needs the RawExtension PR first (openshift/machine-config-operator#996)

@LorbusChris LorbusChris mentioned this pull request Mar 7, 2020
Closed
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2020
@vrutkovs vrutkovs force-pushed the master-ign-abstraction branch 5 times, most recently from 0d3c284 to 3de649f Compare April 6, 2020 08:19
@vrutkovs
Copy link
Member Author

vrutkovs commented Apr 6, 2020

ovirt flaked, workers didn't join on openstack

/retest

@vrutkovs vrutkovs changed the title WIP Abstract Ignition operations in a single file Abstract Ignition operations in a single file Apr 6, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2020
@vrutkovs
Copy link
Member Author

Imagestream import issues
/retest

@vrutkovs
Copy link
Member Author

/retest

@vrutkovs vrutkovs mentioned this pull request May 19, 2020
Merged
@vrutkovs
Copy link
Member Author

/retest

@vrutkovs
Copy link
Member Author

/hold cancel

All issues resolved, PTAL

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 26, 2020
@stbenjam
Copy link
Member

/test e2e-metal-ipi

1 similar comment
@stbenjam
Copy link
Member

/test e2e-metal-ipi

@vrutkovs
Copy link
Member Author

vrutkovs commented Jun 8, 2020

/retest

@LorbusChris
Copy link
Member

@vrutkovs do you mind updating this PR once again? Spec 3 support has now merged into MCO, the migration could happen be done any time now.
I propose we collect all ignition-related functions in one file as proposed in this PR (however we won't be needing the build flag), and then flip the switch to v3 in a follow up PR.

@vrutkovs
Copy link
Member Author

vrutkovs commented Jul 6, 2020

I propose we collect all ignition-related functions in one file as proposed in this PR (however we won't be needing the build flag), and then flip the switch to v3 in a follow up PR

We still need RHCOS to run Ign3 for tests to pass

@vrutkovs
pkg/asset/ignition: create functions which create or modify Ignition
15afe63 
This file would contain functions which create or change Ignition files, importing a version of github.com/coreos/ignition
@vrutkovs
pkg/asset/cluster: use InjectInstallInfo from pkg/asset/ignition
6817ba9
@vrutkovs
pkg/tfvars/openstack: use GenerateIgnitionShim from ignition pkg
1df56ea
@vrutkovs
pkg/asset/ignition/machine: use PointerIgnitionConfig from ignition pkg
7816887
@vrutkovs
pkg/assets/ignition/bootstrap: use functions from ignition pkg
7c81361
@vrutkovs
pkg/asset/machines: use functions in ignition pkg to generate additio…
28ebe9c 
…nal MachineConfigs.

Functions from pkg/asset/ignition/node.go are no longer used, so it is
removed. `runtime.Extension` is used to be compatible with different Ignition versions
@vrutkovs
ignition_v2: use ignition 2.2 for most files and 2.4 for Openstack shim
f6cede3
@vrutkovs
Bump to latest MCO
e89e321
@vrutkovs
go mod vendor
e333587
@LorbusChris
Copy link
Member

/retest

@openshift-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 e333587 link /test e2e-aws-scaleup-rhel7
ci/prow/e2e-ovirt e333587 link /test e2e-ovirt
ci/prow/e2e-openstack e333587 link /test e2e-openstack
ci/prow/e2e-aws e333587 link /test e2e-aws
ci/prow/verify-codegen e333587 link /test verify-codegen

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.

@vrutkovs
Copy link
Member Author

Superceded by #3871

@vrutkovs vrutkovs closed this Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LorbusChris LorbusChris LorbusChris left review comments

@cgwalters cgwalters Awaiting requested review from cgwalters

@abhinavdahiya abhinavdahiya Awaiting requested review from abhinavdahiya

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@runcom runcom Awaiting requested review from runcom

Assignees
No one assigned
Labels
size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vrutkovs @openshift-ci-robot @abhinavdahiya @LorbusChris @Fedosin @stbenjam