-
Notifications
You must be signed in to change notification settings - Fork 350
Add Truncindex for container, sandbox and image #235
Conversation
c5d7128
to
220f353
Compare
1cec173
to
71e9e79
Compare
Really bumpy. It is passed finally. |
Will probably review this a bit later. Will review other PRs first. |
797ec70
to
e9a73a1
Compare
204f8e6
to
341c141
Compare
@yanxuean I'll take a look at this today. |
@Random-Liu Tks |
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 overall with several nits.
pkg/server/container_attach.go
Outdated
@@ -38,7 +38,7 @@ func (c *criContainerdService) Attach(ctx context.Context, r *runtime.AttachRequ | |||
|
|||
cntr, err := c.containerStore.Get(r.GetContainerId()) | |||
if err != nil { | |||
return nil, fmt.Errorf("failed to find container in store: %v", err) | |||
return nil, fmt.Errorf("failed to find container %q in store: %v", r.GetContainerId(), err) |
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.
I intentionally didn't added the id, because the CRI caller already knows the id, and usually print out the id in its error message.
I think it's fine to not add the id.
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.
I don't understand. Why add it at below?
func (c *criContainerdService) Attach(ctx context.Context, r *runtime.AttachRequest) (retRes *runtime.AttachResponse, retErr error) {
glog.V(2).Infof("Attach for %q with tty %v and stdin %v", r.GetContainerId(), r.GetTty(), r.GetStdin())
defer func() {
if retErr == nil {
glog.V(2).Infof("Attach for %q returns URL %q", r.GetContainerId(), retRes.Url)
}
}()
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.
@yanxuean That's log. For log we should include the id, so that we could see what container is throwing this error in log. :)
Any way, I just feel like, on kubelet side, when there is an error, we'll see the id in error 2 or more times in the error message, which is a bit redundant to me. So I kind of prefer not including container id in our error message if caller already knows it.
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.
I see. Thanks
container, err := NewContainer( | ||
metadatas[id], | ||
v, |
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: Can we still use metadatas[id]
?
So that in the future, if this becomes a pointer, this still won't have problem
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/image/image_test.go
Outdated
assert.Contains(got.RepoDigests, oldRepoDigest, newRepoDigest) | ||
|
||
t.Logf("should not be able to add duplicated repo tags/digests") | ||
assert.Equal(nil, s.Add(newImg)) |
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.
assert.NoError.
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
@@ -142,7 +142,9 @@ func (c *criContainerdService) PullImage(ctx context.Context, r *runtime.PullIma | |||
img.RepoTags = []string{repoTag} | |||
} | |||
|
|||
c.imageStore.Add(img) | |||
if err := c.imageStore.Add(img); err != nil { | |||
return nil, fmt.Errorf("failed to add image %q into store: %v", img.ID, err) |
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.
I'm fine with this ID here, because caller doesn't know it.
pkg/store/container/container.go
Outdated
@@ -112,6 +116,10 @@ func (s *Store) Add(c Container) error { | |||
if _, ok := s.containers[c.ID]; ok { | |||
return store.ErrAlreadyExist | |||
} | |||
|
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: unnecessary empty line.
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/image/image_test.go
Outdated
newImg := v | ||
newImg.RepoTags = []string{newRepoTag} | ||
newImg.RepoDigests = []string{newRepoDigest} | ||
s.Add(newImg) |
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.
assert.NoError.
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/sandbox/sandbox.go
Outdated
@@ -58,6 +62,11 @@ func (s *Store) Add(sb Sandbox) error { | |||
if _, ok := s.sandboxes[sb.ID]; ok { | |||
return store.ErrAlreadyExist | |||
} | |||
|
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: empty line not needed and the following one.
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
Are there rules? @Random-Liu
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.
I was told that we should avoid unnecessary empty lines to make the code short and easy to read. So that we could have more code in one screen and easier for reader to understand.
Some convention inside Google. :)
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.
Not from google but i hate empty lines as well ;)
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.
@crosbymichael OK. So everyone hates empty lines not only Google. :)
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.
The rules are subjective:)
pkg/store/sandbox/sandbox.go
Outdated
if err != nil { | ||
return | ||
} | ||
_ = s.idIndex.Delete(id) |
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.
done
pkg/store/sandbox/sandbox_test.go
Outdated
sandboxes[id] = Sandbox{Metadata: metadatas[id]} | ||
//ids := []string{"1", "2", "3"} | ||
for id, v := range metadatas { | ||
sandboxes[id] = Sandbox{Metadata: v} |
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.
done
pkg/store/sandbox/sandbox_test.go
Outdated
for id, sb := range sandboxes { | ||
got, err := s.Get(id) | ||
got, err := s.Get(genTruncIndex(id)) | ||
|
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.
why empty line?
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.
I promise not for KPI:)
Do we add truncindex for the filter of Stat function? |
refer to #293. ContainerStats |
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 very nice
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 with last comment. It's my fault, sorry for the trouble.
pkg/store/sandbox/sandbox.go
Outdated
@@ -88,5 +102,7 @@ func (s *Store) List() []Sandbox { | |||
func (s *Store) Delete(id string) { | |||
s.lock.Lock() | |||
defer s.lock.Unlock() | |||
id, _ = s.idIndex.Get(id) // nolint: errcheck |
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.
One last comment. It's my fault.
I didn't know that idIndex.Delete
doesn't handle truncated index.
If so, I'm fine with your original code, the new one seems fragile. Sorry for the trouble... :(
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.
Both make sence. When i write it at first I just do it like this. We do best effort to clear ID even through there are some abnormal case in idindex. Because it is regular ID not truncated index in most case. We only use truncindex with crictl.Do you think about it?
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.
@yanxuean If there is an error, the returned id will be empty string, I feel like we should just return as what you did before. Please add comment to explain why we return there.
As for the Delete
, I think it's fine to just call s.idIndex.Delete(id), we don't need
_ =` in front.
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
@yanxuean Could you add a simple integration test for this? Just create a single container pod, and get pod/container/image with truncated index. Integration test is here https://github.com/kubernetes-incubator/cri-containerd/tree/master/integration. |
OK, I will add a intergration test. |
@Random-Liu @crosbymichael @mikebrow Thank for review |
51c2f5f
to
5ee3423
Compare
fix containerd#222 Signed-off-by: yanxuean <yan.xuean@zte.com.cn>
fix #222
Signed-off-by: yanxuean yan.xuean@zte.com.cn