Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Expose hostname provided by DHCP in introspection data #190

Closed
russellb opened this issue May 14, 2019 · 17 comments
Closed

Expose hostname provided by DHCP in introspection data #190

russellb opened this issue May 14, 2019 · 17 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@russellb
Copy link
Member

Currently, the network info we have on a BareMetalHost looks like this:

      nics:
      - ip: 172.22.0.86
        mac: 00:5a:10:3f:c2:3d
        model: 0x1af4 0x0001
        name: eth0
        network: Pod Networking
        speedGbps: 0
      - ip: 192.168.111.21
        mac: 00:5a:10:3f:c2:3f
        model: 0x1af4 0x0001
        name: eth1
        network: Pod Networking
        speedGbps: 0

If a hostname is provided by DHCP, I would like to see it as a new field in here.

This is needed by metal3-io/cluster-api-provider-baremetal#49

The issue is that we eventually need all of the addresses that show up on a Node to also be in the Machine status. Right now that includes both IP and hostname. The info we have so far will let us populate the Machine status field with the expected IPs, but not hostname.
@dtantsur
Copy link
Member

This should be an upstream RFE against ironic-python-agent, I guess?

Also, how exactly do you expect this to work? Do any libraries provide such information? Or do you suggest reverse lookup on IPs?

@russellb russellb added the kind/feature Categorizes issue or PR as related to a new feature. label May 21, 2019
@russellb
Copy link
Member Author

A reverse lookup sounds reasonable.

We could also just do the reverse lookup from the baremetal-operator as a quicker change.

@russellb russellb mentioned this issue May 24, 2019
Closed
@dtantsur
Copy link
Member

We could also just do the reverse lookup from the baremetal-operator as a quicker change.

I'd vote for this. We're trying to only collect what's absolutely necessary on the ramdisk side and leave any calculations up to the ironic-inspector side or the consumers.

@dhellmann
Copy link
Member

We could also just do the reverse lookup from the baremetal-operator as a quicker change.

I'd vote for this. We're trying to only collect what's absolutely necessary on the ramdisk side and leave any calculations up to the ironic-inspector side or the consumers.

The operator doesn't need the hostname. Maybe the actuator should do that lookup?

@bcrochet
Copy link
Contributor

metal3-io/cluster-api-provider-baremetal/pull/79

It "works" in that it does the lookup. But no PTR records are present.

@russellb
Copy link
Member Author

We could also just do the reverse lookup from the baremetal-operator as a quicker change.

I'd vote for this. We're trying to only collect what's absolutely necessary on the ramdisk side and leave any calculations up to the ironic-inspector side or the consumers.

The operator doesn't need the hostname. Maybe the actuator should do that lookup?

Well, it's just as useful or not useful to the operator as the IP addresses it includes already.

A downside to having the actuator do it is the differences in network access. The IPs right now include the address on the private provisioning network, which the baremetal-operator can also see. The actuator can't see that. Maybe it doesn't actually affect what DNS results we expect to get, but it feels weird to have the actuator doing this.

@russellb
Copy link
Member Author

metal3-io/cluster-api-provider-baremetal/pull/79

It "works" in that it does the lookup. But no PTR records are present.

Hm, which raises a good point - I don't think we want to introduce any new DNS requirements here.

@bcrochet
Copy link
Contributor

metal3-io/cluster-api-provider-baremetal/pull/79
It "works" in that it does the lookup. But no PTR records are present.

Hm, which raises a good point - I don't think we want to introduce any new DNS requirements here.

Currently under dev-scripts, the reason it doesn't work is because it's trying to resolve the reverse names via 127.0.0.1. Each of the libvirt networks used has their own dnsmasq, which would resolve them, but we'd have to to a lot of work just to find out the proper DNS server to query in order to get the proper PTR record back.

@bcrochet
Copy link
Contributor

A downside to having the actuator do it is the differences in network access. The IPs right now include the address on the private provisioning network, which the baremetal-operator can also see. The actuator can't see that. Maybe it doesn't actually affect what DNS results we expect to get, but it feels weird to have the actuator doing this.

Let me take a look at BMO if there is a difference.

@russellb
Copy link
Member Author

Maybe we're making this more complicated than necessary and a reverse lookup isn't necessary. The introspection agent could always report the system's hostname. Maybe it came from DHCP, maybe it's "localhost", who cares, but it could just always report the system's hostname.

That seems pretty simple and should do what we need. Thoughts?

@dtantsur
Copy link
Member

dtantsur commented Jun 3, 2019

Sounds good and should be trivial to implement.

@bcrochet
Copy link
Contributor

bcrochet commented Jun 3, 2019

Sounds good and should be trivial to implement.

Does that mean you are working on it? I'd be happy to take a crack, just don't want to duplicate effort.

@dtantsur
Copy link
Member

dtantsur commented Jun 3, 2019

No, I won't have cycles for it in the near future. But I'll happily help with reviewing.

@russellb russellb mentioned this issue Jun 5, 2019
Closed
13 tasks
@bcrochet
Copy link
Contributor

bcrochet commented Jun 7, 2019

Patch against IPA: https://review.opendev.org/#/c/663991/

@bcrochet
Copy link
Contributor

#213

@bcrochet
Copy link
Contributor

Latest gophercloud patch: gophercloud/gophercloud/#1627

Once this lands, need to update BMO to the latest gophercloud, then we should be have the hostname populated in the BMH status.

@zaneb
Copy link
Member

zaneb commented Jun 17, 2019

BMO update: #222

@russellb russellb closed this as completed Jul 4, 2019
russellb added a commit to russellb/dev-scripts that referenced this issue Aug 29, 2019
@russellb
Restore fix_certs cron job.
c804817 
The journey of openshift-metal3#260 continues.

This was previously removed because automatic CSR approval was working
for workers.  Automatic CSR approval has stopped working because a
piece of required information (the hostname) is no longer present on
Machine objects.

The hostname is copied to the Machine from the hardware introspection
data on the BareMetalHost.  Ironic was updated to support reporting
the hostname received via DHCP here: https://review.opendev.org/#/c/663991/
More history about adding this to the baremetal-operator is here:
metal3-io/baremetal-operator#190

Ironic was downgraded in OCP 4.2, meaning we lost the change to report
the hostname.  We'll need this cron job until we're able to upgrade
Ironic again.

For more information about automated CSR approval, see:
https://github.com/openshift/cluster-machine-approver/blob/master/README.md

Related issue: openshift-metal3#706
@russellb russellb mentioned this issue Aug 29, 2019
Merged
zaneb pushed a commit to zaneb/baremetal-operator that referenced this issue Nov 24, 2021
@openshift-merge-robot
Merge pull request metal3-io#190 from stbenjam/patch-1
004b494 
Remove stbenjam from OWNERS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@dhellmann @bcrochet @russellb @dtantsur @zaneb