ImageFsInfo for CRI #735

jellonek · 2018-08-13T16:30:13Z

This change is

ivan4th

The PR is fine (except for comments), but please add a call to this newly implemented CRI method in pkg/manager/image_test.go (see below for details), it'll be about 10 extra lines to do this.

Reviewed 2 of 7 files at r1, 15 of 20 files at r2.
Reviewable status: 0 of 2 LGTMs obtained

pkg/image/image.go, line 63 at r2 (raw file):

// space/inodes used by images on it
type FilesystemStats struct {
	// Mountpoint denotes a filesystem mount point

denotes the mountpoint for this filesystem

pkg/image/image.go, line 65 at r2 (raw file):

	// Mountpoint denotes a filesystem mount point
	Mountpoint string
	// UsedBytes contains number of bytes used by images

is the number of

pkg/image/image.go, line 67 at r2 (raw file):

	// UsedBytes contains number of bytes used by images
	UsedBytes uint64
	// UsedInodes contains number of inodes used by images

is the number of

pkg/image/image.go, line 104 at r2 (raw file):

	// FilesystemStats returns info about used bytes/inodes by images
	// in this store

FilesystemStats returns disk space and inode usage info for this store.

pkg/image/image.go, line 567 at r2 (raw file):

// TODO: instead of returning data from filesystem we should retrieve from
// metadata store sizes of images and sum them, or even retrieve precalculated
// sum. That's because same filesystem could be used by other things than images.

FilesystemStats returns disk space and inode usage info for this store.

pkg/image/fake/store.go, line 131 at r2 (raw file):

// FilesystemStats implements FilesystemStats method from Store interface.
func (s *FakeStore) FilesystemStats() (*image.FilesystemStats, error) {

Please add a dummy implementation that returns stats with some constant fields. It can then be used in pkg/manager/image_test.go

pkg/manager/image.go, line 89 at r2 (raw file):

}

// ImageFsInfo returns an info about filesystem used by images service

ImageFsInfo returns the info about filesystem usage by the image store

pkg/manager/image.go, line 90 at r2 (raw file):

// ImageFsInfo returns an info about filesystem used by images service
func (v *VirtletImageService) ImageFsInfo(ctx context.Context, in *kubeapi.ImageFsInfoRequest) (*kubeapi.ImageFsInfoResponse, error) {

Please add a call to this method in pkg/image/image_test.go (by adding imageFsInfo() method to virtletCRITester() in pkg/manager/runtime_test.go) and a simple stub in pkg/image/fake/store.go. It doesn't even require separate test case, just have this call recorded in golden master output by doing as described here.

pkg/utils/fsstat_linux.go, line 26 at r2 (raw file):

// GetFsStatsForPath returns inodes and space in bytes used by filesystem
// releated to passed path

GetFsStatsForPath returns the info about inode usage and space usage (in bytes) for the filesystem that contains the provided path

pkg/utils/fsstat_unsupported.go, line 25 at r2 (raw file):

)

// GetFsStatsForPath is a placeholder for not implemented function

for an unimplemented function

ivan4th

Reviewable status: 0 of 2 LGTMs obtained

pkg/config/config.go, line 43 at r2 (raw file):

	imageDownloadProtocolEnv = "VIRTLET_DOWNLOAD_PROTOCOL"

	defaultImageDir = "/var/lib/libvirt/images"

I doubt we should make this change. Virtlet image store doesn't belong to libvirt and should reside outside the libvirt dir.

ivan4th

... also except for image dir change.

Reviewable status: 0 of 2 LGTMs obtained

jellonek

Reviewable status: 0 of 2 LGTMs obtained

pkg/image/image.go, line 65 at r2 (raw file):