Bug 1826439
| Summary: | Review Request: libvma - LD_PRELOAD-able library with standard BSD sockets API | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | igor.ivanov.va |
| Component: | Package Review | Assignee: | Honggang LI <honli> |
| Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | unspecified | ||
| Version: | rawhide | CC: | ahleihel, alexv, fedora, honli, igor.ivanov.va, mschmidt, package-review |
| Target Milestone: | --- | Flags: | honli:
fedora-review+
|
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | libvma-9.0.2-1.fc34,libvma-9.0.2-1.fc33 | Doc Type: | If docs needed, set a value |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2020-08-14 03:31:30 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
igor.ivanov.va
2020-04-21 17:19:30 UTC
>Group: System Environment/Libraries This tag is not used in Fedora and not in EPEL. Please remove those instead of hiding them behind %if. >BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX) >Prefix: %{_prefix} >... >%install >[ "${RPM_BUILD_ROOT}" != "/" -a -d ${RPM_BUILD_ROOT} ] && rm -rf ${RPM_BUILD_ROOT} >... >%clean The tags are not used in Fedora; the buildroot should not be deleted at start of %install. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections >ExclusiveArch: x86_64 ppc64le ppc64 aarch64 Add a comment explaining why this is needed. >%files >%{_libdir}/%{name}.so The un-versioned .so typically goes into -devel. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages Thank you for review. Spec is updated. I'm taking over this bug after exchanged email with Artur Iwicki. libvma was introduced in Fedora with bz1298665 . It was retired because not fix FTBFS bz1736055 in time. 1 %{!?configure_options: %global configure_options %{nil}}
2 %{!?use_extralib: %global use_extralib 0}
3
4 %global pmake %{__make} %{?_smp_mflags} %{?mflags} V=1
%make_build
5 %global use_systemd %(if ( test -d "%{_unitdir}" > /dev/null); then echo -n '1'; else echo -n '0'; fi)
always be true
6
7 Name: libvma
8 Version: 9.0.2
9 Release: 1%{?dist}
10 Summary: A library for boosting TCP and UDP traffic (over RDMA hardware)
11
12 License: GPLv2 or BSD
13 Url: https://github.com/Mellanox/%{name}
Please use full path https://github.com/Mellanox/libvma
14 Source0: %{url}/archive/%{version}/%{name}-%{version}.tar.gz
Again. Use https://github.com/Mellanox/libvma/archive/%{version}/%{name}-%{version}.tar.gz
Fedora upstream mointor tool will automaticlly try to build new package when new release
available in upstream. An working source URL is necessary.
15
16 # libvma currently supports only the following architectures
17 ExclusiveArch: x86_64 ppc64le ppc64 aarch64
18
19 BuildRequires: pkgconfig
20 BuildRequires: automake
21 BuildRequires: autoconf
22 BuildRequires: libtool
23 BuildRequires: gcc-c++
24 BuildRequires: libibverbs-devel
25 BuildRequires: librdmacm-devel
26 %if 0%{?rhel} >= 7 || 0%{?fedora} >= 24 || 0%{?suse_version} >= 1500
line 26 is always true for Fedora and RHEL release in today. Please remove anything
related to suse for Fedora spec file. So, please delete line 26 and line 28.
27 BuildRequires: libnl3-devel
28 %endif
29
30 %description
31 libvma is a LD_PRELOAD-able library that boosts performance of TCP and
32 UDP traffic. It allows application written over standard socket API to
33 handle fast path data traffic from user space over Ethernet and/or
34 Infiniband with full network stack bypass and get better throughput,
35 latency and packets/sec rate.
36 .
Unwanted dot in line 36, please delete it.
37 No application binary change is required for that.
38 libvma is supported by RDMA capable devices that support "verbs"
39 IBV_QPT_RAW_PACKET QP for Ethernet and/or IBV_QPT_UD QP for IPoIB.
40 .
Again. Unwanted dot in line 36, please delete it.
41 This package includes headers for building programs with libvma's interface
42 directly, as opposed to loading it dynamically with LD_PRELOAD.
43
44 %package devel
45 Summary: Header files and link required to develop with Libvma
46 Requires: %{name}%{?_isa} = %{version}-%{release}
47
48 %description devel
49 This package includes headers for building programs with libvma's
50 interfaces.
51
52 %package utils
53 Summary: Libvma utilities
54 Requires: %{name}%{?_isa} = %{version}-%{release}
55
56 %description utils
57 This package contains the tool vma_stats for collecting and
58 analyzing Libvma statistic.
59
60 %prep
61 %setup -q
62
63 %build
64 export revision=1
65 [ -f configure ] || ./autogen.sh
Maybe, we should remove the test of file configure . As automake, autoconf
and libtool are required as BuildRequires.
66
67 # Debug binary with extra output verbosity
68 %if "%{use_extralib}" == "1"
69 %configure --enable-opt-log=none \
70 %{?configure_options}
71 %{pmake}
please replace %{pmake} with %make_build
72 cp -f src/vma/.libs/%{name}.so %{name}-debug.so
It is better to replace 'cp' with 'install'. And it seems duplicated of line 95.
73 %{pmake} clean
please replace %{pmake} with %make_build
74 %endif
75
76 # Primary installation set
77 %configure --docdir=%{_docdir}/%{name}-%{version} \
^^^^^^^^^^
Please don't install doc in versioned dir. See https://fedoraproject.org//wiki/Changes/UnversionedDocdirs
78 %{?configure_options}
79 %{pmake}
please replace %{pmake} with %make_build
80
81 %install
82 mkdir -p $RPM_BUILD_ROOT%{_includedir}/mellanox
83 mkdir -p $RPM_BUILD_ROOT%{_sysconfdir}
84 mkdir -p $RPM_BUILD_ROOT%{_libdir}
line 83 and 84 is redundance, should be removed.
85
86 %{pmake} DESTDIR=${RPM_BUILD_ROOT} install
87
88 rm -f $RPM_BUILD_ROOT%{_libdir}/*.la
find $RPM_BUILD_ROOT -f -name '*.la' -delete
89
90 install -m 644 src/vma/vma_extra.h $RPM_BUILD_ROOT/%{_includedir}/mellanox/vma_extra.h
91 install -m 644 src/vma/util/libvma.conf $RPM_BUILD_ROOT/%{_sysconfdir}/
92 install -s -m 755 src/stats/vma_stats $RPM_BUILD_ROOT/%{_bindir}/vma_stats
93 install -s -m 755 tools/daemon/vmad $RPM_BUILD_ROOT/%{_sbindir}/vmad
94 %if "%{use_extralib}" == "1"
95 install -m 755 ./%{name}-debug.so $RPM_BUILD_ROOT/%{_libdir}/%{name}-debug.so
96 %endif
97 %if "%{use_systemd}" == "1"
98 install -D -m 644 contrib/scripts/vma.service $RPM_BUILD_ROOT/%{_unitdir}/vma.service
99 install -m 755 contrib/scripts/vma.init $RPM_BUILD_ROOT/%{_sbindir}/vma
100 %else
101 install -m 755 contrib/scripts/vma.init $RPM_BUILD_ROOT/%{_sysconfdir}/init.d/vma
102 %endif
please delete line 97, 100 - 102, as use_systemd always true for Fedora.
103
104 %post
105 if [ `grep memlock /etc/security/limits.conf |grep unlimited |wc -l` -le 0 ]; then
The test in line 105 is not robust. It is really easy to foo it. Should we replace it with 'ulimit -m' ?
106 echo "* - memlock unlimited" >> /etc/security/limits.conf
107 echo "* soft memlock unlimited" >> /etc/security/limits.conf
108 echo "* hard memlock unlimited" >> /etc/security/limits.conf
109 echo "- Changing max locked memory to unlimited (in /etc/security/limits.conf)"
110 echo " Please log out from the shell and login again in order to update this change "
111 echo " Read more about this topic in the VMA's User Manual"
112 fi
113 /sbin/ldconfig
Please see https://fedoraproject.org/wiki/Changes/Removing_ldconfig_scriptlets
114
115 # Package setup, not upgrade
Please don't include 'comment' in scriptlets. They are not processed as you expected.
It will trigger error message.
116 if [ $1 = 1 ]; then
117 if type systemctl >/dev/null 2>&1; then
118 systemctl --no-reload enable vma.service >/dev/null 2>&1 || true
119 elif [ -e /sbin/chkconfig ]; then
120 /sbin/chkconfig --add vma
121 elif [ -e /usr/sbin/update-rc.d ]; then
122 /usr/sbin/update-rc.d vma defaults
123 else
124 %{_libdir}/lsb/install_initd %{_sysconfdir}/init.d/vma
125 fi
126 fi
line 114 - 126 should be re-wrote. see https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_scriptlets
127
128 %preun
129 # Package removal, not upgrade
130 if [ $1 = 0 ]; then
131 if type systemctl >/dev/null 2>&1; then
132 systemctl --no-reload disable vma.service >/dev/null 2>&1 || true
133 systemctl stop vma.service || true
134 elif [ -e /sbin/chkconfig ]; then
135 %{_sysconfdir}/init.d/vma stop
136 /sbin/chkconfig --del vma
137 elif [ -e /usr/sbin/update-rc.d ]; then
138 %{_sysconfdir}/init.d/vma stop
139 /usr/sbin/update-rc.d -f vma remove
140 else
141 %{_sysconfdir}/init.d/vma stop
142 %{_libdir}/lsb/remove_initd %{_sysconfdir}/init.d/vma
143 fi
144 fi
145
same as line 114 - 126.
146 %postun
147 # Package upgrade
148 /sbin/ldconfig
149 if type systemctl >/dev/null 2>&1; then
150 systemctl --system daemon-reload >/dev/null 2>&1 || true
151 fi
same as line 114 - 126.
152
153 %files
154 %{_libdir}/%{name}.so*
155 %doc %{_docdir}/%{name}-%{version}/README.txt
156 %doc %{_docdir}/%{name}-%{version}/journal.txt
157 %doc %{_docdir}/%{name}-%{version}/VMA_VERSION
No versioned doc.
158 %config(noreplace) %{_sysconfdir}/libvma.conf
159 %config(noreplace) %{_sysconfdir}/security/limits.d/30-libvma-limits.conf
160 %{_sbindir}/vmad
161 %if "%{use_systemd}" == "1"
162 %{_prefix}/lib/systemd/system/vma.service
163 %{_sbindir}/vma
164 %else
165 %{_sysconfdir}/init.d/vma
166 %endif
use_systemd always true.
167 %if 0%{?rhel} >= 7 || 0%{?fedora} >= 24 || 0%{?suse_version} >= 1500
168 %license COPYING
169 %endif
delete line 167, 169.
170
171 %files devel
172 %{_includedir}/mellanox/vma_extra.h
173 %if "%{use_extralib}" == "1"
174 %{_libdir}/%{name}-debug.so
175 %endif
176
177 %files utils
178 %{_bindir}/vma_stats
Please create a man page for executable 'vma_stats'.
Thank you for review libvma.spec has been updated accordingly. There are a couple of notes: 1. After applying changes to meet https://fedoraproject.org/wiki/Changes/Removing_ldconfig_scriptlets requirements rpmlint reports error: libvma.x86_64: E: postin-without-ldconfig /usr/lib64/libvma.so.9.0.2 This package contains a library and its %post scriptlet doesn't call ldconfig. libvma.x86_64: E: postun-without-ldconfig /usr/lib64/libvma.so.9.0.2 This package contains a library and its %postun doesn't call ldconfig. I am using rpmlint version 1.10 on fc30 2. I would like to avoid putting man page for executable 'vma_stats' at the moment unless it must be done. (In reply to igor.ivanov.va from comment #5) > Thank you for review > > libvma.spec has been updated accordingly. > > There are a couple of notes: > 1. After applying changes to meet > https://fedoraproject.org/wiki/Changes/Removing_ldconfig_scriptlets > requirements > rpmlint reports error: > libvma.x86_64: E: postin-without-ldconfig /usr/lib64/libvma.so.9.0.2 > This package contains a library and its %post scriptlet doesn't call > ldconfig. > > libvma.x86_64: E: postun-without-ldconfig /usr/lib64/libvma.so.9.0.2 > This package contains a library and its %postun doesn't call ldconfig. > That's OK, as it is a rpmlint bug. > I am using rpmlint version 1.10 on fc30 > > 2. I would like to avoid putting man page for executable 'vma_stats' at the > moment unless it must be done. Yes, it is necessary. Please try search with key words " Man page scan results" in bugzilla. It gives 441 bugs for me. That means 441 bugs had been opened to handle manpage like this. https://pagure.io/ManualPageScan/ $ rpm -qpl libvma*rpm | grep bin /usr/sbin/vma /usr/sbin/vmad /usr/bin/vma_stats /usr/sbin/vma is a bash script. It is OK to ignore manpage for it. But vmad and vma_stats are executables, we should provide manpage for them. $ cat -n libvma.spec
.....
4 %{!?make_build: %global make_build %{__make} %{?_smp_mflags} %{?mflags} V=1}
It was my bad. I should told you to delete line 4.
5 %{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}-%{version}}
Why you think line 5 is necessary? Can we just delete it too?
40 This package includes headers for building programs with libvma's interface
41 directly, as opposed to loading it dynamically with LD_PRELOAD.
Line 40 and 41 are in the base sub-package libvma's %description section. The base sub-package
dose not include any header. It seems line 40 and 41 belongs to sub-package libvma-devel's
%description section. Is it right?
52 Summary: Libvma utilities
57 analyzing Libvma statistic.
Please replace "Libvma" with "libvma" for line 52 and 57.
68 %configure --docdir=%{_pkgdocdir} \
^^^^^^^^^^^^^^^^^^^^^^
"--docdir=%{_pkgdocdir}" is unnecessary. Should be removed?
85 %{?ldconfig}
92 %{?ldconfig}
Why you need line 85 and 92?
105 %license COPYING
Should append "LICENSE" in line 105.
$ tar -tf libvma-9.0.2.tar.gz | grep -i license
libvma-9.0.2/LICENSE
Thanks
Functional smoke test verified libvma works as expected. Performance significantly improved.
+------------------+---------------+
|Socket over IPoIB |6.97 Gb/s |
+------------------+---------------+
|libvma |24.4 Gb/s |
+------------------+---------------+
[root@rdma-dev-21 ~]$ ibstat
CA 'ibp130s0f1'
CA type: MT4115
Number of ports: 1
Firmware version: 12.23.1020
Hardware version: 0
Node GUID: 0x248a07030049d4f1
System image GUID: 0x248a07030049d4f0
Port 1:
State: Active
Physical state: LinkUp
Rate: 56
Base lid: 32
LMC: 0
SM lid: 1
Capability mask: 0x2659e848
Port GUID: 0x248a07030049d4f1
Link layer: InfiniBand
CA 'rocep4s0'
CA type: MT4115
Number of ports: 1
Firmware version: 12.23.1020
Hardware version: 0
Node GUID: 0x248a0703004bf094
System image GUID: 0x248a0703004bf094
Port 1:
State: Active
Physical state: LinkUp
Rate: 100
Base lid: 0
LMC: 0
SM lid: 0
Capability mask: 0x00010000
Port GUID: 0x268a07fffe4bf094
Link layer: Ethernet
CA 'ibp130s0f0'
CA type: MT4115
Number of ports: 1
Firmware version: 12.23.1020
Hardware version: 0
Node GUID: 0x248a07030049d4f0
System image GUID: 0x248a07030049d4f0
Port 1:
State: Active
Physical state: LinkUp
Rate: 56
Base lid: 15
LMC: 0
SM lid: 13
Capability mask: 0x2659e848
Port GUID: 0x248a07030049d4f0
Link layer: InfiniBand
[root@rdma-dev-21 ~]$ ip addr show mlx5_ib0
7: mlx5_ib0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc mq state UP group default qlen 256
link/infiniband 00:00:0e:27:fe:80:00:00:00:00:00:00:24:8a:07:03:00:49:d4:f0 brd 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
inet 172.31.0.121/24 brd 172.31.0.255 scope global dynamic noprefixroute mlx5_ib0
valid_lft 2861sec preferred_lft 2861sec
inet6 fe80::268a:703:49:d4f0/64 scope link noprefixroute
valid_lft forever preferred_lft forever
[root@rdma-dev-21 ~]$ iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 128 KByte (default)
------------------------------------------------------------
[ 4] local 172.31.0.121 port 5001 connected with 172.31.0.122 port 58464
[ ID] Interval Transfer Bandwidth
[ 4] 0.0-10.0 sec 8.12 GBytes 6.97 Gbits/sec
[root@rdma-dev-22 ~]$ iperf -c 172.31.0.121
------------------------------------------------------------
Client connecting to 172.31.0.121, TCP port 5001
TCP window size: 612 KByte (default)
------------------------------------------------------------
[ 3] local 172.31.0.122 port 58464 connected with 172.31.0.121 port 5001
[ ID] Interval Transfer Bandwidth
[ 3] 0.0-10.0 sec 8.12 GBytes 6.98 Gbits/sec
[root@rdma-dev-21 ~]$ VMA_SPEC=latency LD_PRELOAD=libvma.so iperf -s
VMA INFO: ---------------------------------------------------------------------------
VMA INFO: VMA_VERSION: 9.0.2-0 Release built on Apr 17 2020 00:00:00
VMA INFO: Cmd Line: iperf -s
VMA INFO: ---------------------------------------------------------------------------
VMA INFO: VMA Spec Latency [VMA_SPEC]
VMA INFO: Log Level INFO [VMA_TRACELEVEL]
VMA INFO: Ring On Device Memory TX 16384 [VMA_RING_DEV_MEM_TX]
VMA INFO: Tx QP WRE 256 [VMA_TX_WRE]
VMA INFO: Tx QP WRE Batching 4 [VMA_TX_WRE_BATCHING]
VMA INFO: Rx QP WRE 256 [VMA_RX_WRE]
VMA INFO: Rx QP WRE Batching 4 [VMA_RX_WRE_BATCHING]
VMA INFO: Rx Poll Loops -1 [VMA_RX_POLL]
VMA INFO: Rx Prefetch Bytes Before Poll 256 [VMA_RX_PREFETCH_BYTES_BEFORE_POLL]
VMA INFO: GRO max streams 0 [VMA_GRO_STREAMS_MAX]
VMA INFO: Select Poll (usec) -1 [VMA_SELECT_POLL]
VMA INFO: Select Poll OS Force Enabled [VMA_SELECT_POLL_OS_FORCE]
VMA INFO: Select Poll OS Ratio 1 [VMA_SELECT_POLL_OS_RATIO]
VMA INFO: Select Skip OS 1 [VMA_SELECT_SKIP_OS]
VMA INFO: CQ Drain Interval (msec) 100 [VMA_PROGRESS_ENGINE_INTERVAL]
VMA INFO: CQ Interrupts Moderation Disabled [VMA_CQ_MODERATION_ENABLE]
VMA INFO: CQ AIM Max Count 128 [VMA_CQ_AIM_MAX_COUNT]
VMA INFO: CQ Adaptive Moderation Disabled [VMA_CQ_AIM_INTERVAL_MSEC]
VMA INFO: CQ Keeps QP Full Disabled [VMA_CQ_KEEP_QP_FULL]
VMA INFO: TCP nodelay 1 [VMA_TCP_NODELAY]
VMA INFO: Avoid sys-calls on tcp fd Enabled [VMA_AVOID_SYS_CALLS_ON_TCP_FD]
VMA INFO: Internal Thread Affinity 0 [VMA_INTERNAL_THREAD_AFFINITY]
VMA INFO: Thread mode Single [VMA_THREAD_MODE]
VMA INFO: ---------------------------------------------------------------------------
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 128 KByte (default)
------------------------------------------------------------
[ 53] local 172.31.0.121 port 5001 connected with 172.31.0.122 port 59351
[ ID] Interval Transfer Bandwidth
[ 53] 0.0- 9.7 sec 28.4 GBytes 25.1 Gbits/sec
[root@rdma-dev-22 ~]$ iperf -c 172.31.0.121
------------------------------------------------------------
Client connecting to 172.31.0.121, TCP port 5001
TCP window size: 612 KByte (default)
------------------------------------------------------------
[ 3] local 172.31.0.122 port 58464 connected with 172.31.0.121 port 5001
[ ID] Interval Transfer Bandwidth
[ 3] 0.0-10.0 sec 8.12 GBytes 6.98 Gbits/sec
[root@rdma-dev-22 ~]$ VMA_SPEC=latency LD_PRELOAD=libvma.so iperf -c 172.31.0.121
VMA INFO: ---------------------------------------------------------------------------
VMA INFO: VMA_VERSION: 9.0.2-0 Release built on Apr 17 2020 00:00:00
VMA INFO: Cmd Line: iperf -c 172.31.0.121
VMA INFO: ---------------------------------------------------------------------------
VMA INFO: VMA Spec Latency [VMA_SPEC]
VMA INFO: Log Level INFO [VMA_TRACELEVEL]
VMA INFO: Ring On Device Memory TX 16384 [VMA_RING_DEV_MEM_TX]
VMA INFO: Tx QP WRE 256 [VMA_TX_WRE]
VMA INFO: Tx QP WRE Batching 4 [VMA_TX_WRE_BATCHING]
VMA INFO: Rx QP WRE 256 [VMA_RX_WRE]
VMA INFO: Rx QP WRE Batching 4 [VMA_RX_WRE_BATCHING]
VMA INFO: Rx Poll Loops -1 [VMA_RX_POLL]
VMA INFO: Rx Prefetch Bytes Before Poll 256 [VMA_RX_PREFETCH_BYTES_BEFORE_POLL]
VMA INFO: GRO max streams 0 [VMA_GRO_STREAMS_MAX]
VMA INFO: Select Poll (usec) -1 [VMA_SELECT_POLL]
VMA INFO: Select Poll OS Force Enabled [VMA_SELECT_POLL_OS_FORCE]
VMA INFO: Select Poll OS Ratio 1 [VMA_SELECT_POLL_OS_RATIO]
VMA INFO: Select Skip OS 1 [VMA_SELECT_SKIP_OS]
VMA INFO: CQ Drain Interval (msec) 100 [VMA_PROGRESS_ENGINE_INTERVAL]
VMA INFO: CQ Interrupts Moderation Disabled [VMA_CQ_MODERATION_ENABLE]
VMA INFO: CQ AIM Max Count 128 [VMA_CQ_AIM_MAX_COUNT]
VMA INFO: CQ Adaptive Moderation Disabled [VMA_CQ_AIM_INTERVAL_MSEC]
VMA INFO: CQ Keeps QP Full Disabled [VMA_CQ_KEEP_QP_FULL]
VMA INFO: TCP nodelay 1 [VMA_TCP_NODELAY]
VMA INFO: Avoid sys-calls on tcp fd Enabled [VMA_AVOID_SYS_CALLS_ON_TCP_FD]
VMA INFO: Internal Thread Affinity 0 [VMA_INTERNAL_THREAD_AFFINITY]
VMA INFO: Thread mode Single [VMA_THREAD_MODE]
VMA INFO: ---------------------------------------------------------------------------
------------------------------------------------------------
Client connecting to 172.31.0.121, TCP port 5001
TCP window size: 0.00 Byte (default)
------------------------------------------------------------
[ 25] local 172.31.0.122 port 59351 connected with 172.31.0.121 port 5001
[ ID] Interval Transfer Bandwidth
[ 25] 0.0-10.0 sec 28.4 GBytes 24.4 Gbits/sec
1 Package Review
2 ==============
3
4 Legend:
5 [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
6 [ ] = Manual review needed
7
8
9 Issues:
10 =======
11 - Development (unversioned) .so files in -devel subpackage, if present.
12 Note: Unversioned so-files directly in %_libdir.
13 See: https://docs.fedoraproject.org/en-US/packaging-
14 guidelines/#_devel_packages
That's OK, as the libvma-9.0.2-1.fc33.x86_64.rpm/usr/lib64/libvma.so file is used as PRE_LOAD.
So, it should be included in the main sub-package.
15 - Package does not use a name that already exists.
16 Note: A package with this name already exists. Please check
17 https://src.fedoraproject.org/rpms/libvma
18 See: https://docs.fedoraproject.org/en-US/packaging-
19 guidelines/Naming/#_conflicting_package_names
It's OK, as we are import libvma again for fedora. See https://bugzilla.redhat.com/show_bug.cgi?id=1826439#c3 .
20
21
22 ===== MUST items =====
23
24 C/C++:
25 [ ]: Package does not contain kernel modules.
PASS
26 [ ]: Package contains no static executables.
PASS
27 [x]: If your application is a C or C++ application you must list a
28 BuildRequires against gcc, gcc-c++ or clang.
29 [x]: Header files in -devel subpackage, if present.
30 [x]: ldconfig not called in %post and %postun for Fedora 28 and later.
31 [x]: Package does not contain any libtool archives (.la)
32 [x]: Rpath absent or only used for internal libs.
33
34 Generic:
35 [ ]: Package is licensed with an open-source compatible license and meets
36 other legal requirements as defined in the legal section of Packaging
37 Guidelines.
PASS
38 [ ]: License field in the package spec file matches the actual license.
39 Note: Checking patched sources after %prep for licenses. Licenses
40 found: "Unknown or generated", "BSD (unspecified) GPL (v2)", "BSD
41 3-clause "New" or "Revised" License", "GPL (v3 or later)", "BSD
42 2-clause "Simplified" License GPL (v2)", "Expat License". 113 files
43 have unknown license. Detailed output of licensecheck in
44 /home/honli/fedora/libvma/1826439-libvma/licensecheck.txt
PASS
45 [ ]: License file installed when any subpackage combination is installed.
PASS.
46 [ ]: If the package is under multiple licenses, the licensing breakdown
47 must be documented in the spec.
PASS
48 [ ]: Package requires other packages for directories it uses.
PASS
49 Note: No known owner of /usr/share/doc/libvma, /usr/include/mellanox
Please fix this. /usr/share/doc/libvma should be owned by sub-package "libvma".
/usr/include/mellanox should be owned by sub-package "libvma-devel".
50 [ ]: Package must own all directories that it creates.
51 Note: Directories without known owners: /etc/security/limits.d,
52 /etc/security, /usr/include/mellanox, /usr/share/doc/libvma
PASS. The first two directories are co-owned. The last two are duplicated of line 49.
53 [ ]: %build honors applicable compiler flags or justifies otherwise.
PASS
54 [ ]: Package contains no bundled libraries without FPC exception.
PASS
55 [ ]: Changelog in prescribed format.
PASS
56 [ ]: Sources contain only permissible code or content.
PASS
57 [ ]: Package contains desktop file if it is a GUI application.
PASS. Not GUI application.
58 [ ]: Development files must be in a -devel package
PASS
59 [ ]: Package uses nothing in %doc for runtime.
PASS
60 [ ]: Package consistently uses macros (instead of hard-coded directory
61 names).
Except the doc dir. Others looks good.
62 [ ]: Package is named according to the Package Naming Guidelines.
PASS
63 [ ]: Package does not generate any conflict.
PASS
64 [ ]: Package obeys FHS, except libexecdir and /usr/target.
PASS
65 [ ]: If the package is a rename of another package, proper Obsoletes and
66 Provides are present.
PASS
67 [ ]: Requires correct, justified where necessary.
PASS
68 [ ]: Spec file is legible and written in American English.
PASS
69 [ ]: Package contains systemd file(s) if in need.
PASS. Yes, it does.
70 [ ]: Useful -debuginfo package or justification otherwise.
PASS
71 [ ]: Package is not known to require an ExcludeArch tag.
PASS, see inline comments in spec file.
72 [ ]: Large documentation must go in a -doc subpackage. Large could be size
73 (~1MB) or number of files.
74 Note: Documentation size is 184320 bytes in 3 files.
PASS
75 [ ]: Package complies to the Packaging Guidelines
PASS
76 [x]: Package successfully compiles and builds into binary rpms on at least
77 one supported primary architecture.
78 [x]: Package installs properly.
79 [x]: Rpmlint is run on all rpms the build produces.
80 Note: There are rpmlint messages (see attachment).
81 [x]: If (and only if) the source package includes the text of the
82 license(s) in its own file, then that file, containing the text of the
83 license(s) for the package is included in %license.
84 [x]: Package does not own files or directories owned by other packages.
85 [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
86 [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
87 beginning of %install.
88 [x]: %config files are marked noreplace or the reason is justified.
89 [x]: Macros in Summary, %description expandable at SRPM build time.
90 [x]: Dist tag is present.
91 [x]: Package does not contain duplicates in %files.
92 [x]: Permissions on files are set properly.
93 [x]: Package must not depend on deprecated() packages.
94 [x]: Package use %makeinstall only when make install DESTDIR=... doesn't
95 work.
96 [x]: Package is named using only allowed ASCII characters.
97 [x]: No %config files under /usr.
98 [x]: Package is not relocatable.
99 [x]: Sources used to build the package match the upstream source, as
100 provided in the spec URL.
101 [x]: Spec file name must match the spec package %{name}, in the format
102 %{name}.spec.
103 [x]: systemd_post is invoked in %post, systemd_preun in %preun, and
104 systemd_postun in %postun for Systemd service files.
105 Note: Systemd service file(s) in libvma
106 [x]: File names are valid UTF-8.
107 [x]: Packages must not store files under /srv, /opt or /usr/local
108
109 ===== SHOULD items =====
110
111 Generic:
112 [ ]: If the source package does not include license text(s) as a separate
113 file from upstream, the packager SHOULD query upstream to include it.
PASS
114 [ ]: Final provides and requires are sane (see attachments).
PASS
115 [ ]: Package functions as described.
PASS. See https://bugzilla.redhat.com/show_bug.cgi?id=1826439#c8
116 [ ]: Latest version is packaged.
PASS
117 [ ]: Package does not include license text files separate from upstream.
PASS
118 [ ]: Sources are verified with gpgverify first in %prep if upstream
119 publishes signatures.
120 Note: gpgverify is not used.
PASS. NO GPG.
121 [ ]: Description and summary sections in the package spec file contains
122 translations for supported Non-English languages, if available.
PASS
123 [ ]: %check is present and all tests pass.
PASS
124 [ ]: Packages should try to preserve timestamps of original installed
125 files.
PASS
126 [x]: Reviewer should test that the package builds in mock.
127 [x]: Buildroot is not present
128 [x]: Package has no %clean section with rm -rf %{buildroot} (or
129 $RPM_BUILD_ROOT)
130 [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
131 [x]: Fully versioned dependency in subpackages if applicable.
132 [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
133 [x]: Sources can be downloaded from URI in Source: tag
134 [x]: SourceX is a working URL.
135 [x]: Package should compile and build into binary rpms on all supported
136 architectures.
137 [x]: Spec use %global instead of %define unless justified.
138
139 ===== EXTRA items =====
140
141 Generic:
142 [!]: Package should not use obsolete m4 macros
143 Note: Some obsoleted macros found, see the attachment.
144 See: https://fedorahosted.org/FedoraReview/wiki/AutoTools
Please fix this.
145 [x]: Rpmlint is run on debuginfo package(s).
146 Note: No rpmlint messages.
147 [x]: Rpmlint is run on all installed packages.
148 Note: There are rpmlint messages (see attachment).
149 [x]: Large data in /usr/share should live in a noarch subpackage if package
150 is arched.
151 [x]: Spec file according to URL is the same as in SRPM.
152
153
154 Rpmlint
155 -------
156 Checking: libvma-9.0.2-1.fc33.x86_64.rpm
157 libvma-devel-9.0.2-1.fc33.x86_64.rpm
158 libvma-utils-9.0.2-1.fc33.x86_64.rpm
159 libvma-debuginfo-9.0.2-1.fc33.x86_64.rpm
160 libvma-debugsource-9.0.2-1.fc33.x86_64.rpm
161 libvma-9.0.2-1.fc33.src.rpm
162 libvma.x86_64: W: shared-lib-calls-exit /usr/lib64/libvma.so.9.0.2 exit.5
163 libvma.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libvma.so
164 libvma.x86_64: E: postin-without-ldconfig /usr/lib64/libvma.so.9.0.2
165 libvma.x86_64: E: postun-without-ldconfig /usr/lib64/libvma.so.9.0.2
line 164 and 165 are false positive.
166 libvma.x86_64: W: no-manual-page-for-binary vma
167 libvma.x86_64: W: no-manual-page-for-binary vmad
should be fixed.
168 libvma-devel.x86_64: W: no-documentation
169 libvma-utils.x86_64: W: spelling-error %description -l en_US vma -> ma, via, v ma
170 libvma-utils.x86_64: W: no-documentation
171 libvma-utils.x86_64: W: no-manual-page-for-binary vma_stats
should be fixed.
172 6 packages and 0 specfiles checked; 2 errors, 8 warnings.
173
174
175
176
177 Rpmlint (debuginfo)
178 -------------------
179 Checking: libvma-debuginfo-9.0.2-1.fc33.x86_64.rpm
180 1 packages and 0 specfiles checked; 0 errors, 0 warnings.
181
182
183
184
185
186 Rpmlint (installed packages)
187 ----------------------------
188 libvma-debugsource.x86_64: W: invalid-url URL: https://github.com/Mellanox/libvma <urlopen error [Errno -2] Name or service not known>
189 libvma.x86_64: W: invalid-url URL: https://github.com/Mellanox/libvma <urlopen error [Errno -2] Name or service not known>
190 libvma.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libvma.so.9.0.2 /lib64/librt.so.1
191 libvma.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libvma.so.9.0.2 /lib64/libm.so.6
192 libvma.x86_64: W: shared-lib-calls-exit /usr/lib64/libvma.so.9.0.2 exit.5
193 libvma.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libvma.so
194 libvma.x86_64: E: postin-without-ldconfig /usr/lib64/libvma.so.9.0.2
195 libvma.x86_64: E: postun-without-ldconfig /usr/lib64/libvma.so.9.0.2
196 libvma.x86_64: W: no-manual-page-for-binary vma
197 libvma.x86_64: W: no-manual-page-for-binary vmad
198 libvma-debuginfo.x86_64: W: invalid-url URL: https://github.com/Mellanox/libvma <urlopen error [Errno -2] Name or service not known>
199 libvma-utils.x86_64: W: spelling-error %description -l en_US vma -> ma, via, v ma
200 libvma-utils.x86_64: W: invalid-url URL: https://github.com/Mellanox/libvma <urlopen error [Errno -2] Name or service not known>
201 libvma-utils.x86_64: W: no-documentation
202 libvma-utils.x86_64: W: no-manual-page-for-binary vma_stats
203 libvma-devel.x86_64: W: invalid-url URL: https://github.com/Mellanox/libvma <urlopen error [Errno -2] Name or service not known>
204 libvma-devel.x86_64: W: no-documentation
205 5 packages and 0 specfiles checked; 2 errors, 15 warnings.
duplicated error report.
206
207
208
209 Unversioned so-files
210 --------------------
211 libvma: /usr/lib64/libvma.so
212
213 Source checksums
214 ----------------
215 https://github.com/Mellanox/libvma/archive/9.0.2/libvma-9.0.2.tar.gz :
216 CHECKSUM(SHA256) this package : 1f273c309f553bd479da229a39b93d53fcf3fdda9b8eae2df973f6b8d02aa164
217 CHECKSUM(SHA256) upstream package : 1f273c309f553bd479da229a39b93d53fcf3fdda9b8eae2df973f6b8d02aa164
218
219
220 Requires
221 --------
222 libvma (rpmlib, GLIBC filtered):
223 /bin/sh
224 /usr/bin/bash
225 config(libvma)
226 ld-linux-x86-64.so.2()(64bit)
227 libc.so.6()(64bit)
228 libdl.so.2()(64bit)
229 libgcc_s.so.1()(64bit)
230 libgcc_s.so.1(GCC_3.0)(64bit)
231 libibverbs.so.1()(64bit)
232 libibverbs.so.1(IBVERBS_1.0)(64bit)
233 libibverbs.so.1(IBVERBS_1.1)(64bit)
234 libibverbs.so.1(IBVERBS_1.8)(64bit)
235 libm.so.6()(64bit)
236 libmlx5.so.1()(64bit)
237 libmlx5.so.1(MLX5_1.2)(64bit)
238 libmlx5.so.1(MLX5_1.4)(64bit)
239 libnl-3.so.200()(64bit)
240 libnl-3.so.200(libnl_3)(64bit)
241 libnl-route-3.so.200()(64bit)
242 libnl-route-3.so.200(libnl_3)(64bit)
243 libpthread.so.0()(64bit)
244 librdmacm.so.1()(64bit)
245 librdmacm.so.1(RDMACM_1.0)(64bit)
246 librt.so.1()(64bit)
247 libstdc++.so.6()(64bit)
248 libstdc++.so.6(CXXABI_1.3)(64bit)
249 libstdc++.so.6(CXXABI_1.3.8)(64bit)
250 libstdc++.so.6(CXXABI_1.3.9)(64bit)
251 libvma.so.9()(64bit)
252 rtld(GNU_HASH)
253
254 libvma-devel (rpmlib, GLIBC filtered):
255 libvma(x86-64)
256
257 libvma-utils (rpmlib, GLIBC filtered):
258 libc.so.6()(64bit)
259 libgcc_s.so.1()(64bit)
260 libgcc_s.so.1(GCC_3.0)(64bit)
261 libpthread.so.0()(64bit)
262 libstdc++.so.6()(64bit)
263 libstdc++.so.6(CXXABI_1.3)(64bit)
264 libstdc++.so.6(CXXABI_1.3.9)(64bit)
265 libvma(x86-64)
266 rtld(GNU_HASH)
267
268 libvma-debuginfo (rpmlib, GLIBC filtered):
269
270 libvma-debugsource (rpmlib, GLIBC filtered):
271
272
273
274 Provides
275 --------
276 libvma:
277 config(libvma)
278 libvma
279 libvma(x86-64)
280 libvma.so.9()(64bit)
281
282 libvma-devel:
283 libvma-devel
284 libvma-devel(x86-64)
285
286 libvma-utils:
287 libvma-utils
288 libvma-utils(x86-64)
289
290 libvma-debuginfo:
291 debuginfo(build-id)
292 libvma-debuginfo
293 libvma-debuginfo(x86-64)
294
295 libvma-debugsource:
296 libvma-debugsource
297 libvma-debugsource(x86-64)
298
299
300
301 AutoTools: Obsoleted m4s found
302 ------------------------------
303 AC_PROG_LIBTOOL found in: libvma-9.0.2/configure.ac:107
Please fix this.
304
305
306 Generated by fedora-review 0.7.5 (5fa5b7e) last change: 2020-02-16
307 Command line :/usr/bin/fedora-review -b 1826439
308 Buildroot used: fedora-rawhide-x86_64
309 Active plugins: Generic, C/C++, Shell-api
310 Disabled plugins: fonts, SugarActivity, Haskell, Ocaml, PHP, Perl, Java, R, Python, Ruby
311 Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
Thanks
I added patch with man pages (https://raw.githubusercontent.com/igor-ivanov/libvma/pr/fedora-package/fedora/0001-issue-928161-Add-man-pages.patch) and updated source rpm with spec. Thanks, Igor (In reply to igor.ivanov.va from comment #10) > I added patch with man pages > (https://raw.githubusercontent.com/igor-ivanov/libvma/pr/fedora-package/ > fedora/0001-issue-928161-Add-man-pages.patch) I checked libvma git repo. I did not found this patch. The patch should be applied in upstream before apply it for fedora. Please see https://docs.fedoraproject.org/en-US/packaging-guidelines/#_patch_guidelines . Ok, I will update you after getting pacth into master. (In reply to igor.ivanov.va from comment #10) > I added patch with man pages > (https://raw.githubusercontent.com/igor-ivanov/libvma/pr/fedora-package/ > fedora/0001-issue-928161-Add-man-pages.patch) > and > updated source rpm with spec. The updated spec file looks good to me. I will set the fedora-review+ flag, after the patch get into upstream. Thanks The patch is a part of master as https://github.com/Mellanox/libvma/commit/b9aabc058b0524aadff9c756abaf57b660cec63b Thanks, Igor Please tell me your FSA account. I searched 'igor.ivanov.va' in https://src.fedoraproject.org/users , but did not get a match. May FCA : iivanov @mschmidt, could you please add iivanov into packager group? Michal, Honggang what is next step for getting one into fc33? Thanks, Igor (In reply to igor.ivanov.va from comment #18) > Michal, Honggang > what is next step for getting one into fc33? https://fedoraproject.org/wiki/Orphaned_package_that_need_new_maintainers Thanks Added https://pagure.io/releng/issue/9509 Thanks, Igor Hi Igor, I have not added you to the packagers group yet. I have some questions and comments about the package. libvma.spec: > BuildRequires: libibverbs-devel > BuildRequires: rdma-core-devel These days, libibverbs-devel is provided by rdma-core-devel. > BuildRequires: libnl3-devel Your config/m4/nl.m4 uses PKG_CHECK_MODULES, so you should BR pkgconfig(libnl-route-3.0) instead. https://docs.fedoraproject.org/en-US/packaging-guidelines/PkgConfigBuildRequires/ > %build > export revision=%{use_rel} This seems unnecessary. I did not find anything referencing the "revision" environment variable in your build system. > %install > %{make_build} DESTDIR=${RPM_BUILD_ROOT} install You should use %{make_install} instead. > %files > %{_libdir}/%{name}.so* So the package contains something like this: libvma.so -> libvma.so.9.1.0 libvma.so.9 -> libvma.so.9.1.0 libvma.so.9.1.0 Since libvma.so is to be used only for LD_PRELOAD and no program should be linked against it (and have DT_NEEDED for it),: 1. is there a point in versioning the library? 2. does it have to be installed in the linker's default path? Would it make more sense to have it as %{_libdir}/libvma/libvma.so ? Do you actually use configure_options for anything? I don't think it's common for Fedora packages to have macros like that "just in case". use_rel - This is fascinating. The macro not only determines the value of the Release tag, it also influences the build where its value gets used for version compatibility checks and encoded in version strings. To me the mechanism seems needlessly complicated and I fail to see the benefit of it. Also, are you aware that sometimes automatic scripts (e.g. release engineering mass rebuilds) bump the Release fields in Fedora spec files? Normally in RPM, Version is the upstream version and Release serves packaging needs. 30-libvma-limits.conf: > # Default limits that are needed for proper work of libvma > # Read more about this topic in the VMA's User Manual Can you point me to where in the manual is this discussed? > * - memlock unlimited > * soft memlock unlimited > * hard memlock unlimited Does having the package installed give every user on the system the permission to mlock unlimited amounts of RAM? Is that really necessary? Could it be at least limited to a user group? vma.service: > [Unit] > Description=VMA Daemon. Version: 9.1.0-0 Please consider dropping the version from the Description. No other service has that. > After=network.target syslog.target syslog.target became irrelevant and was removed in 2013. Please remove that dependency. I am aware some packages still have that, but it's just cargo-cult now. > Requires=network.target This does not make sense for your daemon. See man systemd.special(7) about the intended use of network.target. > [Service] > Type=forking > Restart=on-failure > ExecStart=/usr/sbin/vma start > ExecStop=/usr/sbin/vma stop /usr/sbin/vma is a SysV initscript. That's terrible. Why not start the deamon binary directly?: ExecStart=/usr/sbin/vmad (And maybe set KillSignal if needed.) > ExecReload=/usr/sbin/vma restart You must not fake the reload operation with restart. If the service cannot do a reload, just do not define ExecReload. > RestartForceExitStatus=1 SIGTERM It's unusual to need to use this setting. There may be a good reason for it, but please double check. Hi Michal, I have updated libvma.spec to meet most of your requirements and added additional patch as https://raw.githubusercontent.com/igor-ivanov/libvma/pr/fedora-package/fedora/0002-Update-systemctl-files.patch For others please look at my comments inline. Thanks, Igor (In reply to Michal Schmidt from comment #21) > Hi Igor, > I have not added you to the packagers group yet. I have some questions and > comments about the package. > > libvma.spec: > > BuildRequires: libibverbs-devel > > BuildRequires: rdma-core-devel > Done > These days, libibverbs-devel is provided by rdma-core-devel. > > > BuildRequires: libnl3-devel > > Your config/m4/nl.m4 uses PKG_CHECK_MODULES, so you should BR > pkgconfig(libnl-route-3.0) instead. > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > PkgConfigBuildRequires/ > Done > > %build > > export revision=%{use_rel} > > This seems unnecessary. I did not find anything referencing the "revision" > environment variable in your build system. > It is used during configure to set VMA_SVN_REVISION. > > %install > > %{make_build} DESTDIR=${RPM_BUILD_ROOT} install > > You should use %{make_install} instead. > Corrected but following rules as https://docs.fedoraproject.org/en-US/packaging-guidelines/#_why_the_makeinstall_macro_should_not_be_used permit original usage. > > %files > > %{_libdir}/%{name}.so* > > So the package contains something like this: > libvma.so -> libvma.so.9.1.0 > libvma.so.9 -> libvma.so.9.1.0 > libvma.so.9.1.0 > > Since libvma.so is to be used only for LD_PRELOAD and no program should be > linked against it (and have DT_NEEDED for it),: > 1. is there a point in versioning the library? > 2. does it have to be installed in the linker's default path? Would it make > more sense to have it as %{_libdir}/libvma/libvma.so ? > livma has own extra api with vma_extra.h header file and admits direct linkage. > > Do you actually use configure_options for anything? I don't think it's > common for Fedora packages to have macros like that "just in case". > This option is used to provide flexibility in final binary libvma version. For example: this option can be used to enable tso support (--enable-tso) that increases throughput but not ready to be included by default. > use_rel - This is fascinating. The macro not only determines the value of > the Release tag, it also influences the build where its value gets used for > version compatibility checks and encoded in version strings. To me the > mechanism seems needlessly complicated and I fail to see the benefit of it. > Also, are you aware that sometimes automatic scripts (e.g. release > engineering mass rebuilds) bump the Release fields in Fedora spec files? > Normally in RPM, Version is the upstream version and Release serves > packaging needs. > I removed it but it is used in pair with configure_options to differentiate packages for the customers on our side. > 30-libvma-limits.conf: > > # Default limits that are needed for proper work of libvma > > # Read more about this topic in the VMA's User Manual > > Can you point me to where in the manual is this discussed? > > > * - memlock unlimited > > * soft memlock unlimited > > * hard memlock unlimited > > Does having the package installed give every user on the system the > permission to mlock unlimited amounts of RAM? Is that really necessary? > Could it be at least limited to a user group? > There is such recommendation at https://docs.mellanox.com/display/VMAv902/VMA+Installation+Options that forces to https://github.com/Mellanox/libvma/wiki/VMA-over-RHEL-7.x-with-inbox-driver#4-common-configurations > vma.service: > > [Unit] > > Description=VMA Daemon. Version: 9.1.0-0 > > Please consider dropping the version from the Description. No other service > has that. > Done > > After=network.target syslog.target > > syslog.target became irrelevant and was removed in 2013. Please remove that > dependency. I am aware some packages still have that, but it's just > cargo-cult now. > > > Requires=network.target > > This does not make sense for your daemon. See man systemd.special(7) about > the intended use of network.target. > Done > > [Service] > > Type=forking > > Restart=on-failure > > ExecStart=/usr/sbin/vma start > > ExecStop=/usr/sbin/vma stop > > /usr/sbin/vma is a SysV initscript. That's terrible. > Why not start the deamon binary directly?: > ExecStart=/usr/sbin/vmad > (And maybe set KillSignal if needed.) > It does support of some systems easier for us. > > ExecReload=/usr/sbin/vma restart > > You must not fake the reload operation with restart. If the service cannot > do a reload, just do not define ExecReload. > > > RestartForceExitStatus=1 SIGTERM > > It's unusual to need to use this setting. There may be a good reason for it, > but please double check. SIGTERM is generated by kill utility by default. So it is used to restart vmad in this case too. (In reply to igor.ivanov.va from comment #22) > > > %build > > > export revision=%{use_rel} > > > > This seems unnecessary. I did not find anything referencing the "revision" > > environment variable in your build system. > > > > It is used during configure to set VMA_SVN_REVISION. Oh, I see. I was looking at the master branch, where it's not used anymore, but it is indeed used in the version you packaged. > > > %install > > > %{make_build} DESTDIR=${RPM_BUILD_ROOT} install > > > > You should use %{make_install} instead. > > > > Corrected but following rules as > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_why_the_makeinstall_macro_should_not_be_used permit original usage. The current guidelines allow this form: make DESTDIR=$RPM_BUILD_ROOT install but that's not the same as what you originally had: %{make_build} DESTDIR=${RPM_BUILD_ROOT} install There's been a recent push to convert all Fedora spec files to %make_install: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/IPPMIU6DS664TCWTLBR7WA2CUAOFUD7M/ Thanks for having changed it to %{make_install}. > > > %files > > > %{_libdir}/%{name}.so* > > > > So the package contains something like this: > > libvma.so -> libvma.so.9.1.0 > > libvma.so.9 -> libvma.so.9.1.0 > > libvma.so.9.1.0 > > > > Since libvma.so is to be used only for LD_PRELOAD and no program should be > > linked against it (and have DT_NEEDED for it),: > > 1. is there a point in versioning the library? > > 2. does it have to be installed in the linker's default path? Would it make > > more sense to have it as %{_libdir}/libvma/libvma.so ? > > livma has own extra api with vma_extra.h header file and admits direct > linkage. I didn't see such usage in the documentation, but I have now tested linking a C program with libvma directly and it seems to work, so OK. > > Do you actually use configure_options for anything? I don't think it's > > common for Fedora packages to have macros like that "just in case". > > This option is used to provide flexibility in final binary libvma version. OK, it does no harm. It can stay. > > 30-libvma-limits.conf: > > > # Default limits that are needed for proper work of libvma > > > # Read more about this topic in the VMA's User Manual > > > * - memlock unlimited > > > * soft memlock unlimited > > > * hard memlock unlimited > > > > Does having the package installed give every user on the system the > > permission to mlock unlimited amounts of RAM? Is that really necessary? > > Could it be at least limited to a user group? > > There is such recommendation at > https://docs.mellanox.com/display/VMAv902/VMA+Installation+Options that > forces to > https://github.com/Mellanox/libvma/wiki/VMA-over-RHEL-7.x-with-inbox- > driver#4-common-configurations Thanks for the pointer, but it does not answer the question why the memlock resource limit has to be lifted for all users by default. Have other options been considered? Could it be limited to a user group? i.e.: @libvmauser - memlock unlimited > > > [Service] > > > Type=forking > > > Restart=on-failure > > > ExecStart=/usr/sbin/vma start > > > ExecStop=/usr/sbin/vma stop > > > > /usr/sbin/vma is a SysV initscript. That's terrible. > > Why not start the deamon binary directly?: > > ExecStart=/usr/sbin/vmad > > (And maybe set KillSignal if needed.) > > It does support of some systems easier for us. Please elaborate. What systems? How exactly does running a SysV script from a systemd service help? I see the SysV script performs work that systemd already does by itself (checking whether the service is already running, finding a process to kill, reporting status). And it does it less accurately than systemd (which tracks services by cgroups). It also adds some "sleep 1", which is either pointless, or papering over a race condition bug. I tried running the daemon directly using "ExecStart=/usr/sbin/vmad" it the unit file. To my surprise the daemon exits immediately when run this way. This is because of this code in tools/daemon/daemon.c:main(): /* already a daemon */ if (getppid() == 1) { return 0; } That should be removed. Daemons should not change behaviour depending on which process spawned them. > > > RestartForceExitStatus=1 SIGTERM > > > > It's unusual to need to use this setting. There may be a good reason for it, > > but please double check. > > SIGTERM is generated by kill utility by default. So it is used to restart > vmad in this case too. Yes, kill sends SIGTERM by default. I don't understand how the second sentence follows. If I kill a service with SIGTERM, presumably I want it to terminate, don't I? (In reply to Michal Schmidt from comment #23) > > > 30-libvma-limits.conf: > > > > # Default limits that are needed for proper work of libvma > > > > # Read more about this topic in the VMA's User Manual > > > > * - memlock unlimited > > > > * soft memlock unlimited > > > > * hard memlock unlimited > > > > > > Does having the package installed give every user on the system the > > > permission to mlock unlimited amounts of RAM? Is that really necessary? > > > Could it be at least limited to a user group? > > > > There is such recommendation at > > https://docs.mellanox.com/display/VMAv902/VMA+Installation+Options that > > forces to > > https://github.com/Mellanox/libvma/wiki/VMA-over-RHEL-7.x-with-inbox- > > driver#4-common-configurations > > Thanks for the pointer, but it does not answer the question why the memlock > resource limit has to be lifted for all users by default. Have other options > been considered? Could it be limited to a user group? i.e.: > @libvmauser - memlock unlimited In real HPC production environment, we are always to lifted the resource for all users. For example, see "13.5.3. Relaxing memlock restrictions for users" https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/networking_guide/sec-configuring_the_base_rdma_subsystem (In reply to Honggang LI from comment #24) > In real HPC production environment, we are always to lifted the resource for > all users. For example, see "13.5.3. Relaxing memlock restrictions for users" > > https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/ > html/networking_guide/sec-configuring_the_base_rdma_subsystem Do we ship this config file in a package? It's one thing to suggest administrators to configure their systems in a certain way. It still requires a concious decision on their part. It's another thing to ship this config in a package. (In reply to Michal Schmidt from comment #25) > (In reply to Honggang LI from comment #24) > > In real HPC production environment, we are always to lifted the resource for > > all users. For example, see "13.5.3. Relaxing memlock restrictions for users" > > > > https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/ > > html/networking_guide/sec-configuring_the_base_rdma_subsystem > > Do we ship this config file in a package? No, but we setup it with kickstart scripts for our internal cluster. [root@rdma-dev-01 ~]$ cat /etc/security/limits.d/rdma.conf # configuration for rdma performance tuning * soft memlock unlimited * hard memlock unlimited * soft stack unlimited * hard stack unlimited * soft core unlimited * hard core unlimited # rdma tuning end > It's one thing to suggest administrators to configure their systems in a > certain way. It still requires a concious decision on their part. > It's another thing to ship this config in a package. (In reply to Michal Schmidt from comment #23) > (In reply to igor.ivanov.va from comment #22) > > > > > [Service] > > > > Type=forking > > > > Restart=on-failure > > > > ExecStart=/usr/sbin/vma start > > > > ExecStop=/usr/sbin/vma stop > > > > > > /usr/sbin/vma is a SysV initscript. That's terrible. > > > Why not start the deamon binary directly?: > > > ExecStart=/usr/sbin/vmad > > > (And maybe set KillSignal if needed.) > > > > It does support of some systems easier for us. > > Please elaborate. What systems? How exactly does running a SysV script from > a systemd service help? > I see the SysV script performs work that systemd already does by itself > (checking whether the service is already running, finding a process to kill, > reporting status). And it does it less accurately than systemd (which tracks > services by cgroups). It also adds some "sleep 1", which is either > pointless, or papering over a race condition bug. > > I tried running the daemon directly using "ExecStart=/usr/sbin/vmad" it the > unit file. To my surprise the daemon exits immediately when run this way. > This is because of this code in tools/daemon/daemon.c:main(): > > /* already a daemon */ > if (getppid() == 1) { > return 0; > } > > That should be removed. Daemons should not change behaviour depending on > which process spawned them. > It seems that I did not understand you initial question but I see your point now. I will look and back with answer. > > > > RestartForceExitStatus=1 SIGTERM > > > > > > It's unusual to need to use this setting. There may be a good reason for it, > > > but please double check. > > > > SIGTERM is generated by kill utility by default. So it is used to restart > > vmad in this case too. > > Yes, kill sends SIGTERM by default. I don't understand how the second > sentence follows. If I kill a service with SIGTERM, presumably I want it to > terminate, don't I? You are correct. Current behaviour was done basing on customer's requests that want to stop daemon just using systemctl and avoid stopping one by tool like kill, pkill. Thank you for review Package was updated. Changes include: 1. 30-libvma-limits.conf was removed (let administrator configure system basing on internal requirements for user, groups etc. libvma has run-time check as https://github.com/igor-ivanov/libvma/blob/master/src/vma/main.cpp#L423) 2. vma.service started using vmad directly. Hello, Do you see other issue? Thanks, Igor Igor, I can see no more blocking issues. Thanks! I have added you to the "packager" Fedora group. You can reopen https://pagure.io/releng/issue/9509 now. Thank you Hi, We've reopened the ticket to Unretire libvma: https://pagure.io/releng/issue/9509. Thanks, Alex (In reply to alexv from comment #32) > We've reopened the ticket to Unretire libvma: > https://pagure.io/releng/issue/9509. The ticket is closed now. Please build latest upstream release libvma for Fedora Rawhide. It is better to build libvma for f32 and f31 too. Thanks Hi Honggang, Could you advise how to create remote f31, f32 branches for libvma. I can not do it useing `git push origin f32` Thanks, Igor $ cd "$LIBVMA_REPO_DIR" $ fedpkg request-branch "$BRANCH_NAME" or, alternatively: $ fedpkg request-branch --repo "rpms/libvma" "$BRANCH_NAME" I submitted request to unblock f31 as https://pagure.io/releng/issue/9681 Please create fedora update for libvma-9.0.2-1.fc32 and libvma build for F31. Install bodhi-client and run 'bodhi updates new --help' for documentation. libvma-9.0.2-1.fc33 and libvma-9.0.2-1.fc34 had been built. Close it. (In reply to Honggang LI from comment #37) > Please create fedora update for libvma-9.0.2-1.fc32 and libvma build for F31. > > Install bodhi-client and run 'bodhi updates new --help' for documentation. I used "fedpkg update" for this w/o success. It looks as common issue these days (for example: https://www.spinics.net/lists/fedora-devel/msg276677.html). Please use bodhi to create update for libvma. I confirmed it works for me.
fabtests (f32)]$ bodhi updates new --type bugfix --notes "Update fabtests to latest upstream release v1.11.0" --user honli fabtests-1.11.0-1.fc32
================================================================================
fabtests-1.11.0-1.fc32
================================================================================
Update ID: FEDORA-2020-2b79a2faad
Content Type: rpm
Release: Fedora 32
Status: pending
Type: bugfix
Severity: unspecified
Karma: 0
Autokarma: False [-3, 3]
CI Status: no tests are required
Request: testing
Notes: Update fabtests to latest upstream release v1.11.0
Submitter: honli
Submitted: 2020-08-17 09:15:32
Comments: bodhi - 2020-08-17 09:15:32 (karma 0)
This update has been submitted for testing by honli.
bodhi - 2020-08-17 09:15:32 (karma 0)
This update's test gating status has been changed to 'ignored'.
bodhi - 2020-08-17 09:15:32 (karma 0)
This update's test gating status has been changed to 'waiting'.
https://bodhi.fedoraproject.org/updates/FEDORA-2020-2b79a2faad
Caveats:
The number of stable days required was set to the mandatory release value of 7 days
Thank you, Honggang. It works. f31: https://bodhi.fedoraproject.org/updates/FEDORA-2020-cc2738fbff f32: https://bodhi.fedoraproject.org/updates/FEDORA-2020-32cff33f00 |