Bug 1438666 - /distribution/inventory fails if the lab DHCP server puts unqualified hostnames in DHCP option 12
Summary: /distribution/inventory fails if the lab DHCP server puts unqualified hostnam...
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Beaker
Classification: Community
Component: general
Version: 24
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified vote
Target Milestone: 24.3
Assignee: Jonathan Toppins
QA Contact: Dan Callaghan
URL:
Whiteboard:
Keywords: Patch
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-04-04 06:17 UTC by Dan Callaghan
Modified: 2018-03-28 00:33 UTC (History)
5 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2017-05-30 07:08:06 UTC


Attachments (Terms of Use)

Description Dan Callaghan 2017-04-04 06:17:40 UTC
When Beaker provisions a test system, it relies on the system finding the correct hostname from the lab DHCP server. This comes from DHCP option 12.

In our production environments the DHCP server puts the FQDN into DHCP option 12, and we have a lot of different code which assumes that is true. However some DHCP servers (one notable example being dnsmasq, as used by libvirt, as used by the beaker-in-a-box demo environment) only put an unqualified hostname in DHCP option 12. As a result the system hostname is unqualified.

The /distribution/inventory task currently uses $HOSTNAME, which is a bash builtin populated from the system hostname, when reporting its inventory data back up to the Beaker server.

However if $HOSTNAME is an unqualified hostname, this won't match any known records in Beaker (which should always be using fully-qualified hostnames) and the task will fail.

Comment 1 Dan Callaghan 2017-04-04 06:18:08 UTC
I don't have an example of the error from /distribution/inventory handy right now, but it would be good to paste one in here for future searchability.

Comment 3 Dan Callaghan 2017-04-04 06:22:48 UTC
Simplest solution might be to just swap $HOSTNAME for $(hostname -f) in /distribution/inventory.

Comment 4 Neil Horman 2017-04-04 16:31:46 UTC
an alternate solution may be to mandate that your dhcp server provide fqdns in option 12.  dnsmasq has exactly that option in --dhcp-fqdn

Comment 5 Jonathan Toppins 2017-04-11 05:11:17 UTC
It would be nice to give the user the option to override the HOSTNAME parameter easily. That way if the beaker user/admin does not get a choice in what dhcp server they use a user is still able to easily make the inventory update task run.

I was thinking of something like this:


$ git --no-pager log -1 -p
commit 786f5a27b416834980f7b82a1721aceae5cade6d
Author: Jonathan Toppins <jtoppins@redhat.com>
Date:   Mon Apr 10 18:32:45 2017 -0400

    add an additional option to force override the task to use the FQDN instead of the hostname the box gets configured with during install

diff --git a/Client/src/bkr/client/commands/cmd_update_inventory.py b/Client/src/bkr/client/commands/cmd_update_inventory.py
index 69b9cf2a6962..d15bb63889e0 100644
--- a/Client/src/bkr/client/commands/cmd_update_inventory.py
+++ b/Client/src/bkr/client/commands/cmd_update_inventory.py
@@ -123,6 +123,12 @@ def options(self):
             action='store_true',
             help='Wait on job completion',
         )
+        self.parser.add_option(
+            '--force-hostname',
+            default=False,
+            action='store_true',
+            help='Force hostname to be the same as provided on commandline'
+        )
 
     def run(self, *args, **kwargs):
 
@@ -132,6 +138,7 @@ def run(self, *args, **kwargs):
         xml = kwargs.get('xml')
         prettyxml = kwargs.get('prettyxml')
         wait = kwargs.get('wait')
+        force_hostname = kwargs.get('force-hostname')
         self.set_hub(**kwargs)
         requests_session = self.requests_session()
         submitted_jobs = []
@@ -139,7 +146,8 @@ def run(self, *args, **kwargs):
         for fqdn in args:
             res = requests_session.post('jobs/+inventory',
                                         json={'fqdn':fqdn,
-                                              'dryrun':dryrun})
+                                              'dryrun':dryrun,
+                                              'forcehost':force_hostname})
             try:
                 res.raise_for_status()
             except HTTPError, e:
diff --git a/Server/bkr/server/jobs.py b/Server/bkr/server/jobs.py
index 82f378df564b..44a328835add 100644
--- a/Server/bkr/server/jobs.py
+++ b/Server/bkr/server/jobs.py
@@ -1218,7 +1218,7 @@ def update_job_status(id):
 @auth_required
 def submit_inventory_job():
     """
-    Submit a inventory job with the most suitable distro selected automatically.
+    Submit an inventory job with the most suitable distro selected automatically.
 
     Returns a dictionary consisting of the job_id, recipe_id, status (recipe status) 
     and the job XML. If ``dryrun`` is set to ``True`` in the request, the first three 
@@ -1226,6 +1226,7 @@ def submit_inventory_job():
 
     :jsonparam string fqdn: Fully-qualified domain name for the system.
     :jsonparam bool dryrun: If True, do not submit the job
+    :jsonparam bool forcehost: If True, force HOSTNAME parameter to be the same as fqdn
     """
     if 'fqdn' not in request.json:
         raise BadRequest400('Missing the fqdn parameter')
