Skip to content
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.

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

Extreme 9k #1067

Closed
wants to merge 24 commits into from
Closed

Extreme 9k #1067

wants to merge 24 commits into from

Conversation

mayafox
Copy link
Contributor

@mayafox mayafox commented Jan 26, 2023

Added in Extreme 9920, 8520, and 8720 products, fixed incorrect wattage for 7400-48y, and removed mentions of AC from the model and parts for the MLXe and SLX 9xxx boxes.

@mayafox
Copy link
Contributor Author

mayafox commented Jan 26, 2023

Any guidance on resolving this? last I checked 0.01 would go into 10.11 just fine, and the yaml matches up with the sample output format shown in the main readme.md

weight:
  - value: 10.11
    unit: kg

device-types/Extreme Networks/8520-48Y-8C.yaml failed validation: 10.11 is not a multiple of 0.01

Failed validating 'multipleOf' in schema['properties']['weight']['items']['properties']['value']:
{'minimum': 0, 'multipleOf': 0.01, 'type': 'number'}

On instance['weight'][0]['value']:
10.11

Did a bit of poking around, changing the value to 1011 <<< integer, and not a float, caused the check to pass. This seems to potentially be an issue in jsonschema,

https://github.com/python-jsonschema/jsonschema/blob/main/jsonschema/_validators.py line 181

If I am reading the code right jsonschema is running into a rounding error.

db = 0.01
instance = 10.11
isinstance(db, float)
True
quotient = instance / db
quotient
1010.9999999999999
int(quotient)
1010
int(round(quotient, 2))
1011

@danner26
Copy link
Member

danner26 commented Jan 26, 2023

Hey @mayafox, the weight definition is brand new and we are clearly still tackling some issues around that. I will use this as a testing ground to fix that issue as well.

I also would just like to confirm, the correct weight should be 10.11 KG, right?

@danner26 danner26 mentioned this pull request Jan 26, 2023
@danner26
Copy link
Member

@mayafox I have merged PR #1069 which resolves your issue. We can use multipleOfPrecision instead of multipleOf which resolves the issue. I have tested this with 10, 10.1, 10.11, etc.

Updated weight back to 10.11
Updated weight back to 10.11
@danner26
Copy link
Member

@mayafox I started to update some of the weights but I do not know the actual values. I have merged the master branch back into yours, so you should be able to correct the weight values now. Can you please update the weights?

Please also make sure the weights are as accurate as possible, and in a comment on the device, please add a link to the device documentation. Thanks!

@danner26 danner26 added the status: revisions needed This issue requires additional information to be actionable label Jan 26, 2023
@mayafox
Copy link
Contributor Author

mayafox commented Jan 27, 2023

@danner26
Changes in support of fixing the weight look good!
Device documentation should be the hardware information URL, and not the OS admin guides I presume?
Should I call out the different power/airflow combinations as unique models or is AC w/ front-to-rear ok?

Also what's the preferred format for the comments you requested?
comments: '[Extreme Networks X440-G2 Technical Specifications](https://www.extremenetworks.com/product/x440-g2/)'
or
comments: https://www.extremenetworks.com/support/documentation/5520/

@danner26
Copy link
Member

danner26 commented Jan 27, 2023 via email

desnoe and others added 2 commits January 27, 2023 01:27
Co-authored-by: DESNOË Olivier (Canal Plus) <olivier.desnoe@canal-plus.com>
@cwegener
Copy link

@danner26 Just a quick note that you didn't actually "fix" the original issue with the multipleOf keyword. In #1069 you have unknowingly disabled the multipleOf keyword.

There is no such thing as a multipleOfPrecision keyword in the JSON Schema specification.

@danner26
Copy link
Member

@cwegener Thanks for the heads up, I am guessing that this issue isnt an actual solution then. Any idea why it works correctly then?

Do you have a proper solution for this?

@cwegener
Copy link

Do you have a proper solution for this?

Nope. I just noticed the obvious error and thought I point it out.

I'm in a similar situation like yourself. I'm aware of how floating point arithmetic works in Python (and any other language using binary for floating point) but also have no experience on how to properly apply libraries like jsonschema as part of the whole ecosystem.

