-
Notifications
You must be signed in to change notification settings - Fork 349
Conversation
ac63683
to
a31f07f
Compare
@miaoyq This PR is using |
a31f07f
to
4e80f19
Compare
@Random-Liu Yes, I have used |
bbce1d3
to
3eb8ebf
Compare
@abhinandanpb @mikebrow Ready for review. |
e2d861f
to
2e7a61a
Compare
3b7ee00
to
f695f06
Compare
Will rebase after #264 is merged. |
f695f06
to
5015efa
Compare
integration/test_utils.go
Outdated
|
||
// Eventually waits for f to return nil, it checks every period, and | ||
// returns error if timeout exceeds. | ||
func Eventually(f func() (bool, error), period, timeout time.Duration) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be better if func() (bool, error)
is defined as a type, and given an explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
@@ -31,6 +31,8 @@ const configFilePathArgName = "config" | |||
|
|||
// ContainerdConfig contains config related to containerd | |||
type ContainerdConfig struct { | |||
// ContainerdRootDir is the root directory path for containerd. | |||
ContainerdRootDir string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As is suggested by @yanxuean, add toml here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
b0a4b2f
to
051307a
Compare
051307a
to
f8c0b1a
Compare
efadb8f
to
e6ab872
Compare
Signed-off-by: Lantao Liu <lantaol@google.com>
e6ab872
to
9ab6b48
Compare
pkg/store/snapshot/snapshot.go
Outdated
|
||
// Get returns the snapshot with specified id. Returns store.ErrNotExist if the | ||
// snapshot doesn't exist. | ||
func (s *Store) Get(id string) (Snapshot, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: id
to key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/store/snapshot/snapshot.go
Outdated
} | ||
|
||
// Delete deletes the snapshot with specified id. | ||
func (s *Store) Delete(id string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/store/snapshot/snapshot_test.go
Outdated
sns = s.List() | ||
assert.Len(sns, 2) | ||
|
||
t.Logf("get should return nil after deletion") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get should return empty struct and ErrNotExist after deletion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. Just did a first pass
hack/test-integration.sh
Outdated
REPORT_DIR=${REPORT_DIR:-"/tmp/test-integration"} | ||
|
||
mkdir -p ${REPORT_DIR} | ||
start_cri_containerd ${REPORT_DIR} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought. Should we call this testSetup and testTeardown(for kill_cri_containerd) ? Because I remember I was trying to look at why containerd was not being started and realized its started as part of this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
# Start cri-containerd | ||
sudo ${ROOT}/_output/cri-containerd --alsologtostderr --v 4 ${CRI_CONTAINERD_FLAGS} \ | ||
&> ${report_dir}/cri-containerd.log & | ||
readiness_check "sudo ${GOPATH}/bin/crictl --runtime-endpoint=${CRICONTAINERD_SOCK} info" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks cleaner :D
pkg/os/os.go
Outdated
} | ||
|
||
// DeviceUUID gets device uuid of a device. The passed in device should be | ||
// a aboslute path of the device. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: %s/a/an
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit /aboslute/absolute/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
var usedBytes, inodesUsed uint64 | ||
for _, sn := range snapshots { | ||
// Use the oldest timestamp as the timestamp of imagefs info. | ||
if sn.Timestamp < timestamp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for my knowledge. This is being done because we are caching the info stats ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a timestamp for the imagefs stats. And the imagefs stats is got from all snapshot stat.
Given so, it seems that we should use the earliest timestamp as the timestamp of imagefs stats?
pkg/server/imagefs_info_test.go
Outdated
} | ||
resp, err := c.ImageFsInfo(context.Background(), &runtime.ImageFsInfoRequest{}) | ||
require.NoError(t, err) | ||
assert.Equal(t, expected, resp.GetImageFilesystems()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the order returned by resp.GetImageFilesystems() gauranteed to be same here ? Because if we return from cache which is in the store (map) . I am sure you must have tested this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be only one filesystem stat in the list. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed :) May be just be explicit in this case? Sorry just in case somebody reuses this test to build on top they might just expect it to be in order since its done here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
// Store stores all snapshots. | ||
type Store struct { | ||
lock sync.RWMutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we can remove the name ? and just keep the type ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the name means exposing the Lock
/Unlock
function to the user, which we may want to avoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense
pkg/server/snapshots.go
Outdated
tick := time.NewTicker(s.syncPeriod) | ||
go func() { | ||
defer tick.Stop() | ||
for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so how is this go routine going to exit ? should we pass a context and cancel it ?
Note: I did see the comment above :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems expensive to be running on a timer even if there are no changes.. maybe a TODO to come up with better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so how is this go routine going to exit ? should we pass a context and cancel it ?
Note: I did see the comment above :)
The goroutine will only stop when the process stops. Mentioned in the comment No stop function is needed because the syncer doesn't update any persistent states, it's fine to let it exit with the process.
this seems expensive to be running on a timer even if there are no changes.. maybe a TODO to come up with better solution.
Yes, this is expensive, but we have to do it. :( This is also how cadvisor works. Kubernetes retrieves stats every <10 second, and expect the stats should be returned immediately. So we have to cache the stats periodically for Kubernetes to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right.. I remember that.. just pointing out we could use a todo to come up with a better way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikebrow OK. Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pkg/os/os.go
Outdated
} | ||
|
||
// DeviceUUID gets device uuid of a device. The passed in device should be | ||
// a aboslute path of the device. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit /aboslute/absolute/
if sn.Timestamp < timestamp { | ||
timestamp = sn.Timestamp | ||
} | ||
usedBytes += sn.Size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was the response supposed to be an aggregate response? If so why did they create an array of filesystemusage? Or is that what the below todo is for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because there may be multiple filesystems used for image management.
pkg/server/snapshots.go
Outdated
tick := time.NewTicker(s.syncPeriod) | ||
go func() { | ||
defer tick.Stop() | ||
for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems expensive to be running on a timer even if there are no changes.. maybe a TODO to come up with better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One minor comment.
@abhinandanpb @mikebrow Addressed comments. Will squash the latest 2 commits after LGTM. |
Signed-off-by: Lantao Liu <lantaol@google.com>
Signed-off-by: Lantao Liu <lantaol@google.com>
fa5c285
to
8fb57da
Compare
Apply LGTM based on #257 (review). Will merge it after test passes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/LGTM
Fixes #195.
Part of #121.
Depends on kubernetes/kubernetes#52635.
This PR:
ImageFsInfo
support.ImageFsInfo
.@kubernetes-incubator/maintainers-cri-containerd