-
Notifications
You must be signed in to change notification settings - Fork 347
Do not error out if uuid is not found. #510
Conversation
dd1c695 to
c9ae3f4
Compare
c9ae3f4 to
04a71ea
Compare
mikebrow
left a comment
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.
See nit comments.
| ProfilingPort string `toml:"profiling_port" json:"profilingPort,omitempty"` | ||
| // ProfilingAddress is address for profiling via host:port/debug/pprof/ | ||
| ProfilingAddress string `toml:"profiling_addr" json:"profilingAddress,omitempty"` | ||
| // SkipImageFSUUID skips retrieving imagefs uuid. |
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.
/s/./ for new instance of CRIContainerdService./
| fs.StringVar(&c.ProfilingAddress, "profiling-addr", | ||
| defaults.ProfilingAddress, "Profiling address for web interface host:port/debug/pprof/.") | ||
| fs.BoolVar(&c.SkipImageFSUUID, "skip-imagefs-uuid", | ||
| defaults.SkipImageFSUUID, "Skip retrieving imagefs uuid. When turned on, kubelet can't get imagefs capacity and imagefs disk eviction can't work.") |
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.
Skip retrieval of imagefs uuid for new CRIContainerdService instances. When turned on, the kubelet will not be able to get imagefs capacity or request imagefs disk eviction.
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.
CRIContainerdService is code name, I don't think user cares about that in the flag discription. :)
I'll update the other parts.
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.
| c.imageFSUUID, err = c.getDeviceUUID(imageFSPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get imagefs uuid of %q: %v", imageFSPath, err) | ||
| if !c.config.SkipImageFSUUID { |
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.
suggest adding an info logf that it has been skipped..
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.
Signed-off-by: Lantao Liu <[email protected]>
04a71ea to
dca0535
Compare
mikebrow
left a comment
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
Current uuid handling logic is not perfect. Some known issues have been reported such as #509, #399 and #325.
When this doesn't work, kubelet won't be able to get image filesystem capacity and usage. However, we should not keep user from trying cri-containerd out because of this.
Add a flag to skip this logic, and I'll figure out how we could make this more portable across different environments.
Signed-off-by: Lantao Liu [email protected]