Skip to content

Ignition spec v2/v3 dual support #1703

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
Merged
openshift-merge-robot merged 3 commits into from
Jul 6, 2020
Merged

Ignition spec v2/v3 dual support #1703

openshift-merge-robot merged 3 commits into from
Jul 6, 2020

Conversation

@LorbusChris
Copy link
Contributor

@LorbusChris LorbusChris commented Apr 30, 2020 •
edited
Loading

- What I did

  • Added functionality to parse Ignition spec version <= 2.2, 3.0 and 3.1
  • Added transforming function to the MCS to allow serving different config specs for different Ignition User Agents.

- How to verify it

  • OCP: CI
  • OKD: manual testing

- Description for the changelog
Added Ignition spec v2/v3 dual support

/cc @runcom @yuqi-zhang @ashcrow

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 30, 2020
@LorbusChris
Copy link
Contributor Author

LorbusChris commented May 1, 2020

I'll work on fixing verify next week.
Once this is all green I'd like to use this as base for the next subsequent OKD Beta release.
(there are 3 or 4 more small commits that will need to be rebased on top of this for that)
/cc @vrutkovs

@LorbusChris
Copy link
Contributor Author

LorbusChris commented May 9, 2020

/test e2e-gcp-op

@LorbusChris
Copy link
Contributor Author

LorbusChris commented May 10, 2020

/retest

1 similar comment
@vrutkovs
Copy link
Contributor

vrutkovs commented May 11, 2020

/retest

@vrutkovs
Copy link
Contributor

vrutkovs commented May 13, 2020

Seems we're hitting https://bugzilla.redhat.com/show_bug.cgi?id=1815799 here
/retest

@LorbusChris LorbusChris mentioned this pull request May 13, 2020
Merged
@LorbusChris
Copy link
Contributor Author

LorbusChris commented May 14, 2020

/retest

1 similar comment
@LorbusChris
Copy link
Contributor Author

LorbusChris commented May 14, 2020

/retest

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 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 May 16, 2020
@LorbusChris LorbusChris changed the title [WIP] Ignition spec v2/v3 dual support Ignition spec v2/v3 dual support May 16, 2020
@LorbusChris LorbusChris marked this pull request as ready for review May 16, 2020 07:56
@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 May 16, 2020
@LorbusChris
Copy link
Contributor Author

LorbusChris commented May 16, 2020

/retest

@LorbusChris
Copy link
Contributor Author

LorbusChris commented May 16, 2020

/retest

@openshift openshift deleted a comment from openshift-ci-robot May 18, 2020
@openshift openshift deleted a comment from openshift-ci-robot May 18, 2020
@ashcrow
Copy link
Member

ashcrow commented May 18, 2020

/test e2e-metal-ipi

@ashcrow
Copy link
Member

ashcrow commented May 18, 2020

/test e2e-aws

@LorbusChris
Copy link
Contributor Author

LorbusChris commented May 18, 2020

Looks like this code works in OCP.

The last commit is optional -- the migration of the types used internally to spec v3.1 is not strictly necessary at this time as the MCS only supports a subset of Ignition anyway.

For this to work with OKD on GCP, the gcp-routes script needs to be moved into the MCO first (#1670)

@LorbusChris
Copy link
Contributor Author

LorbusChris commented May 18, 2020

/test e2e-metal-ipi

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 4, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@yuqi-zhang
Copy link
Contributor

yuqi-zhang commented Jul 4, 2020

/hold

whoops there was a merge conflict

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 4, 2020
@LorbusChris
*: Update ignition/v2 to v2.3.0 with config spec v3.1
324142b 
Also update ign-converter for spec v3.1 to v2.2 translation
@LorbusChris
*: Use IgnParseWrapper func to always parse to spec v2.2
02f5cd0 
Changes the IgnParseWrapper function to translate to and output Ignition spec v2.2 config.
Use the function everywhere instead of the Ignition spec v2.2 parser
to allow consuming spec v3.0 and v3.1 config.
@LorbusChris
mcs: Add response transform for Ignition config translation
7262fe6 
Makes the MCS aware of what config specification
it has to serve to the requesting machine.
If required and supported, the response configuration is
translated to a version supported by the requesting node.

Adds a convertIgnition3to2 helper function to the controller's
common helpersfor translating spec v3.1 Ignition config to v2.2 ,
as well as a ConvertRawExtIgnition2to3 function to easily
convert configs from spec v2.2 to 3.1 in the RawExtension format.
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2020
@LorbusChris
Copy link
Contributor Author

LorbusChris commented Jul 5, 2020

rebased. should be good to go now, just needs another lgtm
/hold cancel

@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 Jul 5, 2020
@yuqi-zhang
Copy link
Contributor

yuqi-zhang commented Jul 5, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2020
@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 5, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 5, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 5, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 5, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

yuqi-zhang
yuqi-zhang reviewed Jul 5, 2020
View reviewed changes
Copy link
Contributor

@yuqi-zhang yuqi-zhang left a comment

Choose a reason for hiding this comment

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

/lgtm cancel

@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 5, 2020
@yuqi-zhang
Copy link
Contributor

yuqi-zhang commented Jul 5, 2020 •
edited
Loading

/approve
/lgtm

sorry for the noise

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2020
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 5, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LorbusChris, yuqi-zhang

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

The pull request process is described here

Details 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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 5, 2020
@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 5, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 5, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 5, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 5, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 5, 2020 •
edited
Loading

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

Test name Commit Details Rerun command
ci/prow/e2e-metal-ipi 7262fe6 link /test e2e-metal-ipi
ci/prow/e2e-aws-scaleup-rhel7 7262fe6 link /test e2e-aws-scaleup-rhel7

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.

Details

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.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 5, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit b5371aa into openshift:master Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yuqi-zhang yuqi-zhang yuqi-zhang left review comments

@ashcrow ashcrow Awaiting requested review from ashcrow

@runcom runcom Awaiting requested review from runcom

@vrutkovs vrutkovs Awaiting requested review from vrutkovs

+1 more reviewer

@kikisdeliveryservice kikisdeliveryservice kikisdeliveryservice left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@yuqi-zhang yuqi-zhang

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@LorbusChris @vrutkovs @ashcrow @kikisdeliveryservice @yuqi-zhang @openshift-bot @openshift-ci-robot @runcom @openshift-merge-robot