Skip to content

Conversation

@runcom
Copy link
Member

@runcom runcom commented Apr 30, 2020

TL;DR; if somebody provides fields like disks and other we don't specify in the crd
those get dropped and can cause whatever issue you can think of.
I think adding that will preserve the unknown field.

Signed-off-by: Antonio Murdaca [email protected]

holding pending tests

@runcom runcom added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 30, 2020
@openshift-ci-robot openshift-ci-robot added bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Apr 30, 2020
@openshift-ci-robot
Copy link
Contributor

@runcom: This pull request references Bugzilla bug 1829138, which is invalid:

  • expected Bugzilla bug 1829138 to depend on a bug targeting a release in 4.5.0, 4.5.z and in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but no dependents were found

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1829138: [release-4.4] MC CRD: preserve unknown fields

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2020
@kikisdeliveryservice
Copy link
Contributor

need a make update

@cgwalters
Copy link
Member

Needs a bindata update

@runcom
Copy link
Member Author

runcom commented Apr 30, 2020

yeah lol doing it rn

@kikisdeliveryservice kikisdeliveryservice self-requested a review April 30, 2020 01:49
@wking
Copy link
Member

wking commented Apr 30, 2020

/bugzilla refresh

@openshift-ci-robot
Copy link
Contributor

@wking: This pull request references Bugzilla bug 1829138, which is invalid:

  • expected the bug to target the "4.4.0" release, but it targets "4.5.0" instead
  • expected Bugzilla bug 1829138 to depend on a bug targeting a release in 4.5.0, 4.5.z and in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but no dependents were found

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

/bugzilla refresh

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.

@runcom runcom force-pushed the ignition-unknown-fields branch from 4905991 to 14284af Compare April 30, 2020 01:50
@runcom
Copy link
Member Author

runcom commented Apr 30, 2020

/bugzilla refresh

I'll fix BZs

@runcom
Copy link
Member Author

runcom commented Apr 30, 2020

I'm now wondering if every object in the tree needs this tweak also, I guess I'll find out shortly

@sdodson
Copy link
Member

sdodson commented Apr 30, 2020

/retitle Bug 1829138: MC CRD: preserve unknown fields

@openshift-ci-robot openshift-ci-robot changed the title Bug 1829138: [release-4.4] MC CRD: preserve unknown fields Bug 1829138: MC CRD: preserve unknown fields Apr 30, 2020
@sdodson
Copy link
Member

sdodson commented Apr 30, 2020

/retitle Bug 1829651: MC CRD: preserve unknown fields

@openshift-ci-robot openshift-ci-robot changed the title Bug 1829138: MC CRD: preserve unknown fields Bug 1829651: MC CRD: preserve unknown fields Apr 30, 2020
@openshift-ci-robot
Copy link
Contributor

@runcom: This pull request references Bugzilla bug 1829651, which is invalid:

  • expected dependent Bugzilla bug 1829138 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is NEW instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1829651: MC CRD: preserve unknown fields

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.

@runcom runcom force-pushed the ignition-unknown-fields branch from 14284af to e2b47b5 Compare April 30, 2020 02:18
@cgwalters
Copy link
Member

cgwalters commented Apr 30, 2020

Anyone who wants to test this, it should be sufficient to do oc create on this: https://github.com/openshift/release/blob/23b3ddb6b32e8157e9b882172264b8d1b008070c/clusters/build-clusters/01_cluster/machine_config/m5d4x_machineconfig.yaml

Then, notice the storage/disks field is dropped:

$ oc describe machineconfig/m5d4x
...
    storage: {}

After this PR merges you should see the storage section from the submitted object.

@runcom runcom force-pushed the ignition-unknown-fields branch from e2b47b5 to b15c501 Compare April 30, 2020 02:21
@kikisdeliveryservice
Copy link
Contributor

 level=fatal msg="failed to fetch Cluster: failed to fetch dependency of \"Cluster\": failed to generate asset \"Platform Permissions Check\": validate AWS credentials: checking install permissions: error simulating policy: Throttling: Rate exceeded\n\tstatus code: 400, request id: f6c3c3fb-9fc2-49e7-9717-395724888e9f"
error: failed to execute wrapped command: exit status 1 

retest

@wking
Copy link
Member

wking commented Apr 30, 2020

e2e-aws died in a throttled setup.

/test e2e-aws

@cgwalters
Copy link
Member

OK I tested this and it didn't work at first; ended up interactively editing the CRD with more x-kubernetes-preserve-unknown-fields: true and got it to work, will re-test with a clean diff from source.

TL;DR; if somebody provides fields like disks and other we don't specify in the crd
those get dropped and can cause whatever issue you can think of.
I think adding that will preserve the unkwown field.

Signed-off-by: Antonio Murdaca <[email protected]>
@cgwalters cgwalters force-pushed the ignition-unknown-fields branch from b15c501 to 6f63608 Compare April 30, 2020 02:37
@cgwalters
Copy link
Member

Ah, I think I was testing the original version of the PR, then did the fix and committed locally, but in the meantime Antonio pushed the same update I did. So this is likely good.

@openshift-cherrypick-robot

@wking: once the present PR merges, I will cherry-pick it on top of master in a new PR and assign it to you.

Details

In response to this:

/cherrypick master

😆 😭

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.

@sdodson
Copy link
Member

sdodson commented Apr 30, 2020

/cherrypick master

😆 😭

It'll fail on bindata conflicts

@openshift-ci-robot openshift-ci-robot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. and removed bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Apr 30, 2020
@openshift-ci-robot
Copy link
Contributor

@runcom: This pull request references Bugzilla bug 1829651, which is invalid:

  • expected dependent Bugzilla bug 1829138 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is NEW instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

[release-4.4] Bug 1829651: MC CRD: preserve unknown fields

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.

@wking wking added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Apr 30, 2020
@wking
Copy link
Member

wking commented Apr 30, 2020

I've restored the bug-label override after the Bugzilla bot responded to the PR retitle.

@wking
Copy link
Member

wking commented Apr 30, 2020

/hold cancel

We're letting this through if it passes CI, and may end up reverting later if something explodes.

@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 Apr 30, 2020
@kikisdeliveryservice
Copy link
Contributor

failures seem unrelated

/retest

@openshift-bot
Copy link
Contributor

/retest

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

@kikisdeliveryservice
Copy link
Contributor

/skip

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Apr 30, 2020

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

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 6f63608 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.

@sinnykumari
Copy link
Contributor

/retest

@wking
Copy link
Member

wking commented Apr 30, 2020

/override ci/prow/e2e-aws

@openshift-ci-robot
Copy link
Contributor

@wking: wking unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file.

Details

In response to this:

/override ci/prow/e2e-aws

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.

@mrunalp
Copy link
Member

mrunalp commented Apr 30, 2020

/override ci/prow/e2e-aws

@openshift-ci-robot
Copy link
Contributor

@mrunalp: mrunalp unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file.

Details

In response to this:

/override ci/prow/e2e-aws

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.

@kikisdeliveryservice
Copy link
Contributor

/override ci/prow/e2e-aws

@openshift-ci-robot
Copy link
Contributor

@kikisdeliveryservice: Overrode contexts on behalf of kikisdeliveryservice: ci/prow/e2e-aws

Details

In response to this:

/override ci/prow/e2e-aws

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.

@sinnykumari
Copy link
Contributor

sorry about the mess, i didn't realize that folks are already onto this PR :/ Added retest to get the pr merged soon considering the priority :/

@kikisdeliveryservice
Copy link
Contributor

sorry about the mess, i didn't realize that folks are already onto this PR :/ Added retest to get the pr merged soon considering the priority :/

no worries sinny!!! 🤗

@openshift-merge-robot openshift-merge-robot merged commit 46ea964 into openshift:release-4.4 Apr 30, 2020
@openshift-ci-robot
Copy link
Contributor

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

Details

In response to this:

[release-4.4] Bug 1829651: MC CRD: preserve unknown fields

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-cherrypick-robot

@wking: #1698 failed to apply on top of branch "master":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
A	manifests/machineconfig.crd.yaml
M	pkg/operator/assets/bindata.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/operator/assets/bindata.go
CONFLICT (content): Merge conflict in pkg/operator/assets/bindata.go
Auto-merging install/0000_80_machine-config-operator_01_machineconfig.crd.yaml
Patch failed at 0001 MC CRD: preserve unknown fields

Details

In response to this:

/cherrypick master

😆 😭

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

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.