Skip to content

Add IP address sync for KubeVirt compute resource#174

Closed
archanaserver wants to merge 1 commit intotheforeman:masterfrom
archanaserver:sync-ip-address
Closed

Add IP address sync for KubeVirt compute resource#174
archanaserver wants to merge 1 commit intotheforeman:masterfrom
archanaserver:sync-ip-address

Conversation

@archanaserver
Copy link
Contributor

added :ip => :ip_addresses to provided_attributes to enable
foreman's orchestration to sync IPs from VMs.

also, this fix works in conjunction with network name sanitization (PR #172 )

@archanaserver archanaserver marked this pull request as draft January 13, 2026 11:53
@archanaserver archanaserver marked this pull request as ready for review January 15, 2026 08:39
@ShimShtein ShimShtein self-assigned this Jan 19, 2026
@ShimShtein
Copy link
Member

Importing a VM currently throws an error:

11:20:40 rails.1   |  26b78dd2 | NoMethodError (undefined method `ip_address' for   <Fog::Kubevirt::Compute::VmNic
11:20:40 rails.1   |  26b78dd2 |     mac_address="[redacted]",
11:20:40 rails.1   |  26b78dd2 |     type="bridge",
11:20:40 rails.1   |  26b78dd2 |     model=nil,
11:20:40 rails.1   |  26b78dd2 |     ports=nil,
11:20:40 rails.1   |  26b78dd2 |     boot_order=nil,
11:20:40 rails.1   |  26b78dd2 |     network="[redacted]",
11:20:40 rails.1   |  26b78dd2 |     cni_provider="multus"
11:20:40 rails.1   |  26b78dd2 |   >:Fog::Kubevirt::Compute::VmNic
11:20:40 rails.1   |  26b78dd2 | Did you mean?  ip_addresses):
11:20:40 rails.1   |  26b78dd2 |   
11:20:40 rails.1   |  26b78dd2 | /home/vagrant/foreman_kubevirt/app/models/concerns/fog_extensions/kubevirt/server.rb:34:in `ip_addresses'
11:20:40 rails.1   |  26b78dd2 | app/services/compute_resource_host_importer.rb:25:in `public_send'
11:20:40 rails.1   |  26b78dd2 | app/services/compute_resource_host_importer.rb:25:in `block in copy_vm_attributes'
11:20:40 rails.1   |  26b78dd2 | app/services/compute_resource_host_importer.rb:24:in `each'
11:20:40 rails.1   |  26b78dd2 | app/services/compute_resource_host_importer.rb:24:in `copy_vm_attributes'
11:20:40 rails.1   |  26b78dd2 | app/services/compute_resource_host_importer.rb:16:in `initialize'
11:20:40 rails.1   |  26b78dd2 | app/controllers/compute_resources_vms_controller.rb:94:in `new'
11:20:40 rails.1   |  26b78dd2 | app/controllers/compute_resources_vms_controller.rb:94:in `import'


def provided_attributes
{ :uuid => :name, :mac => :mac }
{ :uuid => :name, :mac => :mac, :ip => :ip_addresses }
Copy link
Member

@evgeni evgeni Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly off topic: other CRs use super.merge({the: hash}): https://github.com/search?q=org%3Atheforeman%20%22def%20provided_attributes%22&type=code

Should we do the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes def

Comment on lines 35 to 37
def ip_addresses
[interfaces&.first&.ip_address]
[ip_address].compact
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two questions:

  1. we only need one address, so can't we use the ip_address attribute directly?
  2. in my tests (on our internal CNV cluster), the attribute returned an IPv6 address, does foreman cope with that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2: nope, I got a

2026-01-20T10:20:39 [I|app|b34e2035] Backtrace for 'Rolling back due to exception during save' error (ActiveRecord::ValueTooLong): PG::StringDataRightTruncation: ERROR:  value too long for type character varying(15)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With :ip6 => :ip_address it worked (but I guess we should rather have some methods that return the address only of the right type), but then the VM deployment was blocked until I manually booted the VM in CNV as in https://github.com/theforeman/foreman/blob/47772af31368013686e28b9f626aca26af241895/app/models/concerns/orchestration/compute.rb#L63-L84 the "acquire ip" step happens before the "power on" step, but it seems a VM in CNV doesn't have an IP before it wasn't powered on?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def ip
  Net::Validations.validate_ip(ip_address) ? ip_address : nil
end

def ip6
  Net::Validations.validate_ip6(ip_address) ? ip_address : nil
end

and then in provided_attributes: :ip => :ip, :ip6 => :ip6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With :ip6 => :ip_address it worked (but I guess we should rather have some methods that return the address only of the right type), but then the VM deployment was blocked until I manually booted the VM in CNV as in https://github.com/theforeman/foreman/blob/47772af31368013686e28b9f626aca26af241895/app/models/concerns/orchestration/compute.rb#L63-L84 the "acquire ip" step happens before the "power on" step, but it seems a VM in CNV doesn't have an IP before it wasn't powered on?

Ah so the timing issue you mentioned (IP acquisition before power on) is seems interesting to me. In my testing with pod network specifically, VM started automatically (I set "start" => "1" in create_vm), so it had an IP by the time orchestration checked.

But you're right that if the VM isn't auto-started, orchestration might try to get IP before the VM is running. Thisi believe seems like a broader orchestration issue for KubeVirt (not specific to this PR), but worth noting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I see we inherently can't use the provided_attributes since Kubevirt does not give an IP address before the machine boots.
Maybe we should change the task execution order, or add another step in the orchestration. But the current solution won't really work.

I propose closing this PR, and getting back to the drawing board to understand how and when we should get an IP address from the compute resource.

CC @ekohl

Copy link
Member

@ekohl ekohl Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Foreman's compute orchestration: https://github.com/theforeman/foreman/blob/3ec941eff26ef521ecf1bd2b63cc364a13bd79a9/app/models/concerns/orchestration/compute.rb#L63-L84

The bit that receives the IP address (https://github.com/theforeman/foreman/blob/3ec941eff26ef521ecf1bd2b63cc364a13bd79a9/app/models/concerns/orchestration/compute.rb#L70-L73) is before the machine is actually turned on (https://github.com/theforeman/foreman/blob/3ec941eff26ef521ecf1bd2b63cc364a13bd79a9/app/models/concerns/orchestration/compute.rb#L80-L83). Note priority 4 vs priority 1000. It can even be guarded by :start to not even start the VM up.

What you can do is introduce new provided attributes like :late_ip & :late_ip6 and queue a task after the VM is started:

if compute_attributes && compute_attributes[:start] == '1'
  queue.create(:name   => _("Power up compute instance %s") % self, :priority => 1000,
    :action => [self, :setComputePowerUp])

  if compute_provides?(:late_ip) || compute_provides?(:late_ip6)
    queue.create(:name => _("Acquire IP addresses for %s") % self, :priority => 1001,
      :action => [self, :setLateComputeIP])
  end
end

I'm not sure if 1001 is the right priority, just some thoughts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evgeni asked what value this brings and that was an excellent point. Having priority 1001 means it can't be used to create DNS records, which has priority 10 (https://github.com/theforeman/foreman/blob/3ec941eff26ef521ecf1bd2b63cc364a13bd79a9/app/models/concerns/orchestration/dns.rb#L63-L71).

Realistically, I don't think KubeVirt can provide the IP address and we can instead rely on fact reporting like Puppet, subman, or Ansible already.

@ekohl
Copy link
Member

ekohl commented Jan 29, 2026

Because of #174 (comment) I don't think KubeVirt can provide the attribute at all and I'm closing this as something we can't do.

@ekohl ekohl closed this Jan 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants