Bug 1821888: controller: do not error on empty Ignition configs #1631

yuqi-zhang · 2020-04-08T15:36:33Z

With the introduction of rawExtension for Ignition config objects
via #996,
we also now use ignition.Parse directly to process the validity of
ignition configs. This will cause machineconfigs without a Ignition
section to fail, which we don't want.

Also modify some error messages for clarity.

Signed-off-by: Yu Qi Zhang jerzhang@redhat.com

openshift-ci-robot · 2020-04-08T15:36:39Z

@yuqi-zhang: This pull request references Bugzilla bug 1821888, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1821888: controller: do not error on empty Ignition configs

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.

yuqi-zhang · 2020-04-08T15:39:26Z

Right now any machineconfig we apply, during or after bootstrap, without an ignition section fails since the parse errors with a nil ignition config

With the introduction of rawExtension for Ignition config objects via openshift#996, we also now use ignition.Parse directly to process the validity of ignition configs. This will cause machineconfigs without a Ignition section to fail, which we don't want. Also modify some error messages for clarity. Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>

yuqi-zhang · 2020-04-08T15:41:47Z

There are also other locations where we introduced ign.Parse directly but those paths shouldn't be affected like this

ashcrow

Looks good. Since we do the check and shuffle a few times it may make sense to make that part a function to reuse in the future.

LorbusChris

looks sensible

yuqi-zhang · 2020-04-08T15:51:40Z

/test e2e-gcp-rt-4.5

I wonder if this works...

kikisdeliveryservice · 2020-04-08T16:08:38Z

Can we add a test for these types of configs to our e2e? If this is a valid config, then adding the test to be a POC of this PR and as insurance would be good.

yuqi-zhang · 2020-04-08T16:28:14Z

Will take a look at adding a test to catch this regression. Alternatively I can also modify existing tests to have empty ignition sections to catch this

kikisdeliveryservice · 2020-04-08T17:56:56Z

Will take a look at adding a test to catch this regression. Alternatively I can also modify existing tests to have empty ignition sections to catch this

If I'm understanding correctly, you should be able to just mod this test and remove igncfg, no?

machine-config-operator/test/e2e/mcd_test.go

Line 229 in 219a942

func TestKernelType(t *testing.T) {

yuqi-zhang · 2020-04-08T18:45:09Z

Discussed with Kirsten, the test is harder to fix than that because:

Today our tests uses this clientset function to create a machineconfig to post to the server: https://github.com/openshift/machine-config-operator/blob/master/pkg/generated/clientset/versioned/typed/machineconfiguration.openshift.io/v1/machineconfig.go#L90
this requires a NON-EMPTY config (ignition) section, e.g. what we do here: https://github.com/openshift/machine-config-operator/blob/master/test/e2e/mcd_test.go#L243 which creates a ignition section with just version field. User applications of empty configs sections, however, is perfectly fine. So until we modify that behaviour we are stuck unable to test this case.

So for now we can merge this PR (I manually tested on GCP to fix). I will open another ticket regarding the test discrepancy

kikisdeliveryservice · 2020-04-08T18:56:08Z

I'm swamped.. Can someone else test this pre-merge?

kikisdeliveryservice · 2020-04-08T18:58:37Z

im spinning up cluster just in case..
hold to verify
/hold

yuqi-zhang · 2020-04-08T18:59:27Z

/retest

kikisdeliveryservice · 2020-04-08T20:05:53Z

/hold cancel

kikisdeliveryservice

verified

/lgtm

openshift-ci-robot · 2020-04-08T20:08:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, kikisdeliveryservice, 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:

~~OWNERS~~ [ashcrow,kikisdeliveryservice,yuqi-zhang]

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

yuqi-zhang · 2020-04-08T21:19:13Z

/test e2e-gcp-upgrade

openshift-bot · 2020-04-08T21:23:54Z

/retest

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

kikisdeliveryservice · 2020-04-08T21:46:40Z

I took a look a the gcp-upgrade failure was unrelated to this PR. Will just have to retest..

kikisdeliveryservice · 2020-04-09T00:50:26Z

No degraded pools or errors in mc* logs.

Will just let retest bot handle this

openshift-bot · 2020-04-09T02:22:48Z

/retest

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

openshift-bot · 2020-04-09T03:27:49Z

/retest

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

openshift-bot · 2020-04-09T04:45:45Z

/retest

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

openshift-ci-robot · 2020-04-09T04:48:39Z

@yuqi-zhang: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-scaleup-rhel7	`823c298`	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 · 2020-04-09T04:58:50Z

/retest

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

openshift-ci-robot · 2020-04-09T06:05:22Z

@yuqi-zhang: All pull requests linked via external trackers have merged: openshift/machine-config-operator#1631. Bugzilla bug 1821888 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1821888: controller: do not error on empty Ignition configs

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.

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 8, 2020

openshift-ci-robot requested review from mtrmac and umohnani8 April 8, 2020 15:37

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2020

yuqi-zhang requested review from LorbusChris and removed request for mtrmac and umohnani8 April 8, 2020 15:37

yuqi-zhang force-pushed the fix-ign-parsing branch from b3f391d to 823c298 Compare April 8, 2020 15:41

ashcrow approved these changes Apr 8, 2020

View reviewed changes

LorbusChris approved these changes Apr 8, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2020

kikisdeliveryservice approved these changes Apr 8, 2020

View reviewed changes

kikisdeliveryservice added the lgtm Indicates that a PR is ready to be merged. label Apr 8, 2020

openshift-ci-robot assigned kikisdeliveryservice Apr 8, 2020

openshift-merge-robot merged commit 04c69c5 into openshift:master Apr 9, 2020

Bug 1821888: controller: do not error on empty Ignition configs #1631

Bug 1821888: controller: do not error on empty Ignition configs #1631

Uh oh!

Conversation

yuqi-zhang commented Apr 8, 2020

Uh oh!

openshift-ci-robot commented Apr 8, 2020

Uh oh!

yuqi-zhang commented Apr 8, 2020

Uh oh!

yuqi-zhang commented Apr 8, 2020

Uh oh!

ashcrow left a comment

Choose a reason for hiding this comment

Uh oh!

LorbusChris left a comment

Choose a reason for hiding this comment

Uh oh!

yuqi-zhang commented Apr 8, 2020

Uh oh!

kikisdeliveryservice commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuqi-zhang commented Apr 8, 2020

Uh oh!

kikisdeliveryservice commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuqi-zhang commented Apr 8, 2020

Uh oh!

kikisdeliveryservice commented Apr 8, 2020

Uh oh!

kikisdeliveryservice commented Apr 8, 2020

Uh oh!

yuqi-zhang commented Apr 8, 2020

Uh oh!

kikisdeliveryservice commented Apr 8, 2020

Uh oh!

kikisdeliveryservice left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Apr 8, 2020

Uh oh!

yuqi-zhang commented Apr 8, 2020

Uh oh!

openshift-bot commented Apr 8, 2020

Uh oh!

kikisdeliveryservice commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kikisdeliveryservice commented Apr 9, 2020

Uh oh!

openshift-bot commented Apr 9, 2020

Uh oh!

openshift-bot commented Apr 9, 2020

Uh oh!

openshift-bot commented Apr 9, 2020

Uh oh!

openshift-ci-robot commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-bot commented Apr 9, 2020

Uh oh!

openshift-ci-robot commented Apr 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kikisdeliveryservice commented Apr 8, 2020 •

edited

Loading

kikisdeliveryservice commented Apr 8, 2020 •

edited

Loading

kikisdeliveryservice commented Apr 8, 2020 •

edited

Loading

openshift-ci-robot commented Apr 9, 2020 •

edited

Loading