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

[WIP] Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT #432

Closed
wants to merge 4 commits into from

Conversation

MalloZup
Copy link
Collaborator

@MalloZup MalloZup commented Sep 27, 2018

What does this PR:

In past we used domain.QemuAgentCommand for getting the Interfaces via qemu-agent.

  • Now we are using domain.ListAllInterfaceAddresses(libvirt.DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE

  • additionally as you can see, we get more simpler codebase with the new function from upstream.

  • for domains which are down we just skip. This was also the behaviour from libvirt-api

We still need a loop function with timeouts because otherwise we might endup in a situation where the QEMU-AGENT is installed but not already setup. So we need to wait and retry for a delay of time.

If we don't have the timeout with the loop we will get at 1st try error: Guest agent is not responding: QEMU guest agent is not connected

@MalloZup MalloZup requested a review from dmacvicar September 27, 2018 16:31
@MalloZup MalloZup changed the title WIP: Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT Sep 27, 2018
@MalloZup MalloZup requested a review from flavio September 28, 2018 10:00
Copy link
Owner

@dmacvicar dmacvicar left a comment

Choose a reason for hiding this comment

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

I still don't see how the agent source is used.

libvirt/domain.go Show resolved Hide resolved
libvirt/domain.go Show resolved Hide resolved
Copy link
Collaborator Author

@MalloZup MalloZup left a comment

Choose a reason for hiding this comment

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

OK everything is ok now

libvirt/domain.go Show resolved Hide resolved
@MalloZup MalloZup changed the title Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT WIP: Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT Oct 3, 2018
@MalloZup MalloZup changed the title WIP: Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT Oct 4, 2018
@MalloZup MalloZup changed the title Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT WIP Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT Oct 5, 2018
@MalloZup MalloZup changed the title WIP Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT WIP: Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT and libvirt events Oct 5, 2018
@MalloZup MalloZup changed the title WIP: Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT and libvirt events WIP: Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT Oct 11, 2018
Repository owner deleted a comment from dmacvicar Oct 11, 2018
@MalloZup MalloZup force-pushed the qemu-agent-ref branch 2 times, most recently from a16a1c2 to 7a0b188 Compare October 11, 2018 12:13
@MalloZup MalloZup changed the title WIP: Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT Oct 11, 2018
@dmacvicar
Copy link
Owner

@MalloZup can you please post an example of the configuration that would fail? (no need for it to be real, just an example, to understand it better)

@MalloZup MalloZup changed the title Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT [WIP] Use upstream DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT Nov 9, 2018
@MalloZup
Copy link
Collaborator Author

MalloZup commented Nov 9, 2018

i prefer to concetrate on other issues which will bring more value.
I think this feature would be a nice refactor but i am unsure about the qemu-agent, since at moment we don't tests it in testacc so we cannot assume this could bring a regression. ( all the tests i did were ok but i am maybe missing some complexity). So in this case i prefer to keep the old code that works instead of this refactor. ( which is nice but add some risk without a real gain of feature/values)

@MalloZup MalloZup closed this Nov 9, 2018
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.

3 participants