Differential D232

download RPM headers - feed RPM file to rpmUtils using a pipe
AbandonedPublic
Actions

Authored by garretraziel on Sep 9 2014, 10:53 AM.

Details

Reviewers
kparal
tflink
jdulaney
Maniphest Tasks
T226: depcheck: download only rpm headers?
Summary

Use python-rpm, named pipe and threading
for downloading RPM headers.

Test Plan

Add headers_only=true to task-depcheck to koji directive,
run depcheck.

Diff Detail

Repository
rLTRN libtaskotron
Branch
feature/T226-headers_alt
Lint
No Linters Available
Unit
No Unit Test Coverage
garretraziel retitled this revision from to use different approach for downloading rpm headers.Sep 9 2014, 10:53 AM
garretraziel updated this object.
garretraziel edited the test plan for this revision. (Show Details)
garretraziel added a task: T226: depcheck: download only rpm headers?.Sep 9 2014, 10:53 AM
garretraziel added reviewers: tflink, kparal, jskladan.

note: This isn't complete solution - I am only publishing this for discussion.

jdulaney added a reviewer: jdulaney.Sep 9 2014, 4:22 PM
jdulaney added a subscriber: jdulaney.
jdulaney commandeered this revision.Sep 9 2014, 4:31 PM
jdulaney edited reviewers, added: garretraziel; removed: jdulaney.

Code looks good, testing.

garretraziel added a comment.Sep 10 2014, 10:15 AM

Note: msimacek from koschei also implemented this approach, independently on me, here is his solution.

garretraziel commandeered this revision.Oct 8 2014, 12:33 PM
garretraziel edited reviewers, added: jdulaney; removed: garretraziel.
kparal retitled this revision from use different approach for downloading rpm headers to download RPM headers - feed RPM file to rpmUtils using a pipe.Oct 9 2014, 1:52 PM
kparal added a comment.Oct 9 2014, 3:46 PM

I like this solution much more than D223. That doesn't mean I don't have any reservations about the current code, I have quite a few, but I think the concept is good.

There are some possible improvements, like using an unnamed pipe, similar to the linked koschei script (which is not an easy-to-understand code, however). Also, if there were some concerns about using threads, I think we can do this in a single thread:

  1. download a fixed amount of data, e.g. 10kB (let's measure the usual rpm header size and use this value)
  2. try to pass it to ts.hdrFromFdno()
  3. if it fails, download another chunk, e.g. twice the size than in #1
  4. try to pass it to ts.hdrFromFdno()
  5. if it worked, close and clean up everything
  6. if it didn't work, bail out (return None or something) and fall back to full RPM download

All nice and simple, in a single thread. It does not support downloading arbitrary amount of data, but I think we don't want to support that anyway. If the header is not found in a reasonable small chunk of initial data, we want to print a warning and fall back to regular methods.

@tflink, you had the largest reservation regarding RPM header approach, what do you think about this patch? Of course, if we detect production environment, we can always download full RPMs, if we want to. But with the number of RPMs we test every day on our dev machines, I think that after running this a week or two on our dev machines, we will be reasonably sure whether there are some potential problems with it or not.

jskladan added a comment.May 6 2015, 11:28 AM

as this seems to be abanodned anyway, how about closing the revision?

kparal added a comment.May 6 2015, 11:41 AM

I would still like to incorporate either this or D223. It has not been a pressing issue in the past, because we now have rpm caching in dev mode, but from time to time it is still PITA to work on depcheck and wait for the initial several GBs large download. We also had issues with slow downloads in production, and soon we will have the need to speed up our checks, because esp. during freeze periods our task queues start to fill up rapidly. I think the major issue here is not "this might cause weird issues" but rather "can we detect broken headers and in such case download the full file instead?". And it seems to me that the answer is yes. Not to mention we could even store the rpm headers as artifacts now that we have support for it, and use them for reproducing issues if needed.

jskladan added a comment.May 7 2015, 7:03 AM

Sure, but the Diff is almost 8 months old, and nothing happened. If you do not feel like closing it, I'll remove myself from reviewers - it annoyed me on the front page long enough :)

jskladan removed a reviewer: jskladan.May 7 2015, 7:04 AM
jskladan added a subscriber: jskladan.
garretraziel abandoned this revision.Aug 27 2015, 12:28 PM

It looks like there are currently other things to do. I will abandon this revision, we can open it again when we will need it.

Revision Contents

Diff 651

View Options

libtaskotron/directives/koji_directive.py

Show First 20 Lines • Show All 114 Lines • ▼ Show 20 Line(s)114 else:
115 self.target_dir = input_data['target_dir'] 115​ self.target_dir = input_data['target_dir']
116 116​
117 if 'arch' not in input_data: 117​ if 'arch' not in input_data:
118 detected_args = ', '.join(input_data.keys()) 118​ detected_args = ', '.join(input_data.keys())
119 raise exc.TaskotronDirectiveError( 119​ raise exc.TaskotronDirectiveError(
120 "The koji directive requires 'arch' as an argument. Detected " 120​ "The koji directive requires 'arch' as an argument. Detected "
121 "arguments: %s" % detected_args) 121​ "arguments: %s" % detected_args)
122 122​
123​ headers_only = False
124​ if 'headers_only' in input_data:
125​ headers_only = input_data['headers_only']
126​
123 self.arches = input_data['arch'] 127​ self.arches = input_data['arch']
124 if self.arches and ('noarch' not in self.arches): 128​ if self.arches and ('noarch' not in self.arches):
125 self.arches.append('noarch') 129​ self.arches.append('noarch')
126 130​
127 self.src = input_data.get('src', False) 131​ self.src = input_data.get('src', False)
128 132​
129 # download files 133​ # download files
130 output_data = {} 134​ output_data = {}
131 135​
132 if action == 'download': 136​ if action == 'download':
133 if 'koji_build' not in input_data: 137​ if 'koji_build' not in input_data:
134 detected_args = ', '.join(input_data.keys()) 138​ detected_args = ', '.join(input_data.keys())
135 raise exc.TaskotronDirectiveError( 139​ raise exc.TaskotronDirectiveError(
136 "The koji directive requires 'koji_build' for the 'download' " 140​ "The koji directive requires 'koji_build' for the 'download' "
137 "action. Detected arguments: %s" % detected_args) 141​ "action. Detected arguments: %s" % detected_args)
138 142​
139 nvr = rpm_utils.rpmformat(input_data['koji_build'], 'nvr') 143​ nvr = rpm_utils.rpmformat(input_data['koji_build'], 'nvr')
140 144​
141 output_data['downloaded_rpms'] = self.koji.get_nvr_rpms(nvr, 145​ output_data['downloaded_rpms'] = self.koji.get_nvr_rpms(nvr,
142 self.target_dir, arches=self.arches, src=self.src) 146​ self.target_dir, arches=self.arches, src=self.src,
147​ headers_only=headers_only)
143 148​
144 elif action == 'download_tag': 149​ elif action == 'download_tag':
145 if 'koji_tag' not in input_data: 150​ if 'koji_tag' not in input_data:
146 detected_args = ', '.join(input_data.keys()) 151​ detected_args = ', '.join(input_data.keys())
147 raise exc.TaskotronDirectiveError( 152​ raise exc.TaskotronDirectiveError(
148 "The koji directive requires 'koji_tag' for the 'download_tag' " 153​ "The koji directive requires 'koji_tag' for the 'download_tag' "
149 "action. Detected arguments: %s" % detected_args) 154​ "action. Detected arguments: %s" % detected_args)
150 155​
151 koji_tag = input_data['koji_tag'] 156​ koji_tag = input_data['koji_tag']
152 157​
153 output_data['downloaded_rpms'] = self.koji.get_tagged_rpms(koji_tag, 158​ output_data['downloaded_rpms'] = self.koji.get_tagged_rpms(koji_tag,
154 self.target_dir, arches=self.arches, src=self.src) 159​ self.target_dir, arches=self.arches, src=self.src,
160​ headers_only=headers_only)
155 161​
156 return output_data 162​ return output_data
View Options

libtaskotron/exceptions.py

Show All 28 Lines
29class TaskotronDirectiveError(TaskotronError): 29​class TaskotronDirectiveError(TaskotronError):
30 '''All errors related to Taskotron directives''' 30​ '''All errors related to Taskotron directives'''
31 pass 31​ pass
32 32​
33 33​
34class TaskotronRemoteError(TaskotronError): 34​class TaskotronRemoteError(TaskotronError):
35 '''All network and remote-server related errors''' 35​ '''All network and remote-server related errors'''
36 pass 36​ pass
37​
38​class TaskotronRemoteReadError(TaskotronRemoteError):
39​ '''Error in data obtained from network'''
40​ pass
View Options

libtaskotron/file_utils.py

1# -*- coding: utf-8 -*- 1​# -*- coding: utf-8 -*-
2# Copyright 2009-2014, Red Hat, Inc. 2​# Copyright 2009-2014, Red Hat, Inc.
3# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+> 3​# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4# See the LICENSE file for more details on Licensing 4​# See the LICENSE file for more details on Licensing
5 5​
6from __future__ import absolute_import 6​from __future__ import absolute_import
7import os 7​import os
8import sys 8​import sys
9​import threading
10​import tempfile
11​import urllib2
9import urlgrabber.grabber 12​import urlgrabber.grabber
10import urlgrabber.progress 13​import urlgrabber.progress
14​import rpm
11from libtaskotron.logger import log 15​from libtaskotron.logger import log
12from libtaskotron.exceptions import TaskotronRemoteError 16​from libtaskotron.exceptions import TaskotronRemoteError, TaskotronRemoteReadError
13 17​
14 18​
15def makedirs(fullpath): 19​def makedirs(fullpath):
16 '''This is the same as :meth:`os.makedirs`, but does not raise an exception 20​ '''This is the same as :meth:`os.makedirs`, but does not raise an exception
17 when the destination directory already exists. 21​ when the destination directory already exists.
18 22​
19 :raise OSError: if directory doesn't exist and can't be created 23​ :raise OSError: if directory doesn't exist and can't be created
20 ''' 24​ '''
▲ Show 20 Lines • Show All 90 Lines • ▼ Show 20 Line(s)113 except urlgrabber.grabber.URLGrabError, e:
111 # the file can be incomplete, remove 115​ # the file can be incomplete, remove
112 if os.path.exists(dest): 116​ if os.path.exists(dest):
113 try: 117​ try:
114 os.remove(dest) 118​ os.remove(dest)
115 except OSError, e: 119​ except OSError, e:
116 log.exception('Could not delete incomplete file: %s', dest) 120​ log.exception('Could not delete incomplete file: %s', dest)
117 raise TaskotronRemoteError(e) 121​ raise TaskotronRemoteError(e)
118 122​
119 return dest 123​ return dest
120No newline at end of file 124​
125​def download_hdr(url):
126​ class HdrCheckThread(threading.Thread):
127​ def __init__(self, fifoname):
128​ threading.Thread.__init__(self)
129​ self.fifoname = fifoname
130​
131​ def run(self):
132​ ts = rpm.ts()
133​ fdno_r = os.open(self.fifoname, os.O_RDONLY)
134​ ts.hdrFromFdno(fdno_r)
135​
136​ tmpdir = tempfile.mkdtemp()
137​ fifoname = os.path.join(tmpdir, 'tmpfifo.rpm')
138​ os.mkfifo(fifoname)
139​
140​ hdrCheckThread = HdrCheckThread(fifoname)
141​ hdrCheckThread.start()
142​
143​ netfdno = urllib2.urlopen(url)
144​ fdno_w = os.open(fifoname, os.O_WRONLY)
145​
146​ result = ""
147​
148​ while hdrCheckThread.is_alive():
149​ buff = netfdno.read(10000)
150​ result += buff
151​ os.write(fdno_w, buff)
152​
153​ os.close(fdno_w)
154​
155​ os.remove(fifoname)
156​
157​ return result
158​
159​def download_rpm_header(url, dirname, filename=None, overwrite=False):
160​ '''Download RPM header file.
161​
162​ :param str url: RPM URL to download
163​ :param str dirname: directory path, if the directory does not exist, it gets
164​ created (and all its parent directories).
165​ :param str filename: name of downloaded file, if not provided basename is
166​ extracted from URL
167​ :param bool overwrite: if the destination file already exists, whether to
168​ overwrite or not. If ``False``, a simple check is
169​ performed whether the remote file is the same as the
170​ local file and re-downloads it anyway if they are not
171​ the same.
172​ :param grabber: custom :class:`urlgrabber.grabber.URLGrabber` instance to be
173​ used for actual downloading
174​ :return: file path of the downloaded file
175​ :rtype: str
176​ :raise TaskotronRemoteError: if download fails or directory can't be created
177​ '''
178​ if not filename:
179​ filename = os.path.basename(url)
180​
181​ dest = os.path.join(dirname, filename)
182​
183​ if os.path.exists(dirname):
184​ if not os.path.isdir(dirname):
185​ raise TaskotronRemoteError(
186​ "Can't create directory: %r It is an already "
187​ "existing file.", dirname)
188​ else:
189​ try:
190​ makedirs(dirname)
191​ except OSError, e:
192​ log.exception("Can't create directory: %s", dirname)
193​ raise TaskotronRemoteError(e)
194​
195​ # overwrite?
196​ if os.path.exists(dest):
197​ if overwrite:
198​ os.remove(dest)
199​ else:
200​ log.debug('Already downloaded: %s', os.path.basename(dest))
201​ return dest
202​
203​ log.debug('Downloading: %s', url)
204​ content = download_hdr(url)
205​
206​ res = open(dest, "wb")
207​ res.write(content)
208​ res.close()
209​
210​ return dest
View Options

libtaskotron/koji_utils.py

Show First 20 Lines • Show All 161 Lines • ▼ Show 20 Line(s)110 def nvr_to_urls(self, nvr, arches=['all'], debuginfo=False, src=True):
162 baseurl = '/'.join((config.get_config().pkg_url, info['package_name'], 162​ baseurl = '/'.join((config.get_config().pkg_url, info['package_name'],
163 info['version'], info['release'])) 163​ info['version'], info['release']))
164 urls = ['%s/%s' % (baseurl, koji.pathinfo.rpm(r)) for r in rpms] 164​ urls = ['%s/%s' % (baseurl, koji.pathinfo.rpm(r)) for r in rpms]
165 165​
166 return sorted(urls) 166​ return sorted(urls)
167 167​
168 168​
169 def get_nvr_rpms(self, nvr, dest, arches=['all'], debuginfo=False, 169​ def get_nvr_rpms(self, nvr, dest, arches=['all'], debuginfo=False,
170 src=False): 170​ src=False, headers_only=False):
171 '''Retrieve the RPMs associated with a build NVR into the specified 171​ '''Retrieve the RPMs associated with a build NVR into the specified
172 directory. 172​ directory.
173 173​
174 :param str nvr: build NVR 174​ :param str nvr: build NVR
175 :param str dest: location where to store the RPMs 175​ :param str dest: location where to store the RPMs
176 :param arches: see :meth:`nvr_to_urls` 176​ :param arches: see :meth:`nvr_to_urls`
177 :param debuginfo: see :meth:`nvr_to_urls` 177​ :param debuginfo: see :meth:`nvr_to_urls`
178 :param src: see :meth:`nvr_to_urls` 178​ :param src: see :meth:`nvr_to_urls`
179​ :param headers_only: don't download whole RPMs, download only headers
179 :return: list of local filenames of the grabbed RPMs 180​ :return: list of local filenames of the grabbed RPMs
180 :rtype: list of str 181​ :rtype: list of str
181 :raise TaskotronRemoteError: if the files can't be downloaded or saved 182​ :raise TaskotronRemoteError: if the files can't be downloaded or saved
182 :raise TaskotronValueError: if ``arches=[]`` and ``src=False``, 183​ :raise TaskotronValueError: if ``arches=[]`` and ``src=False``,
183 therefore there is nothing to download 184​ therefore there is nothing to download
184 ''' 185​ '''
185 186​
186 rpm_urls = self.nvr_to_urls(nvr, arches, debuginfo, src) 187​ rpm_urls = self.nvr_to_urls(nvr, arches, debuginfo, src)
187 188​
188 rpm_files = [] 189​ rpm_files = []
189 log.info('Fetching %s RPMs for: %s (into %s)', len(rpm_urls), nvr, dest) 190​ log.info('Fetching %s RPMs for: %s (into %s)', len(rpm_urls), nvr, dest)
190 for url in rpm_urls: 191​ for url in rpm_urls:
191 rpm_file = file_utils.download(url, dest) 192​ if headers_only:
193​ rpm_file = file_utils.download_rpm_header(url, dest)
194​ else:
195​ rpm_file = file_utils.download(url, dest)
192 rpm_files.append(rpm_file) 196​ rpm_files.append(rpm_file)
193 197​
194 return rpm_files 198​ return rpm_files
195 199​
196 200​
197 def get_tagged_rpms(self, tag, dest, arches=['all'], debuginfo=False, 201​ def get_tagged_rpms(self, tag, dest, arches=['all'], debuginfo=False,
198 src=False): 202​ src=False, headers_only=False):
199 '''Downloads all RPMs of all NVRs tagged by a specific Koji tag. 203​ '''Downloads all RPMs of all NVRs tagged by a specific Koji tag.
200 204​
201 Note: This works basically the same as :meth:`get_nvr_rpms`, it just 205​ Note: This works basically the same as :meth:`get_nvr_rpms`, it just
202 downloads a lot of builds instead of a single one. For description of 206​ downloads a lot of builds instead of a single one. For description of
203 all shared parameters and return values, please see that method. 207​ all shared parameters and return values, please see that method.
204 208​
205 :param str tag: Koji tag to be queried for available builds, e.g. 209​ :param str tag: Koji tag to be queried for available builds, e.g.
206 ``f20-updates-pending`` 210​ ``f20-updates-pending``
207 ''' 211​ '''
208 log.debug('Querying Koji for tag: %s' % tag) 212​ log.debug('Querying Koji for tag: %s' % tag)
209 tag_data = self.session.listTagged(tag) 213​ tag_data = self.session.listTagged(tag)
210 214​
211 nvrs = ["%(nvr)s" % x for x in tag_data] 215​ nvrs = ["%(nvr)s" % x for x in tag_data]
212 rpms = [] 216​ rpms = []
213 log.info("Fetching %s builds for tag: %s", len(nvrs), tag) 217​ log.info("Fetching %s builds for tag: %s", len(nvrs), tag)
214 log.debug('Builds to be downloaded:\n %s', '\n '.join(nvrs)) 218​ log.debug('Builds to be downloaded:\n %s', '\n '.join(nvrs))
215 219​
216 for nvr in nvrs: 220​ for nvr in nvrs:
217 rpms.append(self.get_nvr_rpms(nvr, dest, arches, debuginfo, src)) 221​ rpms.append(self.get_nvr_rpms(nvr, dest, arches, debuginfo, src,
222​ headers_only))
218 223​
219 return rpms 224​ return rpms
220 225​
221 226​
222def getNEVR(build): 227​def getNEVR(build):
223 '''Extract RPM version identifier in NEVR format from Koji build object 228​ '''Extract RPM version identifier in NEVR format from Koji build object
224 229​
225 :param dict build: Koji buildinfo dictionary (as returned e.g. from 230​ :param dict build: Koji buildinfo dictionary (as returned e.g. from
Show All 17 Lines