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.

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

Report hostname using dhcp provider hostname for a domain #125

Closed
wants to merge 1 commit into from

Conversation

praveenkumar
Copy link
Contributor

In case of libvirt dom.GetHostname(0) will not provide the hostname info since it need qemu-guest-agent installed on the VM to fetch this info https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetHostname Since for libvirt case we are providing the hostname using libvirt provided dhcp and assign same to domain name. We can reuse it here also instead adding another package to RHCOS image.

cc @abhinavdahiya @zeenix

In case of libvirt dom.GetHostname(0) will not provide the hostname info since it need qemu-guest-agent
installed on the VM to fetch this info https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetHostname
Since for libvirt case we are providing the hostname using libvirt provided dhcp and assign same to domain name
We can reuse it here also instead adding another package to RHCOS image.
@openshift-ci-robot
Copy link
Contributor

Hi @praveenkumar. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 15, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: vikaschoudhary16

If they are not already assigned, you can assign the PR to them by writing /assign @vikaschoudhary16 in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@@ -465,7 +465,11 @@ func NodeAddresses(dom *libvirt.Domain) ([]corev1.NodeAddress, error) {
}
}

hostname, err := dom.GetHostname(0)
// In case of libvirt dom.GetHostname(0) will not provide the hostname info since it need qemu-guest-agent
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: libvirt mention is redundant here.

Copy link
Contributor

Choose a reason for hiding this comment

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

based on the context, i mean.

@zeenix
Copy link
Contributor

zeenix commented Mar 15, 2019

I tested this via the image you provided (docker.io/kumarpraveen/capl:csr_issue) and it at least fixes the TLS errors I have recently been getting.

@cynepco3hahue
Copy link
Contributor

IMHO the right solution is to use information from DHCP leases that libvirt provides via virsh net-dhcp-leases.

@bison
Copy link
Contributor

bison commented Mar 18, 2019

IMHO the right solution is to use information from DHCP leases that libvirt provides via virsh net-dhcp-leases.

I agree. Does #128 alone solve the immediate problem? If so, I'd vote for that.

@praveenkumar
Copy link
Contributor Author

I agree. Does #128 alone solve the immediate problem? If so, I'd vote for that.

@bison @cynepco3hahue So I don't understand why we need to get the hostname from the dhcp lease since we know them already before creating those VM on libvirt? Do you folks thinks that there might be a situation where name which provided by the dhcp and part of the domain can be differ from what we get from the dhcp lease?

@zeenix
Copy link
Contributor

zeenix commented Mar 18, 2019

o I don't understand why we need to get the hostname from the dhcp lease since we know them already before creating those VM on libvirt?

Because it's more future-proof as we don't assume a direct relationship between hostname and name of the VM.

@praveenkumar
Copy link
Contributor Author

closing this in favour of #128

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants