drop volumes full paths #45

enxebre · 2018-10-08T16:45:37Z

Drop full paths in favour of pool and name based look up for volumes
See openshift/machine-api-operator#70

enxebre · 2018-10-08T16:50:25Z

ping @bison @ingvagabund @vikaschoudhary16

bison · 2018-10-08T20:13:21Z

cloud/libvirt/actuators/machine/utils/volume.go

 	}
-	defer volPool.Free()
+	//defer volume.Free()


Do we need to remove this and instead free the returned volumes in each of the calling sites?

paulfantom · 2018-10-10T15:00:15Z

/approve cancel

openshift-ci-robot · 2018-10-10T15:00:28Z

[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: enxebre

If they are not already assigned, you can assign the PR to them by writing /assign @enxebre 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:

OWNERS

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

openshift-bot · 2018-10-11T06:42:43Z

@enxebre: PR needs rebase.

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.

frobware · 2018-10-11T10:01:25Z

cloud/libvirt/actuators/machine/actuator.go

@@ -115,7 +115,7 @@ func createVolumeAndDomain(machine *clusterv1.Machine, offset int) error {
 	}

 	// Create domain
-	if err = libvirtutils.CreateDomain(name, ignKey, name, name, networkInterfaceName, networkInterfaceAddress, autostart, memory, vcpu, offset, client); err != nil {
+	if err = libvirtutils.CreateDomain(name, ignKey, pool, name, name, networkInterfaceName, networkInterfaceAddress, autostart, memory, vcpu, offset, client); err != nil {


Although you've added pool here, why do we pass name, twice?

Although you've added pool here, why do we pass name, twice?

We actually pass it three times ;). CreateDomain allows the domain, volume, and host names to be configured seperately, but we opt to use the same value for all three here.

frobware · 2018-10-11T10:04:13Z

cloud/libvirt/actuators/machine/utils/domain.go

-		if err := setCoreOSIgnition(&domainDef, ignKey); err != nil {
+		ignVolume, err := getVolumeFromPool(ignKey, poolName, virConn)
+		if err != nil {
+			return fmt.Errorf("error getting ignition volume: %v", err)


Add which pool:

error getting ignition from pool %q ...

It will help any subsequent debugging if we know which pool (by name).

Add which pool...

I'd rather handle that in getVolumeFromPool, where all callers can benefit. And at least one of the errors there already includes the pool name.

frobware · 2018-10-11T10:07:02Z

cloud/libvirt/actuators/machine/utils/volume.go

@@ -171,6 +158,17 @@ func CreateVolume(volumeName, poolName, baseVolumeID, source, volumeFormat strin
 		}

 		// create the volume
+		pool, err := virConn.LookupStoragePoolByName(poolName)
+		defer pool.Free()


Can you actually call pool.Free() in the face of an error? Won't pool be nil in those cases?

frobware · 2018-10-11T10:07:47Z

cloud/libvirt/actuators/machine/utils/volume.go

+		// Refresh the pool of the volume so that libvirt knows it is
+		// not longer in use.
+		waitForSuccess("error refreshing pool for volume", func() error {
+			return pool.Refresh(0)


How does this exit? Will it run forever if the pool fails to refresh successfully?

frobware · 2018-10-11T10:08:04Z

cloud/libvirt/actuators/machine/utils/volume.go

+			return pool.Refresh(0)
+		})
+		if err != nil {
+			return fmt.Errorf("can't find storage pool '%s'", poolName)


Use %q for quoting strings.

frobware · 2018-10-11T10:08:56Z

cloud/libvirt/actuators/machine/utils/volume.go


 	// Refresh the pool of the volume so that libvirt knows it is
 	// not longer in use.
-	volPool, err := volume.LookupPoolByVolume()
+	waitForSuccess("error refreshing pool for volume", func() error {
+		return pool.Refresh(0)


Can this run forever?

I'm new to Go/Installer so maybe I'm missing something but pool.Refresh call is blocking and since it does I/O, theoretically it can take a long time before returning. If the call returns an error, it's very unlikely to succeed if called again. So I'm not sure looking over it until it succeeds is the way to go here.

frobware · 2018-10-11T10:09:31Z

cloud/libvirt/actuators/machine/utils/volume.go


+// DeleteVolume removes the volume identified by `key` from libvirt


Comment needs updating; key isn't listed as a parameter.

frobware · 2018-10-11T10:09:55Z

cloud/libvirt/actuators/machine/utils/volume.go

 	// TODO: add locking support
-	//poolName, err := volPool.GetName()
+	//poolName, err := pool.GetName()


Remove dead code.

wking · 2018-10-21T06:57:35Z

cloud/libvirt/actuators/machine/utils/domain.go

-		volumeName = name
+	diskVolume, err := getVolumeFromPool(volumeName, poolName, virConn)
+	if err != nil {
+		return fmt.Errorf("can't retrieve volume %s for pool %s: %v", volumeName, poolName, err)


If getVolumeFromPool becomes consistent about listing the volume and pool in its error messages, we can drop them here.

jcantrill · 2018-11-14T19:41:32Z

👍 I need this as had to move my default location due to limitations in the root partition

openshift-ci-robot · 2018-12-05T04:21:07Z

@enxebre: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/rhel-images	`00b16f1`	link	`/test rhel-images`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

nhosoi · 2019-01-14T17:27:06Z

I need this as had to move my default location due to limitations in the root partition

+100

praveenkumar · 2019-03-08T05:55:00Z

@enxebre ping, Any plan to update it with suggested changes and rebase with current master.

enxebre · 2019-03-08T12:37:59Z

hey @praveenkumar sorry for the delay, we still plan on doing this but libvirt development moves slow due to our priorities pipeline. Any contributions are always welcome :)

praveenkumar · 2019-03-12T07:02:33Z

@zeenix can you take a look to this PR and try to update?

zeenix · 2019-03-12T11:41:19Z

@zeenix can you take a look to this PR and try to update?

Sure. Thanks for pointing it out.

zeenix · 2019-03-28T18:14:56Z

I finally started looking into this. It's not a straight-forward rebase on current master but it's not too difficult either.

zeenix · 2019-04-16T11:44:37Z

People with access rights to do so, please close this in favour of #144

ingvagabund · 2019-04-16T11:56:36Z

Closing per #45 (comment)

drop volumes full paths

00b16f1

openshift-ci-robot requested review from ingvagabund and vikaschoudhary16 October 8, 2018 16:45

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 8, 2018

bison reviewed Oct 8, 2018

View reviewed changes

enxebre added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 9, 2018

openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2018

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2018

frobware requested changes Oct 11, 2018

View reviewed changes

wking reviewed Oct 21, 2018

View reviewed changes

wking mentioned this pull request Jan 5, 2019

openshift-ingress stuck in 'Pending' with "won't update DNS record for load balancer service openshift-ingress/router-default because status contains no ingresses" openshift/installer#973

Closed

This was referenced Jan 12, 2019

libvirt pool path is wrong when it is not default #108

Closed

libvirt: can't create workers with non-default storage path openshift/installer#308

Closed

wking mentioned this pull request Jan 30, 2019

pkg/types/defaults/installconfig: Set defaults for null replicas openshift/installer#1146

Merged

zeenix mentioned this pull request Mar 25, 2019

Use own storage pool openshift/installer#1457

Closed

2 tasks

zeenix mentioned this pull request Apr 16, 2019

Drop volumes full paths #144

Merged

ingvagabund closed this Apr 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drop volumes full paths #45

drop volumes full paths #45

enxebre commented Oct 8, 2018 •

edited

Loading

enxebre commented Oct 8, 2018

bison Oct 8, 2018

paulfantom commented Oct 10, 2018

openshift-ci-robot commented Oct 10, 2018

openshift-bot commented Oct 11, 2018

frobware Oct 11, 2018

wking Oct 21, 2018

frobware Oct 11, 2018

wking Oct 21, 2018

frobware Oct 11, 2018

frobware Oct 11, 2018

frobware Oct 11, 2018

frobware Oct 11, 2018

zeenix Mar 14, 2019

frobware Oct 11, 2018

frobware Oct 11, 2018

wking Oct 21, 2018

jcantrill commented Nov 14, 2018

openshift-ci-robot commented Dec 5, 2018

nhosoi commented Jan 14, 2019

praveenkumar commented Mar 8, 2019

enxebre commented Mar 8, 2019

praveenkumar commented Mar 12, 2019

zeenix commented Mar 12, 2019

zeenix commented Mar 28, 2019

zeenix commented Apr 16, 2019

ingvagabund commented Apr 16, 2019


		// DeleteVolume removes the volume identified by `key` from libvirt

drop volumes full paths #45

drop volumes full paths #45

Conversation

enxebre commented Oct 8, 2018 • edited Loading

enxebre commented Oct 8, 2018

Choose a reason for hiding this comment

paulfantom commented Oct 10, 2018

openshift-ci-robot commented Oct 10, 2018

openshift-bot commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcantrill commented Nov 14, 2018

openshift-ci-robot commented Dec 5, 2018

nhosoi commented Jan 14, 2019

praveenkumar commented Mar 8, 2019

enxebre commented Mar 8, 2019

praveenkumar commented Mar 12, 2019

zeenix commented Mar 12, 2019

zeenix commented Mar 28, 2019

zeenix commented Apr 16, 2019

ingvagabund commented Apr 16, 2019

enxebre commented Oct 8, 2018 •

edited

Loading