bugfix: make sure SnapshotID not empty when start container

[HusterWan]

Signed-off-by: Michael Wan [email protected]

Ⅰ. Describe what this PR did

Found a problem on production environment:

time="2019-01-21T16:56:13.761316917+08:00" level=error msg="failed to create new containerd container: failed to create container 1edc1d9a416f3ed03d91fd941f040ae16c7a7376c24bf0b5370147c4cc9350e3: snapshot  does not exist: not found: not found"

I checked the container's meta.json file and found the SnapshotID is empty, though i cannot figure out why this field was empty, but we should make sure the SnapshotID should be assigned to proper value.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

[codecov]

Codecov Report

Merging #2678 into master will increase coverage by 6.19%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2678      +/-   ##
==========================================
+ Coverage   63.03%   69.23%   +6.19%     
==========================================
  Files         276      285       +9     
  Lines       17356    18959    +1603     
==========================================
+ Hits        10941    13126    +2185     
+ Misses       5206     4357     -849     
- Partials     1209     1476     +267

Flag	Coverage Δ
#criv1alpha1test	31.33% <100%> (?)
#criv1alpha2test	35.9% <100%> (?)
#integration_test_0	?
#integration_test_2	?
#integrationtest	41.98% <100%> (?)
#node_e2e_test	?
#nodee2etest	32.06% <100%> (?)
#unittest	26.96% <0%> (-0.42%)	⬇️

Impacted Files	Coverage Δ
daemon/mgr/container.go	58.94% <100%> (+9.15%)	⬆️
cri/ocicni/cni_manager.go	58.82% <0%> (-11.77%)	⬇️
daemon/mgr/system.go	67.93% <0%> (-5.35%)	⬇️
daemon/config/config.go	66.21% <0%> (-1.65%)	⬇️
daemon/daemon.go	69.23% <0%> (-0.52%)	⬇️
daemon/mgr/spec.go	66.66% <0%> (ø)	⬆️
daemon/mgr/spec_seccomp_linux.go	71.42% <0%> (ø)
cri/v1alpha1/cri_utils.go	83.53% <0%> (ø)
cri/v1alpha1/service/cri.go	84.61% <0%> (ø)
cri/v1alpha1/cri.go	59.43% <0%> (ø)
... and 57 more

[fuweid]

@HusterWan the following code is used for old containerd shim right?

// SnapshotKey returns id of container's snapshot
func (c *Container) SnapshotKey() string {
        c.Lock()
        defer c.Unlock()
        // for old container, SnapshotKey equals to Container ID
        if c.SnapshotID == "" {
                return c.ID
        }

        return c.SnapshotID
}

[HusterWan]

@HusterWan the following code is used for old containerd shim right?

// SnapshotKey returns id of container's snapshot
func (c *Container) SnapshotKey() string {
        c.Lock()
        defer c.Unlock()
        // for old container, SnapshotKey equals to Container ID
        if c.SnapshotID == "" {
                return c.ID
        }

        return c.SnapshotID
}

@fuweid No, this old container is not old shim, but before upgrade container

[Ace-Tang]

I check that snapshot id is first given to SnapshotID , snapID := id, SnapshotID: snapID, so why this field will be empty , if this happened, do c.SnapshotKey() will also be empty

[HusterWan]

I check that snapshot id is first given to SnapshotID , snapID := id, SnapshotID: snapID, so why this field will be empty

I also cannot figure it out why this happened, maybe some extreme cases

if this happened, do c.SnapshotKey() will also be empty

No, SnapshotKey() will return c.ID if the SnapshotID is empty

[Ace-Tang]

Look good to me, make the ci pass, @HusterWan

[fuweid]

LGTM