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.

Sign up for GitHub

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

ZTS: Remove non-standard awk hex numbers usage #16892

Closed
wants to merge 3 commits into from
Closed

ZTS: Remove non-standard awk hex numbers usage #16892

wants to merge 3 commits into from
+6 −22

Conversation

amotin
Copy link
Member

@amotin amotin commented Dec 20, 2024 •
edited
Loading

FreeBSD recently removed non-standard hex numbers support from awk. Neither it supports -n argument, enabling it in gawk. Instead of depending on those rewrite list_file_blocks() function to handle the hex math in shell instead of awk.

Fixes #11141

How Has This Been Tested?

Called the function manually and saw it returning some numbers instead of constant 4MB offset and zero length.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@amotin amotin mentioned this pull request Dec 20, 2024
Closed
13 tasks
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Dec 20, 2024
@amotin
Copy link
Member Author

amotin commented Dec 23, 2024 •
edited
Loading

Looking on remaining failures I was able to reproduce one in rsend/recv_dedup_encrypted_zvol. It seems to be a diff regression in FreeBSD 15. The diff binary from FreeBSD 14 shows no problem, so it seems not a ZFS issue. I've sent email to possibly related developer.

PS: Fixed in FreeBSD by https://cgit.freebsd.org/src/commit/?id=893839b119880d3fe8ab18aba4563af6c80cb875 . I have suspicion that it might fix cli_root/zfs_diff/zfs_diff_mangle also, but I am not sure the test is fully correct there either, since it seems to rely on the assumption that diff won't ever exit early.

@amotin
Copy link
Member Author

amotin commented Dec 25, 2024 •
edited
Loading

Added one more commit trying to fix unrelated failure in zpool_import/zpool_import_status on FreeBSD and its consequences.

@amotin amotin requested a review from tonyhutter December 26, 2024 21:19
@amotin
Copy link
Member Author

amotin commented Dec 26, 2024

@mcmilk It seems CI is still running 15 CURRENT from December 12 (2 weeks ago), failing tests due to not having diff patched. Have you had a chance to look on the official images to have it updating?

@amotin amotin added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 27, 2024
@amotin
ZTS: Remove non-standard awk hex numbers usage
d9f79ce 
FreeBSD recently removed non-standard hex numbers support from awk.
Neither it supports -n argument, enabling it in gawk.  Instead of
depending on those rewrite list_file_blocks() fuction to handle the
hex math in shell instead of awk.

Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
@amotin
ZTS: Remove procfs use from zpool_import_status
d034cf6 
procfs might be not mounted on FreeBSD.  Plus checking for specific
PID might be not exactly reliable.  Check for empty list of jobs
instead.

Premature loop exit can result in failed test and failed cleanup,
failing also some following tests.

Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
@amotin
ZTS: Reduce file size in redacted_panic to 1GB
b81428a 
This test takes 3 minutes on RELEASE FreeBSD bots, but on CURRENT,
probably due to debugging it has in kernel, it does not complete
within 10 minutes, ending up killed.  As I see all the redacting
here happens within the first ~128MB of the file, so I hope it
won't matter if there is 1GB of data instead of 2GB.

Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
@github-actions github-actions bot removed the Status: Accepted Ready to integrate (reviewed, tested) label Dec 27, 2024
@amotin amotin added the Status: Accepted Ready to integrate (reviewed, tested) label Dec 27, 2024
@mcmilk
Copy link
Contributor

mcmilk commented Dec 28, 2024

@mcmilk It seems CI is still running 15 CURRENT from December 12 (2 weeks ago), failing tests due to not having diff patched. Have you had a chance to look on the official images to have it updating?

They seem to get an IP, which is good: https://github.com/mcmilk/zfs/actions/runs/12528035230/job/34942221559
But sudo and/ot bash are not installed. I am trying to work around this.

@amotin
Copy link
Member Author

amotin commented Dec 28, 2024

But sudo and/ot bash are not installed. I am trying to work around this.

FreeBSD never had bash or sudo in a base system. They are always installed from ports or packages.

@behlendorf behlendorf closed this Dec 29, 2024
@behlendorf
Copy link
Contributor

Merged, but I mistakenly didn't include the PR number in the commit message so I was forced to manually close the PR.

a153397 ZTS: Reduce file size in redacted_panic to 1GB
b66d910 ZTS: Remove procfs use from zpool_import_status
8bf1e83 ZTS: Remove non-standard awk hex numbers usage

@mcmilk
Copy link
Contributor

mcmilk commented Dec 30, 2024

But sudo and/ot bash are not installed. I am trying to work around this.

FreeBSD never had bash or sudo in a base system. They are always installed from ports or packages.

I tried different ways of getting the FreeBSD cloud-init enabled images to work.
There seems to be no documentation on it and the default behaviour does not work as expected.
The runcmd:, passwd: and ssh_authorized_keys: sections seem not to work... the ssh server itself is there...

I will try this in a month or so again...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonyhutter tonyhutter tonyhutter approved these changes

@mcmilk mcmilk mcmilk approved these changes

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amotin @mcmilk @behlendorf @tonyhutter