My case is even more complex as I ran into this issue in the context of a distributed system where the JSON Schema spec is the 'connecting' tissue but the implementations that send data to each other are blissfully unaware of each other. (https://github.com/singer-io/getting-started/blob/master/docs/SPEC.md)

@danner26
Copy link
Member

Gotcha, I appreciate you pointing it out @cwegener ! Here's to hoping we find a solution

@danner26
Copy link
Member

danner26 commented Jan 31, 2023

@mayafox we fixed the decimal issue, please try applying the proper weight values again. Thank you for your patience

Also @cwegener feel free to take a look at our implementation, one of our devs was able to reverse engineer and fix the issue for us.

@mayafox
Copy link
Contributor Author

mayafox commented Jan 31, 2023

@danner26 Done!

@danner26
Copy link
Member

I am a little confused, what is going on with all of the renames such as the BR-MLXE-16-MR2-AC being renamed to BR-MLXE-16-MR2? I see the BR-MLXE-16-MR2-AC listed here.

We shouldnt be removing or renaming device definitions, you should add new files for different devices. Unless I am missing something. Can you explain? @mayafox

@mayafox
Copy link
Contributor Author

mayafox commented Jan 31, 2023

So there's a couple issues here with the -AC models. I chatted with one of my colleagues on adding in all the various Power supply (AC/DC) and air-flow combinations that are listed, and they viewed it as a moot point and not worth the effort (not sure they fully understood the question either). I also asked you what the projects preference was, but you missed that question as well. I don't mind doing the work to add in the variations, but a clear direction would be good. For the moment I dropped the -AC from the previously submitted items, and will await your response on what I need to do on this matter.

@danner26
Copy link
Member

Sorry about that, I totally missed that questions. So the only thing the -AC is used for is when the AC power module is installed? If that is the case, then I agree it is a moot point. I think modelling them without the AC makes sense, and then the modules that you install (AC or DC power supply) is what will determine which power type it uses. Is that understanding where your changes came from?

@mayafox
Copy link
Contributor Author

mayafox commented Jan 31, 2023

Yes, and for some boxes (MLX and the 9920) it is possible to have both AC and DC power supplies installed and in service. Airflow is generally uniform, and can impact the power supplies choices as well, but most of the platforms allow you to switch by swapping the AC PSU for DC, and swapping fans and PSU's to change the air flow. It's a mess of permutations I would rather not poke if I can avoid it.
For information purposes I am modeling them with the correct wattages, and AC connections and front to rear airflow, and max possible weight.

@danner26
Copy link
Member

danner26 commented Feb 1, 2023

So I see what the mess is here, this is quite the issue. Let me ask you this, if I were to go to the vendor and ask for a BR-MLXE-16-MR2-AC, would they provide me with the unit and the AC power supplies? Like do they actually list for sale BR-MLXE-16-MR2-AC that includes all of the kit, plus the BR-MLXE-16-MR2 which doesnt include the PSU? Or do you have to buy either a BR-MLXE-16-MR2-AC or a BR-MLXE-16-MR2-DC?

Because this library acts as a source of truth for a specific device, I do not think we should be removing/renaming legitimate models even if they do have the option to be changed on the fly. If a product can be purchased by default under the BR-MLXE-16-MR2-AC sku, it should have its own file. Just as if you can buy the product without the AC/DC PSUs, you should also have another file that is BR-MLXE-16-MR2.

Does that make sense? Unless you cannot buy a BR-MLXE-16-MR2-AC and you can only buy a BR-MLXE-16-MR2, then I think ultimately you should revert the renames/changes in those files.

@danner26 danner26 added status: gathering feedback Further discussion is needed to determine this issue's scope and/or implementation and removed status: revisions needed This issue requires additional information to be actionable labels Feb 1, 2023
@mayafox
Copy link
Contributor Author

mayafox commented Feb 1, 2023

@danner26
The TLDR answer is yeap it makes sense and I will update the platform files accordingly. Thanks for providing a clear direction on what's expected. Might take me a few days to get the work done though, the cube farm is demanding more of my time.

"Let me ask you this, if I were to go to the vendor and ask for a BR-MLXE-16-MR2-AC, would they provide me with the unit and the AC power supplies?" Based of the ordering information page from the data sheet, yeap, they used to list the bare chassis sans mgmt, and power, but that's been removed,

BR-MLXE-16-MR2-M-AC
MLXe-16 AC system with one MR2 (M) management module, three high-speed switch fabric modules, four 1,800W
AC power supplies, two exhaust fan assembly kits, and air filter. Power cord not included.
BR-MLXE-16-MR2-M-DC
MLXe-16 DC system with one MR2 (M) management module, three high-speed switch fabric modules, four 1,800W
DC power supplies, two exhaust fan assembly kits, and air filter. Power cord not included.
BR-MLXE-16-MR2-X-AC
MLXe-16 AC system with one MR2 (X) management module, three high-speed switch fabric modules, four 1,800W
AC power supplies, two exhaust fan assembly kits, and air filter. Power cord not included.
BR-MLXE-16-MR2-X-DC
MLXe-16 DC system with one MR2 (X) management module, three high-speed switch fabric modules, four 1,800W
DC power supplies, two exhaust fan assembly kits, and air filter. Power cord not included.

@mayafox
Copy link
Contributor Author

mayafox commented Feb 1, 2023

@danner26
Does this fall in line with what you expect? If so, I'll work on the rest of the Extreme boxes.

@danner26
Copy link
Member

danner26 commented Feb 2, 2023

Hey @mayafox take your time.. let me know when you are ready for me to review it again
And yes that looks like what I am thinking, whatever is the default configurations from the vendor is what we should define. The idea is that this is a repo that you can import the standardized models based off of, and then after imported and created in NetBox, the user can modify the device in NetBox from the default configuration to model the real world. Let me know if you need any clarification, I hope I am making sense

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2023

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further progress is made.

@github-actions github-actions bot added the stale label Mar 5, 2023
@danner26
Copy link
Member

danner26 commented Mar 9, 2023

Hello, unfortunately today we had to make a change to the schema in which weight is defined by. The upstream NetBox API requires weight to be passed to it in the following format:

weight: 12.01
weight_unit: lb

Due to this change, I have updated your branch against the current master. This might require you to update your PR, if you had weight currently defined, so that your PR is in-line with the new weight schema. If you have any issues, please let me know or refer to this PR for more information.

@mayafox
Copy link
Contributor Author

mayafox commented Apr 7, 2023

Closing for rework of the code.

@mayafox mayafox closed this Apr 7, 2023
@mayafox mayafox deleted the extreme-9k branch April 7, 2023 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale status: gathering feedback Further discussion is needed to determine this issue's scope and/or implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants