Fix cpu partitioning struct tag/field #5227
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@carbonin: This pull request references Jira Issue OCPBUGS-13310, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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. |
|
/test ? |
openshift-ci
bot
added
the
size/XS
|
@carbonin: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@carbonin: This pull request references Jira Issue OCPBUGS-13310, which is invalid:
Comment In response to this:
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. |
carbonin
changed the title
openshift-ci-robot
removed
jira/valid-reference
|
@carbonin: No Jira issue is referenced in the title of this pull request. In response to this:
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. |
|
/test edge-e2e-metal-assisted-4-10 |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #5227 +/- ##
==========================================
+ Coverage 67.46% 67.57% +0.10%
==========================================
Files 218 218
Lines 32683 32775 +92
==========================================
+ Hits 22051 22149 +98
+ Misses 8650 8644 -6
Partials 1982 1982 |
carbonin
force-pushed
the
yaml_cpu_partitioning
branch
from
7f8551b
to
140812d
Compare
|
/test edge-e2e-metal-assisted-4-10 |
|
I don't understand what this have to do with JSON vs YAML and why do we still keep both the JSON and YAML tags after this change? |
|
The failure was caused because we serialized a struct with an empty cpu partitioning field into yaml, but without the yaml tag. This left us with a field like All the other fields don't have json tags but this field is the only one where the serialized name doesn't match the field name so it also needs to be specified to ensure it's consistent. We use json deserialization to do the install config overrides. That said, after doing a brief test with this I noticed that all the json field names are capitalized because we are not currently providing the json struct tags. I'll change this one to agree with the others for consistency. |
|
/hold Going to ensure consistency for overrides |
openshift-ci
bot
added
the
do-not-merge/hold
carbonin
force-pushed
the
yaml_cpu_partitioning
branch
from
140812d
to
e8e90a4
Compare
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
The struct tag for cpu partitioning was using but in order for the install config to be rendered correctly in all cases it need the yaml tag like the other fields. Without the yaml tag cpu partitioning was being included in the install config even if it was empty with an invlid name. This meant that all 4.10 installs always failed to prepare for installation with an event like: ``` Failed to prepare the installation due to an unexpected error: failed generating install config for cluster 1583dc9b-e059-48fc-96b9-5e711d1de6b1: error running openshift-install manifests, level=fatal msg=failed to fetch Master Machines: failed to load asset "Install Config": failed to unmarshal install-config.yaml: failed to parse first occurence of unknown field: error unmarshaling JSON: while decoding JSON: json: unknown field "cpupartitioning" : exit status 1. Please retry later ``` To correct this while also maintaining the current overrides behavior the field name is updated to match the API name (cpuPartitioningMode) and the json struct tag is changed to yaml. This allows for overrides to work in a case-insensitive way (as is supported currently for all other fields) as well as for the install config yaml to render properly. NOTE: This field name is intentionally different than the one in the openshift installer repo. This allows for us to take advantage of the case-insensitive matching allowed by json unmarshaling while also using the correct name. https://issues.redhat.com/browse/OCPBUGS-13310
carbonin
force-pushed
the
yaml_cpu_partitioning
branch
from
e8e90a4
to
492ff1b
Compare
carbonin
changed the title
|
/test edge-e2e-metal-assisted-4-10 |
|
This should be ready for final review whenever you get the chance @omertuc |
|
/lgtm |
|
/retest |
|
@carbonin: all tests passed! Full PR test history. Your PR dashboard. 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. |
|
/cherry-pick cloud_hotfix_releases |
|
@osherdp: new pull request created: #5231 In response to this:
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. |
The struct tag for cpu partitioning was using but in order for the install config to be rendered correctly in all cases it need the yaml tag like the other fields. Without the yaml tag cpu partitioning was being included in the install config even if it was empty with an invlid name. This meant that all 4.10 installs always failed to prepare for installation with an event like: ``` Failed to prepare the installation due to an unexpected error: failed generating install config for cluster 1583dc9b-e059-48fc-96b9-5e711d1de6b1: error running openshift-install manifests, level=fatal msg=failed to fetch Master Machines: failed to load asset "Install Config": failed to unmarshal install-config.yaml: failed to parse first occurence of unknown field: error unmarshaling JSON: while decoding JSON: json: unknown field "cpupartitioning" : exit status 1. Please retry later ``` To correct this while also maintaining the current overrides behavior the field name is updated to match the API name (cpuPartitioningMode) and the json struct tag is changed to yaml. This allows for overrides to work in a case-insensitive way (as is supported currently for all other fields) as well as for the install config yaml to render properly. NOTE: This field name is intentionally different than the one in the openshift installer repo. This allows for us to take advantage of the case-insensitive matching allowed by json unmarshaling while also using the correct name. https://issues.redhat.com/browse/OCPBUGS-13310
The struct tag for cpu partitioning was using but in order for the
install config to be rendered correctly in all cases it need the yaml
tag like the other fields.
Without the yaml tag cpu partitioning was being included in the install
config even if it was empty with an invlid name.
This meant that all 4.10 installs always failed to prepare for
installation with an event like:
To correct this while also maintaining the current overrides behavior
the field name is updated to match the API name (cpuPartitioningMode)
and the json struct tag is changed to yaml.
This allows for overrides to work in a case-insensitive way (as is
supported currently for all other fields) as well as for the install
config yaml to render properly.
NOTE:
This field name is intentionally different than the one in the openshift
installer repo. This allows for us to take advantage of the
case-insensitive matching allowed by json unmarshaling while also using
the correct name.
Originally committed in #5207
List all the issues related to this PR
https://issues.redhat.com/browse/OCPBUGS-13310
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist