state, filter: Fix the interpretation of ifaces names #709
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kubevirt-bot
added
dco-signoff: yes
EdDev
force-pushed
the
fix-nmstate-state-interpretation
branch
from
b0fe982 to
98bc857
Compare
|
change: Fixed gofmt |
qinqon
reviewed
Mar 2, 2021
In order to ease the interpretation of the state yaml data, use a type
structure that represents the basic root items of state:
- interfaces
- routes:
- config
- running
In order to keep the generic nature of these items content,
they use as value a generic Go interface{}.
The filter helper functions, with this change, have been also made
immutable. They now return the filtered values.
Signed-off-by: Edward Haas <[email protected]>
EdDev
force-pushed
the
fix-nmstate-state-interpretation
branch
from
98bc857 to
d735b2c
Compare
|
change: Do not expose the new state types |
EdDev
force-pushed
the
fix-nmstate-state-interpretation
branch
3 times, most recently
from
0390c71 to
1212087
Compare
qinqon
requested changes
Mar 3, 2021
EdDev
force-pushed
the
fix-nmstate-state-interpretation
branch
from
be618a0 to
651d6d6
Compare
qinqon
approved these changes
Mar 3, 2021
kubevirt-bot
added
lgtm
|
Looks like problem is just with float64 @EdDev can you add a unit test for that ? |
Interfaces names with numeric values are accepted by the kernel. Introduce tests that checks the behavior when using numeric names. Signed-off-by: Edward Haas <[email protected]>
In scenarios where an interface name is composed of numbers in scientific notation without a dot, the application panics with a failure to extract a string type from a Go interface (which is actually a float64). In order to fix the problem, the parsing of the interface name is performed using a dedicated interface-state structure. Note: this change does not solve the problem of incorrectly representing valid scientific numeric values like `10e+02` as strings (they are now represented back as full integers: "1000"). This is due to the YAML-to-JSON conversion which does not obey to the defined member type. ref: yaml/pyyaml#173 Signed-off-by: Edward Haas <[email protected]>
This change solves the problem of incorrectly representing valid scientific numeric valuesi without a dot in them (e.g. `10e20`) as strings (e.g. represented back as `1000`). The problem originates from the YAML-to-JSON conversion which does not obey to the defined member type. Fixed by using the go-yaml package (which does not perform such a conversion from YAML to JSON). The go-yaml is used just to unmarshal the name from the original raw byte stream and update the state structure with the proper name string. Signed-off-by: Edward Haas <[email protected]>
EdDev
force-pushed
the
fix-nmstate-state-interpretation
branch
from
651d6d6 to
4db577f
Compare
I have changed a bit the commits to show better what has been done and what each fixes. |
|
/lgtm We will do a follow up to try to use just one yaml parser. |
kubevirt-bot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qinqon 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 |
kubevirt-bot
added
the
approved
|
@EdDev: The following test failed, say
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Is this a BUG FIX or a FEATURE ?:
/kind bug
What this PR does / why we need it:
In scenarios where an interface has only numeric characters, the
application panics with a failure to extract a string type from a Go
interface (which is actually a float64).
In order to fix the problem, the parsing of the interface name is performed
using a dedicated interface-state structure.
In addition, the representation of special numeric values that an interface name can have is also fixed, such that
0xfecan be correctly represented back and not shown as an integer (254).In order to support this, a type structure that represents the basic state root items has been used.
This structure includes:
In order to keep the generic nature of these items content,
they use as value a generic Go interface{}.
Special notes for your reviewer:
Release note: