ignore update status file when no space#11458
Conversation
|
Hi @yylt. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
yylt
force-pushed
the
nospace
branch
2 times, most recently
from
36fe892 to
ce34076
Compare
| } | ||
|
|
||
| // writeFileInplace writes the data to the file in-place. | ||
| func writeFileInplace(path string, data []byte, perm os.FileMode) error { |
There was a problem hiding this comment.
What is "in-place"? How does this differ from os.WriteFile?
There was a problem hiding this comment.
Inplace write cannot guarantee to be a transaction and may result in incompleteness file
There was a problem hiding this comment.
Why can't we just use os.WriteFile here?
There was a problem hiding this comment.
could use file descripter execute Sync to make sure had saved on disk, try to minimize the time that may cause inconsistent JSON struct content in the file
There was a problem hiding this comment.
not update status file when root directory full
| } | ||
| if err := continuity.AtomicWriteFile(s.path, data, 0600); err != nil { | ||
| return fmt.Errorf("failed to checkpoint status to %q: %w", s.path, err) | ||
| if strings.Contains(err.Error(), "no space") { |
There was a problem hiding this comment.
errors.Is(err, unix.ENOSPC) may work?
There was a problem hiding this comment.
use syscall.ENOSPC which is depending on the underlying system
| runtime "k8s.io/cri-api/pkg/apis/runtime/v1" | ||
| ) | ||
|
|
||
| func TestIssue7247(t *testing.T) { |
There was a problem hiding this comment.
Needs an explanation of the issue
yylt
force-pushed
the
nospace
branch
3 times, most recently
from
b5fa99a to
2e4d435
Compare
| path := filepath.Join(root, "status") | ||
| if err := continuity.AtomicWriteFile(path, data, 0600); err != nil { | ||
| return nil, fmt.Errorf("failed to checkpoint status to %q: %w", path, err) | ||
| if strings.Contains(err.Error(), "no space") { |
There was a problem hiding this comment.
I filed a fix plan in #11504.
Please take a look on that.
My idea is to introduce a new StoreStatus which is used in restart.go file only.
In your patch, in place doesn't make sense to me. We can't guarantee it always work.
Based on that, we can just ignore no-space error and use memory status directly.
There was a problem hiding this comment.
looks better and has been modified.
yylt
changed the title
Signed-off-by: yang yang <[email protected]>
Signed-off-by: yang yang <[email protected]>
4 participants
Fix #7247
Failed to start recovre, the root cause is inconsistent data between
criandcontainer in metadataEnsure consistency, the following manual recovery could be resolve.
As described in here, ignoring noSpace error is a more convenient way to handle such issues