Set hostname for dhcp networks

[pablochacin]

For dhcp networks, ensure the hostname is set when the ip address is allocated.

Unify behavior when network is referenced either by name or by id.

Fix bug using network after it was freed

Improve test for covering new behavior

Fixes #708

Please make sure you read the contributor documentation before opening a Pull Request.

[pablochacin]

• Please do not conflate all the changes into a single commit. You're solving different problems with the same PR (which is acceptable), but you should have multiple commits (eg: the fixes about the network resource being released twice should be part of a different commit)

I tend to do this except that in this case, without this change, the refactoring for setting the hostname doesn't work. The test fails. I generally don't like to make commits that don't work.

On the other hand, the way the reference to the network is freed depends on the re-organization of the code introduced. I could make a fix for that under the actual code structure, but that change will be completely lost in the second commit.

[MalloZup]

thx @flavio and @pablochacin for this. i will have a look to this PR also next week

[flavio]

To elaborate more on why I'm not so convinced about using the Ref() method.

When dealing with resource cleanup a Go developer doesn't have a double garbage collector running like in this case, where we have Go's GC and the one implemented inside of libvirt.

When dealing with resources like a database object a Go developer has still to take care of invoking some finalization code before Go's GC takes the object instance out of reach. The developer does that by leveraging the defer construct and by taking a close look at how return statements are used.

I would prefer to ignore the Ref() method and take a more "idiomatic" approach to this problem. I would also, with a later series of PRs, refactor this portion of code to make it more manageable. Right now I admit that understanding when to do a defer/Free/nothing at all is complicated, but that's caused by the incredible length and branching we have in this area of the codebase.

[pablochacin]

@flavio I gave a try to your proposal and the resulting code is extremely convoluted and hard to maintain because it must ensure it will work on every possible code execution path. It is a lot of changes for something that we expect to solve by refactoring the code.

One alternative could be to change the pendingMapping structure and pass the network name or ID instead of the reference to the network and then whoever takes care of these pending mappings will obtain and release the network reference from this name or ID. I think this is a tradeoff between a non-idiomatic solution using Ref and refactoring the code (it is a minor change in the code)

If you disagree with this alternative, then do you want to include this refactoring in the present PR? I would prefer not, but this is the only alternative to introducing multiple Free in the code, I prefer to refactor the code.

[flavio]

Let's go ahead with the Ref() approach for now. We can try to get rid of them with a future PR

[MalloZup]

@pablochacin @flavio regarding the Ref or not Ref approach.

I'm fine with both approaches, BUT please add some comment on the design.

I remember to have looked in the past how we process networks and until you don't dive in actively in the code, you can't understand ( the code is also pretty complicated for good and bad reasons).

So if we can have comments explaining how we use the REF and why, this will be appreciate, because I have already forgotten that part of code. :grin:

A part of this, I think we might check 2 times the testacc because imho we are modifying them to pass in an usual way and this could be there is a regression since we need to modify them to make them pass see my comments.

Thx sofar.. :sun_with_face:

[pablochacin]

I have updated the PR. I think I've addressed all the concerns except the change in the test, as @MalloZup pointed out.

I finally changed the code not to pass a reference to the network in the structure of the pending interface, but the network name. I think this way we delegate to the routine that handles the pending interfaces when to acquire an release the network reference.

[flavio]

@MalloZup I would be fine approving this PR. What do you think?

Also, once merged, how can we tag a new release including this fix?

[MalloZup]

@flavio ok I will prepare a new tag including this fix. Then we can build the RPM . @dmacvicar is ok for you at this point to do a new release? I think we have a release draft but is more a sketch. I will need to go trough the PR merged and summarize them.

Sofar thx lot for helping and contributing. Looking forward :sunflower: :guitar:

[MalloZup]

see https://github.com/dmacvicar/terraform-provider-libvirt/releases/tag/untagged-30bccf4e7c1526ff1438 but is not complete.

If you want to help feel free @flavio , otherwise I will do it during the day.

@pablochacin @flavio if you want a RPM package from master you could use the unstable repo just trigger it from the service.

If you wanna right there feel free to ask via obs I will accept.

https://build.opensuse.org/package/show/systemsmanagement:terraform:unstable/terraform-provider-libvirt

otherwise for the "official" normally the process is to :

• create release draft
• release it
• adjust OBS pkgs to point to release version and build them.
• announcement on social medias etc ( this is done by @dmacvicar since he write more convincing English then me :grin: )

[pablochacin]

Thanks a lot, @MalloZup. Not sure what can/should/must do here.

[MalloZup]

we need to release a new version. I will make it happen