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

Add devicemapper GC for persistent rootfs #771

Merged
merged 1 commit into from
Oct 3, 2018
Merged

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented Sep 27, 2018

Upon GC at the Virtlet startup, the virtual block devices that are
created by Virtlet are recognized by name and the sector 0 of underlying
block device (Virtlet magic header). If they're not related to any
active domain, they're removed (note that this doesn't mean the data on
the device is affected in any way).


This change is Reviewable

@ivan4th ivan4th force-pushed the ivan4th/devicemapper-gc branch from ff4a93e to 9bd67ed Compare September 27, 2018 18:51
Upon GC at the Virtlet startup, the virtual block devices that are
created by Virtlet are recognized by name and the sector 0 of underlying
block device (Virtlet magic header). If they're not related to any
active domain, they're removed (note that this doesn't mean the data on
the device is affected in any way).
@ivan4th ivan4th force-pushed the ivan4th/devicemapper-gc branch from 9bd67ed to 57337af Compare September 27, 2018 18:54
Copy link
Contributor

@jellonek jellonek left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r1.
Reviewable status: 0 of 2 approvals obtained (waiting on @ivan4th)


pkg/libvirttools/gc.go, line 275 at r1 (raw file):

		idsInUse[id] = true
	}
	ldh := blockdev.NewLogicalDeviceHandler(v.Commander(), devPath, sysfsPath)

Maybe this could be done during creation of VirtualizationTool once, stored in its structure, then could be used there and also could be returned by devHandler?

Copy link
Contributor

@lukaszo lukaszo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 change requests, 0 of 2 approvals obtained (waiting on @ivan4th)


pkg/libvirttools/gc.go, line 284 at r1 (raw file):

	for _, dmName := range dmNames {
		if !strings.HasPrefix(dmName, blockdev.VirtletLogicalDevicePrefix) {
			panic("bad dmname " + dmName)

why panic?

Copy link
Contributor

@jellonek jellonek left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 change requests, 0 of 2 approvals obtained (waiting on @ivan4th)


pkg/libvirttools/gc.go, line 284 at r1 (raw file):

Previously, lukaszo (Łukasz Oleś) wrote…

why panic?

I'm reading that ListVirtletLogicalDevices returned a device which has virtlet magic but wrong name. This should never happened, unless there was breaking api virtlet pod upgrade which would include change of virtlet logical device prefix.

Copy link
Contributor

@lukaszo lukaszo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 change requests, 0 of 2 approvals obtained (waiting on @ivan4th)


pkg/libvirttools/gc.go, line 284 at r1 (raw file):

Previously, jellonek (Piotr Skamruk) wrote…

I'm reading that ListVirtletLogicalDevices returned a device which has virtlet magic but wrong name. This should never happened, unless there was breaking api virtlet pod upgrade which would include change of virtlet logical device prefix.

It's gc. I think that device can just be skipped with a big warning message.

Copy link
Contributor Author

@ivan4th ivan4th left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 change requests, 0 of 2 approvals obtained (waiting on @ivan4th and @jellonek)


pkg/libvirttools/gc.go, line 275 at r1 (raw file):

Previously, jellonek (Piotr Skamruk) wrote…

Maybe this could be done during creation of VirtualizationTool once, stored in its structure, then could be used there and also could be returned by devHandler?

I thought about this. What stopped me from this approach is that with gc.go being a set of VirtualizationTool methods, VirtualizationTool itself becomes an instance of "God Object" antipattern. Because of this, I'd prefer not to add extra coupling between GC methods and other parts of VirtualizationTool.


pkg/libvirttools/gc.go, line 284 at r1 (raw file):

Previously, lukaszo (Łukasz Oleś) wrote…

It's gc. I think that device can just be skipped with a big warning message.

panic is used here because ListVirtletLogicalDevices should only return Virtlet-related devices that MUST include the prefix in their names. If that's not the case, this means some severe bug in Virtlet code.

Copy link
Contributor

@jellonek jellonek left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 2 change requests, 0 of 2 approvals obtained (waiting on @jellonek and @lukaszo)


pkg/libvirttools/gc.go, line 275 at r1 (raw file):

Previously, ivan4th (Ivan Shvedunov) wrote…

I thought about this. What stopped me from this approach is that with gc.go being a set of VirtualizationTool methods, VirtualizationTool itself becomes an instance of "God Object" antipattern. Because of this, I'd prefer not to add extra coupling between GC methods and other parts of VirtualizationTool.

That's solid reason with which I fully agree.

Copy link
Contributor

@pigmej pigmej left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r1.
Reviewable status: 1 change requests, 1 of 2 approvals obtained (waiting on @ivan4th and @lukaszo)

@pigmej pigmej merged commit b5c9b48 into master Oct 3, 2018
@pigmej pigmej deleted the ivan4th/devicemapper-gc branch October 3, 2018 11:04
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.

4 participants