Parse the disk.sector-size hardware requirement
#2972
Merged
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
skycastlelily
requested review from
psss,
lukaszachy,
happz,
thrix and
janhavlin
as code owners
skycastlelily
added
the
ci | full test
happz
added
specification
happz
reviewed
Jun 15, 2024
|
@skycastlelily when you have some spare time, rebase on top of the recent main, please, and resolve conflicts. |
1 similar comment
|
@skycastlelily when you have some spare time, rebase on top of the recent main, please, and resolve conflicts. |
skycastlelily
force-pushed
the
sector-size
branch
from
9750284
to
0dbdf0c
Compare
|
Updated^^
…On Sat, Jun 15, 2024 at 4:52 PM Miloš Prchlík ***@***.***> wrote:
@skycastlelily <https://github.com/skycastlelily> when you have some
spare time, rebase on top of the recent main, please, and resolve conflicts.
—
Reply to this email directly, view it on GitHub
<#2972 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23ENVUOWCNTIQ75QO4DZHP6DLAVCNFSM6AAAAABIR6OOBSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRZGIYTSNRYGE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
happz
reviewed
Jun 18, 2024
happz
reviewed
Jun 18, 2024
skycastlelily
force-pushed
the
sector-size
branch
from
0dbdf0c
to
1b76aba
Compare
|
Updated^^
…On Tue, Jun 18, 2024 at 3:27 PM Miloš Prchlík ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tmt/hardware.py
<#2972 (comment)>:
> @@ -1158,7 +1158,6 @@ def _parse_disk(spec: Spec, disk_index: int) -> BaseConstraint:
group.constraints += _parse_size_constraints(spec, f'disk[{disk_index}]', ('size',))
group.constraints += _parse_text_constraints(spec,
f'disk[{disk_index}]', ('model-name', 'driver'))
-
And unrelated change, empty line removed ;)
—
Reply to this email directly, view it on GitHub
<#2972 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23APMYM2YDNIOK53UY3ZH7OL7AVCNFSM6AAAAABIR6OOBSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMRUG4YTKMZXGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
skycastlelily
force-pushed
the
sector-size
branch
from
1b76aba
to
0af1e86
Compare
therazix
requested changes
Jun 27, 2024
skycastlelily
force-pushed
the
sector-size
branch
from
0af1e86
to
0fba735
Compare
|
Updated,thanks for your review:)
…On Thu, Jun 27, 2024 at 10:57 PM Filip Vágner ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In spec/hardware/disk.fmf
<#2972 (comment)>:
> + # Number or String, logical sector size
+ - logical-sector-size: 512|">= 512"
+
+ # Number or String, physical sector size
+ - physical-sector-size: 4096|">= 4096"
⬇️ Suggested change
- # Number or String, logical sector size
- - logical-sector-size: 512|">= 512"
-
- # Number or String, physical sector size
- - physical-sector-size: 4096|">= 4096"
+ # Number or string, logical sector size.
+ - logical-sector-size: 512|">= 512"
+
+ # Number or string, physical sector size.
+ - physical-sector-size: 4096|">= 4096"
------------------------------
In tests/unit/test_hardware.py
<#2972 (comment)>:
> + physical-sector-size: "4096"
- size: 120 GiB
driver: virtblk
+ logical-sector-size: "512"
The test_parse_maximal_constraint in tests/unit/test_hardware.py fails
after this change. Could you also update the expected result?
—
Reply to this email directly, view it on GitHub
<#2972 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23D6LFISJWDVCLLUJJDZJQR67AVCNFSM6AAAAABIR6OOBSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNBVGY3DMOJWGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
therazix
approved these changes
Jun 28, 2024
happz
reviewed
Jul 2, 2024
skycastlelily
force-pushed
the
sector-size
branch
from
0fba735
to
669f480
Compare
|
What do you think?
^^
we would add a note to HW requirements specification, to memory and
disk.size and sector sizes, "Hey, the default unit is bytes".
I think "# Bytes are assumed when no unit is specified." is for that,please
let me know if I miss anything:)
…On Wed, Jul 3, 2024 at 3:46 AM Miloš Prchlík ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/unit/test_hardware.py
<#2972 (comment)>:
> @@ -257,9 +259,11 @@ def test_parse_maximal_constraint() -> None:
- and:
- and:
- disk[0].size: == 40 GiB
+ - disk[0].physical-sector-size: == 4096 dimensionless
Now this is unexpected - it's not dimensionless, right? What should be the
unit, bytes?
1. the examples should include units,
2. the specification does not mention units anywhere, disk.size and
memory do not mention the default unit, because there is none, which
is not correct, so let's keep sector size also not mentioning unit in its
specification, just examples, and
3. in a dedicated patch, we should add a bit of code to tmt.hardware,
to coerce dimensionless values of SizeConstraint to values with
dimensions. Something like a default unit passed to
SizeConstraint.from_specification, is to be used for dimensionless
values. So far it seems the default would be bytes, could be a keyword
argument with the bytes default. Together with this addition, we would
add a note to HW requirements specification, to memory and disk.size
and sector sizes, "Hey, the default unit is bytes".
What do you think?
—
Reply to this email directly, view it on GitHub
<#2972 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23BFB5ZGCL2SPPMUKQTZKL7RPAVCNFSM6AAAAABIR6OOBSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNJUG4YTQOJTGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Blocked by #3076 |
|
@skycastlelily #3076 is now merged, time to wrap this one up :) |
happz
removed
the
status | blocked
skycastlelily
force-pushed
the
sector-size
branch
from
9256a53
to
97d3e28
Compare
|
Updated^^
…On Mon, Jul 22, 2024 at 3:14 PM Miloš Prchlík ***@***.***> wrote:
@skycastlelily <https://github.com/skycastlelily> #3076
<#3076> is now merged, time to wrap
this one up :)
—
Reply to this email directly, view it on GitHub
<#2972 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23EDAGW3HWDRXUGXT7TZNSWLFAVCNFSM6AAAAABIR6OOBSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBSGI2TANRVGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
happz
reviewed
Jul 22, 2024
|
Updated😅
…On Mon, Jul 22, 2024 at 8:03 PM Miloš Prchlík ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In spec/hardware/disk.fmf
<#2972 (comment)>:
> @@ -60,11 +68,13 @@ example:
model-name: PERC H310
driver: megaraid_sas
block-device: /dev/sda
+ logical-sector-size: "512 byte"
Indentation by a tab instead of spaces cripples fmf parsing pretty much
everywhere.
—
Reply to this email directly, view it on GitHub
<#2972 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23CCAKJWPYMKDBVJJZDZNTYCTAVCNFSM6AAAAABIR6OOBSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJRGM4DINZXGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
happz
approved these changes
Jul 22, 2024
|
Unrelated failures, merging. |
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.
Pull Request Checklist