Set hostname for dhcp networks #714

pablochacin · 2020-03-05T21:15:24Z

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.

flavio

Good work. The whole code is needs a lot of refactoring to clean it up, but this is something that should be done later, with a dedicated set or PRs.

Some overall comments/requests:

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 preferred to leave the deallocation of the network instances to the caller of the setNetworkInterfaces function. Yes, we can build on top of the GC offered by the libvirt library (using the Ref() method), but I would prefer not to do that because 1) I don't know how much we can trust that to work 2) IMHO it makes the code less idiomatic

Thanks a lot for your fixes, I know it has been a hard one 👏

libvirt/domain.go

libvirt/resource_libvirt_domain.go

pablochacin · 2020-03-06T13:21:46Z

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 · 2020-03-06T14:16:05Z

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

flavio · 2020-03-09T09:29:16Z

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 · 2020-03-09T11:43:44Z

@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 · 2020-03-10T08:28:10Z

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

MalloZup · 2020-03-10T08:37:08Z

libvirt/resource_libvirt_domain_test.go


 	resource.Test(t, resource.TestCase{
 		PreCheck:     func() { testAccPreCheck(t) },
 		Providers:    testAccProviders,
 		CheckDestroy: testAccCheckLibvirtDomainDestroy,
 		Steps: []resource.TestStep{
 			{
-				Config: config,
+				Config:             config,
+				ExpectNonEmptyPlan: false, // TODO: find why if set to true, the test fails


we might need to check this before mergin because it could be a code smell of regression.

same also the Destroy I think we never used in Testacc

I agree. I was unable to identify the reason, except that before the plan had the hostname but after the apply, it was empty. Terraform complained about this. I pointed this out in the issue #708 (comment)

Actually, not expecting nonempty plans is good:

Expecting errors or non-empty plans:

It is possible for scenarios to exist where a valid configuration (no errors during plan) would result in a non-empty plan after successfully running terraform apply. This is typically due to a valid but otherwise misconfiguration of the resource, and is generally undesirable.

MalloZup · 2020-03-10T08:47:54Z

@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. 😁

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.. 🌞

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 Signed-off-by: Pablo Chacin <pchacin@suse.com>

pablochacin · 2020-03-10T09:52:25Z

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

This looks so much better!

flavio · 2020-03-11T08:41:59Z

@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

thx!

MalloZup · 2020-03-11T13:08:16Z

@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 🌻 🎸

MalloZup · 2020-03-11T13:10:32Z

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 😁 )

PR dmacvicar#714 introduced spurious changes in the test configuration for `resource_libvirt_domain_test.go`. The added `Destroy` parameter has no effect and the comment regarding the `ExpectNonEmptyPlan` are outdated. dmacvicar#714 Signed-off-by: Pablo Chacin <pchacin@suse.com>

pablochacin · 2020-03-16T12:01:56Z

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

MalloZup · 2020-03-17T10:20:36Z

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

pablochacin force-pushed the dhcp-hostname branch 2 times, most recently from ee36ca0 to 96bc263 Compare March 6, 2020 08:36

MalloZup self-requested a review March 6, 2020 11:31

flavio requested changes Mar 6, 2020

View reviewed changes

libvirt/domain.go Outdated Show resolved Hide resolved

libvirt/domain.go Outdated Show resolved Hide resolved

libvirt/domain.go Show resolved Hide resolved

libvirt/domain.go Outdated Show resolved Hide resolved

libvirt/resource_libvirt_domain.go Outdated Show resolved Hide resolved

MalloZup reviewed Mar 10, 2020

View reviewed changes

pablochacin force-pushed the dhcp-hostname branch from 96bc263 to aeec698 Compare March 10, 2020 09:48

flavio approved these changes Mar 11, 2020

View reviewed changes

MalloZup approved these changes Mar 11, 2020

View reviewed changes

MalloZup merged commit da06479 into dmacvicar:master Mar 11, 2020

pablochacin mentioned this pull request Mar 16, 2020

Remove sporious test changes #716

Merged

MalloZup mentioned this pull request Sep 21, 2020

libvirt doesn't set the correct network hostname on the first run SUSE/ha-sap-terraform-deployments#197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set hostname for dhcp networks #714

Set hostname for dhcp networks #714

pablochacin commented Mar 5, 2020

flavio left a comment

pablochacin commented Mar 6, 2020

MalloZup commented Mar 6, 2020

flavio commented Mar 9, 2020

pablochacin commented Mar 9, 2020 •

edited

flavio commented Mar 10, 2020

MalloZup Mar 10, 2020

pablochacin Mar 10, 2020

pablochacin Mar 10, 2020 •

edited

MalloZup commented Mar 10, 2020

pablochacin commented Mar 10, 2020 •

edited

flavio left a comment

flavio commented Mar 11, 2020

MalloZup left a comment

MalloZup commented Mar 11, 2020

MalloZup commented Mar 11, 2020 •

edited

pablochacin commented Mar 16, 2020

MalloZup commented Mar 17, 2020

Set hostname for dhcp networks #714

Set hostname for dhcp networks #714

Conversation

pablochacin commented Mar 5, 2020

flavio left a comment

Choose a reason for hiding this comment

pablochacin commented Mar 6, 2020

MalloZup commented Mar 6, 2020

flavio commented Mar 9, 2020

pablochacin commented Mar 9, 2020 • edited

flavio commented Mar 10, 2020

MalloZup Mar 10, 2020

Choose a reason for hiding this comment

pablochacin Mar 10, 2020

Choose a reason for hiding this comment

pablochacin Mar 10, 2020 • edited

Choose a reason for hiding this comment

MalloZup commented Mar 10, 2020

pablochacin commented Mar 10, 2020 • edited

flavio left a comment

Choose a reason for hiding this comment

flavio commented Mar 11, 2020

MalloZup left a comment

Choose a reason for hiding this comment

MalloZup commented Mar 11, 2020

MalloZup commented Mar 11, 2020 • edited

pablochacin commented Mar 16, 2020

MalloZup commented Mar 17, 2020

pablochacin commented Mar 9, 2020 •

edited

pablochacin Mar 10, 2020 •

edited

pablochacin commented Mar 10, 2020 •

edited

MalloZup commented Mar 11, 2020 •

edited