@@ -1234,6 +1235,11 @@ def submit_inventory_job():
         dryrun = request.json['dryrun']
     else:
         dryrun = False
+    if 'forcehost' in request.json:
+        forcehost = request.json['forcehost']
+    else:
+        forcehost = False
+
     try:
         system = System.by_fqdn(fqdn, identity.current.user)
     except DatabaseLookupError:
@@ -1246,8 +1252,11 @@ def submit_inventory_job():
     job_details = {}
     job_details['system'] = system
     job_details['whiteboard'] = 'Update Inventory for %s' % fqdn
+    job_details['fqdn'] = fqdn
+
     with convert_internal_errors():
-        job_xml = Job.inventory_system_job(distro, dryrun=dryrun, **job_details)
+        job_xml = Job.inventory_system_job(distro, dryrun=dryrun,
+                                           forcehost=forcehost, **job_details)
     r = {}
     if not dryrun:
         r = system.find_current_hardware_scan_recipe().__json__()
diff --git a/Server/bkr/server/model/scheduler.py b/Server/bkr/server/model/scheduler.py
index b67e40755791..6fc53be5d117 100644
--- a/Server/bkr/server/model/scheduler.py
+++ b/Server/bkr/server/model/scheduler.py
@@ -977,7 +977,8 @@ def provision_system_job(cls, distro_trees, pick='auto', **kw):
         return job
 
     @classmethod
-    def inventory_system_job(cls, distro_tree, dryrun=False, **kw):
+    def inventory_system_job(cls, distro_tree, dryrun=False,
+                             forcehost=False, **kw):
         """ Create a new inventory job and return the job XML.
         If this is not a dry run, then also submit the job
         """
@@ -1013,6 +1014,10 @@ def inventory_system_job(cls, distro_tree, dryrun=False, **kw):
         recipe.tasks.append(install_task)
         # Add inventory task
         inventory_task = RecipeTask.from_task(Task.by_name(u'/distribution/inventory'))
+        if forcehost:
+            inventory_task.params = [
+                RecipeTaskParam("HOSTNAME", kw.get('fqdn')),
+            ]
         recipe.tasks.append(inventory_task)
         recipeSet.recipes.append(recipe)
         job.recipesets.append(recipeSet)

Comment 6 Jonathan Toppins 2017-04-17 18:56:42 UTC
I will continue what my RFC code and take a crack at getting this upstream.

Comment 7 Dan Callaghan 2017-04-19 07:25:40 UTC
Jonathan, do you want to try using $(hostname -f) in /distribution/inventory first?

Fixing it there will be much easier than doing all this extra work on the server side to inject a HOSTNAME parameter.

Comment 8 Jonathan Toppins 2017-04-19 13:21:25 UTC
Hi Dan, I can change to using $(hostname -f). The issue I was seeing was that at least on my box typing in 'hostname -f' still only returned the hostname and not the full fqdn. My thought process here was to allow the inventory collection to work even if the network is not configured as it should be.

Comment 9 Dan Callaghan 2017-04-19 23:37:28 UTC
Okay fair enough. I guess even hostname -f won't work properly if search domains are missing from DHCP or something else like that.

Ultimately the Beaker server knows the "authoritative" FQDN which the system is recorded in the database as. And we always want /distribution/inventory to report back to Beaker using the same hostname so we can match it up. So maybe we should just unconditionally always fill in HOSTNAME parameter for /distribution/inventory? I think no need to add an extra parameter for it.

If you could post a patch to Gerrit that would be great. :-) Let me know if you have any issues using Gerrit and I will help out.

Comment 10 Dan Callaghan 2017-04-19 23:47:34 UTC
(In reply to Dan Callaghan from comment #9)
> I think no need to add an extra parameter for it.

I meant, no need for a new XMLRPC argument or command line option. I think we can just fill in the HOSTNAME parameter unconditionally.

Comment 11 Jonathan Toppins 2017-05-25 22:55:01 UTC
Posted: https://gerrit.beaker-project.org/c/5702/

Comment 14 Dan Callaghan 2017-05-30 07:08:06 UTC
Beaker 24.3 has been released.

Comment 15 Dan Callaghan 2018-03-28 00:33:24 UTC
Turns out this never actually did anything, see bug 1559667.

However the problem is (probably) solved more simply by bug 1346068.


Note You need to log in before you can comment on or make changes to this bug.