Project

General

Profile

Bug #19990

Failed to update a compute vmware (VMware) instance node.test.domain: undefined method `[]' for nil:NilClass

Added by Christian Meißner about 1 year ago. Updated about 1 month ago.

Status:
Closed
Priority:
Normal
Category:
Compute resources - VMware
Target version:
Team Backlog:
Marek
Fixed in Releases:
Found in Releases:

Description

If I try to update the environment of a machine in a vmware compute instance i get the above error.

I attached the sanitized debug output of the following command.

hammer -u user -p pass -d host update --id 127 --environment production

If I use --environment-id instead of --environment it leads to the same error.

Other actions for example start and stop of a virtual machine works fine.

hammer_cli_debug.log hammer_cli_debug.log 6.62 KB Christian Meißner, 06/13/2017 05:00 AM
production.log production.log 23.3 KB Christian Meißner, 06/14/2017 01:10 AM
production.log production.log 472 KB Steven Miller, 06/15/2017 03:02 PM
guiheader.txt guiheader.txt 3.78 KB Header sent when submitted request to create new host Steven Miller, 06/15/2017 03:02 PM
foremanpost-smiller-20180119.txt foremanpost-smiller-20180119.txt 2.83 KB Steven Miller, 01/19/2018 02:25 PM

Related issues

Copied to Foreman - Bug #23752: VMWare allow save vm w/o size_gb attr (1.16.2)Closed2018-05-31

Associated revisions

Revision 1dbb2c5b (diff)
Added by Steve Miller 3 months ago

Fixes #19990 - VMWare allow save vm w/o size_gb attr

History

#1 Updated by Tomáš Strachota about 1 year ago

Hello Christian,
thank you for reporting the issue. Attached hammer logs show that the error occurs on the server side. May I ask you to attach relevant part of foreman logs too?
If you try to perform the same action in the UI, does it fail with the same error?

#2 Updated by Christian Meißner about 1 year ago

Tomáš Strachota wrote:

Hello Christian,
thank you for reporting the issue. Attached hammer logs show that the error occurs on the server side. May I ask you to attach relevant part of foreman logs too?

Hi Tomas attached you can find the cut off of our production.log.

If you try to perform the same action in the UI, does it fail with the same error?

No, in the UI it works pretty good.

#3 Updated by Tomáš Strachota about 1 year ago

  • Project changed from Hammer CLI to Foreman
  • Category changed from Hosts to Compute resources - VMware

I'm switching the component to VMWare as this is a server side API issue. The compute node orchestration fails on update via API because it misses some compute attribute data. UI works fine because it loads the compute attributes into the form when the page is rendered and then when user hits update the request is sent with required parameters. In API the only workaround is to send the fill set of current compute attributes with the update request.

#4 Updated by Tomáš Strachota about 1 year ago

  • Target version set to 115

#5 Updated by Tomáš Strachota about 1 year ago

Most likely introduced with https://github.com/theforeman/foreman/pull/4146/
oVirt is probably affected too.

#6 Updated by Marek Hulán about 1 year ago

  • Legacy Backlogs Release (now unused) set to 266

I suppose this is a candidate for 1.15.2 then. Please reset the Release if you disagree.

#7 Updated by Steven Miller about 1 year ago

I am seeing the same bug on this provider, even when provisioning a new system from the GUI. I've attached my production logs, along with the final form data that is submitted when attempting to create a host. On the VMware side the actual VM is created, but then when it hits the "undefined method" error it rolls back the VMware machine (but not the record within foreman)

#8 Updated by Daniel Lobato Garcia about 1 year ago

  • Legacy Backlogs Release (now unused) changed from 266 to 276

#9 Updated by Daniel Lobato Garcia about 1 year ago

  • Legacy Backlogs Release (now unused) changed from 276 to 287

#10 Updated by Marek Hulán 11 months ago

Tomas, why do you think it was introduced by the linked PR? According to the trace, it seems that compute_attrs for a given VM are nil. Based on incoming parameters, they are part of API request. I don't see how attributes can become nil so far

Parameters: {"host"=>{"environment_id"=>20, "compute_attributes"=>{"volumes_attributes"=>{}}, "host_parameters_attributes"=>{}, "interfaces_attributes"=>{}}, "apiv"=>"v2", "id"=>"119"}

#11 Updated by Tomáš Strachota 11 months ago

Marek, the issue has actually been there even before the commit I mentioned, but this change eventually exposed the buggy code to users:
https://github.com/theforeman/foreman/pull/4146/files#diff-4443c0de1e4e867a30d3c878b9cb64e9R24

Before that it wasn't possible to update attribute of existing VMs on vmWare. From the traceback, the process fails on:
https://github.com/theforeman/foreman/blob/c952c782adac290fd5a1145090380424665f5fe1/app/models/compute_resources/foreman/model/vmware.rb#L434
(please note that the bug was reported against the foreman 1.15 and line numbers have been shifted since then)

So the issue isn't caused by compute_attributes being nil, but by volume attributes that can't be found. The code isn't prepared for partial updates and always requires the full set of compute attributes (including volumes and nics). The reason it works in the UI is that the host edit form pre-loads current compute attributes and therefore the attributes are complete.

#12 Updated by Marek Hulán 11 months ago

  • Legacy Backlogs Release (now unused) deleted (287)

Now that makes more sense, thanks for clarification. I suppose this is API RFE then, removing from 1.15.4

#13 Updated by Bhanu Prasad Ganguru 10 months ago

Hi, we recently updated foreman to 1.15.6
And still seeing the same issue
is there a solution for this issue?

Bhanu

#15 Updated by Adam Winberg 9 months ago

Im not using hammer, but the workaround I use when using the API is to first do a GET to get the nodes 'vm_compute_attributes' and then attach that result to my PUT operation on the node.

In perl (just to get the idea):

  my $client = REST::Client->new();

  $client->setHost($foreman_host);

  # Get compute attributes, bug workaround: 
  # http://projects.theforeman.org/issues/19990
  $client->GET("/api/hosts/$fqdn/vm_compute_attributes", $headers);

  my $compute_attrs = decode_json($client->responseContent());
  # Set the attributes for a new host
  my %json_data = (
    host => {
      comment => "Updating the comment attribute",
      compute_attributes => $compute_attrs,
    }
  );

  # Translate the perl hash in json notation
  my $data = encode_json(\%json_data);

  # Update parameter
  $client->PUT("/api/hosts/$fqdn",($data,$headers));

I'm not using hammer though...

#16 Updated by Steven Miller 7 months ago

I recently upgraded my environment to 1.16.0 and still found the issue. I was able to eliminate the issue by commenting out the following line in line 457 of the app/models/compute_resources/foreman/model/vmware.rb (the line in the 1.16.0 codebase). I may submit a pull request at some point, but want to do a little more research to make sure this doesn't break anything unexpected. For us this fixes compute creating through the UI and API.

    450     def save_vm(uuid, attr)
    451       vm = find_vm_by_uuid(uuid)
    452       vm.attributes.merge!(attr.deep_symbolize_keys)
    453       #volumes are not part of vm.attributes so we have to set them seperately if needed
    454       if attr.has_key?(:volumes_attributes)
    455         vm.volumes.each do |vm_volume|
    456           volume_attrs = attr[:volumes_attributes].values.detect {|vol| vol[:id] == vm_volume.id}
    457           #vm_volume.size_gb = volume_attrs[:size_gb]
    458         end
    459       end
    460       vm.save
    461     end

#17 Updated by Steven Miller 7 months ago

Forgot to mention that it appears the UI is passing the parameter "sizeGb" to specify volume size. It appear the older, non-React UI code still uses that parameter, but the newer view code does not...maybe...

#18 Updated by Marek Hulán 7 months ago

  • Priority changed from High to Normal
  • Status changed from New to Need more information

The patch would disable the volume size adjusting. What I did not fully understand was the last comment, do you say that you are now able to reproduce this bug even using UI? The new react implementation work the same way.

#19 Updated by Steven Miller 7 months ago

The patch I submitted is the only way we could foreman to work. The hard drive size is still set correctly, even with this line completely commented out. I have a more sophisticated patch that will check if size_gb is set, and if it is not, then just skip the assignment (line 457 in the example).

In our 1.16 install, the "size_gb" parameter is never passed through the web interface, and seems to break at this line. I've attached "foremanpost-smiller-20180119.txt", which contains all the POSTed variables from a web browser doing a VMware deployment (on a 1.16 install). I see that both "size" and "SizeGB" are set within the host[compute_attributes][scsi_controllers] attribute. I don't see a size_gb set on the UI end at least.

#20 Updated by Dmitry Okun 4 months ago

Steven Miller wrote:

The patch I submitted is the only way we could foreman to work. The hard drive size is still set correctly, even with this line completely commented out. I have a more sophisticated patch that will check if size_gb is set, and if it is not, then just skip the assignment (line 457 in the example).

In our 1.16 install, the "size_gb" parameter is never passed through the web interface, and seems to break at this line. I've attached "foremanpost-smiller-20180119.txt", which contains all the POSTed variables from a web browser doing a VMware deployment (on a 1.16 install). I see that both "size" and "SizeGB" are set within the host[compute_attributes][scsi_controllers] attribute. I don't see a size_gb set on the UI end at least.

I was also hitting the same error when attempting to provision a VMware VM. Moreover, my VMs were created and right away deleted via foreman. By commenting these lines out, I was able to create VMs via Foreman

#21 Updated by Steven Miller 3 months ago

  • Pull request https://github.com/theforeman/foreman/pull/5577 added

Created a pull request for a slightly more sophisticated version of my fix. If volume_attrs is present as a has it will attempt to set size_gb, otherwise it will skip the assignment. Internal testing seems to work for us (both UI and API usage)

#22 Updated by The Foreman Bot 3 months ago

  • Status changed from Need more information to Ready For Testing

#23 Updated by Tomer Brisker 3 months ago

  • Bugzilla link set to 1551014

#24 Updated by The Foreman Bot 3 months ago

  • Assignee set to Ondřej Pražák
  • Pull request https://github.com/theforeman/foreman/pull/5626 added

#25 Updated by The Foreman Bot 3 months ago

  • Pull request https://github.com/theforeman/foreman/pull/5627 added

#26 Updated by Daniel Lobato Garcia 3 months ago

  • Legacy Backlogs Release (now unused) set to 330

#27 Updated by Daniel Lobato Garcia 3 months ago

  • Copied to Bug #23752: VMWare allow save vm w/o size_gb attr (1.16.2) added

#28 Updated by Anonymous 3 months ago

  • % Done changed from 0 to 100
  • Status changed from Ready For Testing to Closed

Also available in: Atom PDF