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

Jump to bottom

Add validation to xfs scenario #10775

Merged
merged 2 commits into from Aug 10, 2020
Merged

Add validation to xfs scenario #10775

merged 2 commits into from Aug 10, 2020

Conversation

b10n1k
Copy link
Contributor

@b10n1k b10n1k commented Aug 3, 2020 •
edited

@b10n1k b10n1k changed the title Add validation to xfs scenario WIP: Add validation to xfs scenario Aug 3, 2020
@b10n1k b10n1k force-pushed the 69226_xfs_validation branch 6 times, most recently from be26f80 to 60cf72b Compare August 5, 2020 10:57
@b10n1k b10n1k changed the title WIP: Add validation to xfs scenario Add validation to xfs scenario Aug 5, 2020
@b10n1k b10n1k force-pushed the 69226_xfs_validation branch 4 times, most recently from 83a97cc to 7a0f1e7 Compare August 5, 2020 12:43
rwx788
rwx788 reviewed Aug 7, 2020
View reviewed changes
Copy link
Member

@rwx788 rwx788 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it worked mainly because we don't validate size of partitions. On powerVM, for instance, we have 40 GB disk, which will require separate test data. So if we remove that from yamls, not to confuse anyone, we are good to go. We can consider adding validation of the partition sizes in future.

test_data/yast/xfs/xfs_partition.yaml Outdated Show resolved Hide resolved
test_data/yast/xfs/xfs_partition.yaml Outdated
- name: vda
allowed_unpartitioned: 0.00GB
partitions:
- size: 9250Mib
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the code of console/validate_fs_table looks like we don't really validates size of the partitions. I guess you were confused with same parameters in msdos yaml. I guess we can skip it for now, as we should implement solution which ignores rounding errors due to block size, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please.

test_data/yast/xfs/xfs_partition.yaml Outdated Show resolved Hide resolved
rwx788
rwx788 approved these changes Aug 7, 2020
View reviewed changes
Copy link
Member

@rwx788 rwx788 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@b10n1k
Copy link
Contributor Author

b10n1k commented Aug 7, 2020

Actually if you dont mind give me some time to update it with a small change in the representation of the test_data. I kinda do not like the [SWAP] and i think it should be there just as SWAP.

@rwx788
Copy link
Member

rwx788 commented Aug 7, 2020

Actually if you dont mind give me some time to update it with a small change in the representation of the test_data. I kinda do not like the [SWAP] and i think it should be there just as SWAP.

Sure, why not. But, please, adjust other test data files which contain [SWAP], so we are consistent.

@b10n1k b10n1k force-pushed the 69226_xfs_validation branch 2 times, most recently from e9bdf8b to 13b1832 Compare August 8, 2020 08:42
@b10n1k b10n1k requested a review from rwx788 August 10, 2020 09:33
rwx788
rwx788 reviewed Aug 10, 2020
View reviewed changes
Copy link
Member

@rwx788 rwx788 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks promising!

lib/partitions_validator_utils.pm Outdated
@@ -38,7 +39,7 @@ sub validate_partition_creation {
my @lsblk_output = split(/\n/, script_output("lsblk -n"));
my $check;
foreach (@lsblk_output) {
if ($_ =~ /(?<check>\Q$args->{mount_point}\E\s*\Z)/) {
if ($_ =~ /(?<check>$args->{mount_point}]?\Z)/) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should get rid of \Z instead of looking for optional square bracket. Or at least we should escape it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\Z ensure that it looks for matching at the end of the line. i think it is fine to keep it. without it if [SWAP] is represented twice in the line it would not get the right one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then let's escape ] and maybe even add one in from of the regexp, so it's clear that we look for something in brackets.

Copy link
Member

@rwx788 rwx788 Aug 10, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we don't use /m flag for multiline search, we could use /\[?(?<check>$args->{mount_point})\]?$/. Also, if we would use lsblk -o MOUNTPOINT we would get list of mount points only and we could just match string SWAP or path from test data without any risk of false positives. That would simplify regexp too, so we could just use /?<check>$args->{mount_point}/. We also don't really need to have named capture group and set flag to true. But that's side point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we take into account not match 'home1' for example? Removing \Z I think it has that effect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With lsblk -o MOUNTPOINT I believe that we can set strict expectations, even with strict match of the beginning of the string using ^.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/\[?(?<check>$args->{mount_point})\]?$/ is neat and with lsblk -o MOUNTPOINT works perfectly as expected. i adapted that. thanks. out of curiosity tho i think /(?<check>$args->{mount_point}]?\Z)/ match the ] literally so i got confused with you asking to escape it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not sure how it worked, maybe perl is smart enough to change behavior on the fly, but as [] are special symbols, it will be safe to escape them.

jknphy
jknphy reviewed Aug 10, 2020
View reviewed changes
Copy link
Contributor

@jknphy jknphy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice you solved the problem with the brackets for SWAP adjusting regex and test data, I didn't know that variable are interpolated in regex escaping characters but it seems so. Only added a minor comment about renaming.

lib/partitions_validator_utils.pm Outdated Show resolved Hide resolved
@b10n1k b10n1k force-pushed the 69226_xfs_validation branch 2 times, most recently from 762f85b to 61f1239 Compare August 10, 2020 12:08
@rwx788 rwx788 merged commit bf4b590 into os-autoinst:master Aug 10, 2020
@b10n1k b10n1k deleted the 69226_xfs_validation branch August 24, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwx788 rwx788 rwx788 approved these changes

@jknphy jknphy jknphy left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@b10n1k @rwx788 @jknphy