-
Notifications
You must be signed in to change notification settings - Fork 157
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
Update to OpenZFS 2.2.2 #3779
Update to OpenZFS 2.2.2 #3779
Conversation
We need this in the 11.0-stable branch as well. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3779 +/- ##
=======================================
Coverage 19.69% 19.69%
=======================================
Files 235 235
Lines 51757 51757
=======================================
+ Hits 10192 10193 +1
+ Misses 40825 40823 -2
- Partials 740 741 +1 ☔ View full report in Codecov by Sentry. |
It's on the way |
@eriknordmark Here is the same PR for 11.0-stable #3780 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kick off tests
@eriknordmark do we want to merge this to stable branches as well? Update: I found your comment, where you say we need this to backport to the 11.0-stable. Only there, correct? |
pkg/dom0-ztools/Dockerfile
Outdated
ENV ZFS_COMMIT=zfs-${ZFS_VERSION} | ||
ENV ZFS_REPO=https://github.com/openzfs/zfs | ||
ENV ZFS_PATCH_DIR=/patches-zfs-"${ZFS_VERSION}" | ||
|
||
WORKDIR /tmp/zfs | ||
|
||
ADD ${ZFS_REPO}/tarball/${ZFS_COMMIT}/ zfs.tgz | ||
RUN tar -zxvf zfs.tgz --strip-components=1 && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewd-zededa since you are touching here I would like to propose an improvement apart from your required changes: Eliminate the additional step of extracting the tarball.... we can just simply fetch directly from github using the ADD command, like this:
diff --git a/pkg/dom0-ztools/Dockerfile b/pkg/dom0-ztools/Dockerfile
index dcd7c86a6..3ba4a396c 100644
--- a/pkg/dom0-ztools/Dockerfile
+++ b/pkg/dom0-ztools/Dockerfile
@@ -10,14 +10,12 @@ COPY /patches /
# * ZFS on Linux
ENV ZFS_VERSION=2.1.2
ENV ZFS_COMMIT=zfs-${ZFS_VERSION}
-ENV ZFS_REPO=https://github.com/openzfs/zfs
+ENV ZFS_REPO=https://github.com/openzfs/zfs.git
ENV ZFS_PATCH_DIR=/patches-zfs-"${ZFS_VERSION}"
WORKDIR /tmp/zfs
+ADD ${ZFS_REPO}#${ZFS_COMMIT} /tmp/zfs
-ADD ${ZFS_REPO}/tarball/${ZFS_COMMIT}/ zfs.tgz
-RUN tar -zxvf zfs.tgz --strip-components=1 && \
- rm zfs.tgz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I'll make the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
vdevs.ScanStat.ToExamine = uint64(ps.pss_to_examine) | ||
vdevs.ScanStat.Examined = uint64(ps.pss_examined) | ||
- vdevs.ScanStat.ToProcess = uint64(ps.pss_to_process) | ||
+ //vdevs.ScanStat.ToProcess = uint64(ps.pss_to_process) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't need this line, just remove it......
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this in my go-libzfs fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in fork
pkg/pillar/Dockerfile
Outdated
@@ -92,6 +92,12 @@ ENV GOARCH=${TARGETARCH} | |||
ENV CC=${CROSS_COMPILE_ENV}gcc | |||
ARG GOPKGVERSION | |||
|
|||
# Temporary gomodule patches until accepted upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've never patched go modules directly like this... IMO would be better to push this modified version to a fork on GH (you did these changes, right? So you can push them to a forked repo under your account) and use the fork as the GO module (changing at the go.mod). Once the changes become upstream, you can update the go.mod to fetch from the official repo again...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I authored this patch. Sounds good, I'll make the changes to importing a module instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Yes, because with the old kernel (which lived in eve/pkg/kernel) we had the old ZFS patches to get the same behavior. |
kernel-commits.mk
Outdated
KERNEL_COMMIT_amd64_v6.1.38_rt = 49dba60a59d9 | ||
KERNEL_COMMIT_arm64_v5.10.104_nvidia = c21d5a4dd498 | ||
KERNEL_COMMIT_arm64_v5.10.186_generic = 6a861e0433a6 | ||
KERNEL_COMMIT_arm64_v6.1.38_generic = 92466d23a9bf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewd-zededa , please, move the kernel update to a dedicated commit, so it helps with traceability (our kernel updates are all in dedicated commits) and separates logically the changes of your PR....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split the kernel change out to its own commit. I can squash the other two commits down to one if needed.
b382721
to
f11b95d
Compare
@rene @christoph-zededa @zedi-pramodh Please review my latest changes as time allows. If this looks good I will copy them to the 11.0-stable PR as well. |
@andrewd-zededa , LGTM, I have just some final comments:
|
f11b95d
to
8326f97
Compare
8326f97
to
86941af
Compare
@rene I rebased to fix the first yetus issue. The next yetus issue "could not import github.com/andrewd-zededa/go-libzfs" is not new in my PR, here's an example of it seen with the old bicomsystems version of the module.
Since 3793 has not been merged yet can we instead merge this one first as a complete pr? |
Thanks @andrewd-zededa , I checked this yetus error, that's happening due to the lack of the libzfslinux-dev package inside the yetus container. We can ignore it in your PR, but we'll need to think in a proper fix for that... As you probably saw, the PR #3793 was merged. So I think you just need to drop the kernel update commit and we are good to go... |
@andrewd-zededa , I've started a PR to fix Yetus issue: #3798 , feel free to review and comment on it... it's still a draft because installing the libzfs is now leading to another Yetus errors.... Edit: |
Remove now unused patches which are in upstream. Remove module param modifications which no longer exist. Signed-off-by: Andrew Durbin <andrewd@zededa.com>
Migrate go-libzfs to a fork including 2.2.2 support. Signed-off-by: Andrew Durbin <andrewd@zededa.com>
86941af
to
e0ea38d
Compare
Ok @rene, dropped the kernel commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
See comment in #3780 |
@@ -10,7 +10,7 @@ ARG BUILD_PKGS_BASE="git gcc linux-headers libc-dev make linux-pam-dev m4 findut | |||
# we use the same image in several places | |||
ARG EVE_ALPINE_IMAGE=lfedge/eve-alpine:9fb9b9cbf7d90066a70e4704d04a6fe248ff52bb | |||
|
|||
FROM lfedge/eve-dom0-ztools:417d4ff6a57d2317c9e65166274b0ea6f6da16e2 as zfs | |||
FROM lfedge/eve-dom0-ztools:81a7af28b32c7558e1c4622e60a9e3197075decd as zfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this not pull in the correct version with the include files etc?
The unit tests fail consistently. How can we fix that?
Any ideas @rene ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be that it is like when we update eve-alpine? First the PR for eve-dom0-ztools has to be merged and only then it can be used in other Dockerfiles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to get the unit tests to build and pass.
8317674
to
751d367
Compare
Now libzfs headers are available for go-libzfs cgo build. Signed-off-by: Andrew Durbin <andrewd@zededa.com>
751d367
to
0ce6a55
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like with #3804 we can avoid having to build this and avoid keeping the version of zfs packages in the build container in synch with the kernel.
@eriknordmark should I back out my latest commit and then this can merge after #3804 merges? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this move to OpenZFS 2.2.2 eliminate those custom patch files? If so, that is a lovely thing.
Separately, why are we depending on a custom fork of go-libzfs
which is in a personal repo? I refer to this and about 10 other places.
I think that makes sense unless @rene thinks that is a bad idea. |
[Answering for Andrew since it is late for him]
Some fixes were needed for 2.2.2. See bicomsystems/go-libzfs@master...andrewd-zededa:go-libzfs:andrewd-zfs-2.2.2 Plan is to submit a PR for that upstream. Don't know if @andrewd-zededa has already started the upstreaming. |
I think they usually are pretty responsive. We should link to that PR before we start to depend on a fork. Is the PR there? If not, can we see the diff here? |
To me it still makes sense to put openzfs into the build-tools-container; I am not sure about others but I use it every time for development of pillar. Also I would suggest to first merge this PR and only then #3804 because then we already have the new version of the pillar container pushed to the registry and avoid splitting this PR into two PRs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the team wants to include the zfs packages in the build container I can live with that. But it means an additional maintenance burden to update that when there is a new version of ZFS.
@andrewd-zededa can you start the discussion (maybe in the form of a github issue) for the upstream zfs repo and see if they will take your changes?
@eriknordmark yes I'll start the upstream discussion. The correct diff link of my fork which my PR uses is here: bicomsystems/go-libzfs@v0.4.0...andrewd-zededa:go-libzfs:andrewd-zfs-2.2.2-0.4.0 I originally intended to base my changes off their master branch but it seems its missing one or more changes which are in v0.4.0 (the one eve seems to use currently). So the module version linked in my PR here is a branch off of v0.4.0 at the moment. |
master is missing things in v0.4.0? That is strange. The change is pretty small (at least diffing to v0.4.0), so should get it in and have them engage. |
I have a starting point with questions on bicomsystems's repo here: bicomsystems/go-libzfs#51. When I hear back from them i'll move forward. |
I'll merge this now and we can see how soon we have the support in the upstream. |
Kernel update - [amd64-generic, amd64-generic, amd64-rt, arm64-nvidia, arm64-generic, arm64-generic]
eve-kernel-amd64-v5.10.186-generic
4d7866684f79: Update to OpenZFS 2.2.2
eve-kernel-amd64-v6.1.38-generic
820be3b1c356: Update to OpenZFS 2.2.2
eve-kernel-amd64-v6.1.38-rt
7148ced52fc1: Update to OpenZFS 2.2.2
eve-kernel-arm64-v5.10.104-nvidia
ab0df9cfc431: Update to OpenZFS 2.2.2
eve-kernel-arm64-v5.10.186-generic
438959506e33: Update to OpenZFS 2.2.2
eve-kernel-arm64-v6.1.38-generic
50c6e530bbd2: Update to OpenZFS 2.2.2
Remove modifications to now unused kernel module parameters.
Don't apply patches as they're now included in upstream.
Remove now unused patches under dom0-ztools
Patch go-libzfs for OpenZFS 2.2.2 support