Skip to content

fix can't edit object by using ctr content edit command#6847

Merged
kzys merged 1 commit intocontainerd:mainfrom
zhanghe9702:pr/fix-content-edit
Jul 6, 2022
Merged

fix can't edit object by using ctr content edit command#6847
kzys merged 1 commit intocontainerd:mainfrom
zhanghe9702:pr/fix-content-edit

Conversation

@zhanghe9702
Copy link
Copy Markdown
Contributor

@zhanghe9702 zhanghe9702 commented Apr 24, 2022

Signed-off-by: zhanghe9702 [email protected]
fix #6846

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Apr 24, 2022

Hi @zhanghe9702. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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/test-infra repository.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Apr 24, 2022

Build succeeded.

@zhanghe9702 zhanghe9702 force-pushed the pr/fix-content-edit branch from 1bb92e6 to 3b058d4 Compare April 25, 2022 02:37
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Apr 25, 2022

Build succeeded.

@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Apr 25, 2022

Signed-off-by: zhanghe9702 <[email protected]>

Please sign the commit using your real name rather than github username

@zhanghe9702 zhanghe9702 force-pushed the pr/fix-content-edit branch from 3b058d4 to 5e435f7 Compare April 26, 2022 01:55
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Apr 26, 2022

Build succeeded.

fuweid
fuweid reviewed Jun 24, 2022
View reviewed changes
Comment thread cmd/ctr/commands/content/content.go Outdated
Comment thread cmd/ctr/commands/content/content.go Outdated
@fuweid
Copy link
Copy Markdown
Member

fuweid commented Jun 24, 2022

The vim/nvim works in different ways under different filesystem. If the temp file in tmpfs, the vim/nvim will open the original file and rewrite all the content. If it is in ext4, it will use other file to rename to override the original file so that the inode will be changed.

@zhanghe9702 zhanghe9702 force-pushed the pr/fix-content-edit branch from 5e435f7 to 2cb34ff Compare June 24, 2022 10:31
fuweid
fuweid reviewed Jun 24, 2022
View reviewed changes
Comment thread cmd/ctr/commands/content/content.go Outdated
@zhanghe9702 zhanghe9702 force-pushed the pr/fix-content-edit branch 2 times, most recently from 3a34358 to ddcd694 Compare June 24, 2022 13:25
@fuweid
Copy link
Copy Markdown
Member

fuweid commented Jun 24, 2022

=== Failed
=== FAIL: pkg/cri/server (0.00s)
==6088==ERROR: ThreadSanitizer failed to allocate 0x000003f29000 (66228224) bytes at 0x200de7e6b4000 (error code: 87)
FAIL github.com/containerd/containerd/pkg/cri/server 0.059s

hmm... rerun it again.

@zhanghe9702
Copy link
Copy Markdown
Contributor Author

zhanghe9702 commented Jun 25, 2022

:-), looks like race detector alloc memory failed in windows, is the same issue golang/go#46099?

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Jun 25, 2022

:-), looks like race detector alloc memory failed in windows, is the same issue golang/go#46099?

not sure why. I don't have windows so it is hard to debug. I will file a issue for that.

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Jun 25, 2022

@zhanghe9702 would you mind to rebase and repush it again? I rerun several times but it is unhappy... thanks 🙏

@zhanghe9702
Copy link
Copy Markdown
Contributor Author

zhanghe9702 commented Jun 25, 2022

no problem!

@zhanghe9702
Copy link
Copy Markdown
Contributor Author

zhanghe9702 commented Jun 25, 2022 •
edited by fuweid
Loading

hmm... , i should already done rebase commit, this is git info

commit ddcd6946000bd871140e241bfd0361f7a5b66da9 (HEAD -> pr/fix-content-edit, zhanghe/pr/fix-content-edit)
Author: zhanghe9702 <[email protected]>
Date:   Sun Apr 24 18:53:04 2022 +0800

    fix can't edit object by using ctr content edit command
    
    Signed-off-by: zhang he <[email protected]>

commit e3298cf12785fd7fd0f21ebba06d3309d050e24c (origin/main)
Merge: a43dfdf4c 4f0ea7831
Author: Maksym Pavlenko <[email protected]>
Date:   Wed Jun 22 10:58:41 2022 -0700```

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Jun 25, 2022

hmm... , i should already done rebase commit, this is git info


commit ddcd6946000bd871140e241bfd0361f7a5b66da9 (HEAD -> pr/fix-content-edit, zhanghe/pr/fix-content-edit)

Author: zhanghe9702 <[email protected]>

Date:   Sun Apr 24 18:53:04 2022 +0800



    fix can't edit object by using ctr content edit command

    

    Signed-off-by: zhang he <[email protected]>



commit e3298cf12785fd7fd0f21ebba06d3309d050e24c (origin/main)

Merge: a43dfdf4c 4f0ea7831

Author: Maksym Pavlenko <[email protected]>

Date:   Wed Jun 22 10:58:41 2022 -0700```

You can try git commit --amend --no-edit

@zhanghe9702 zhanghe9702 force-pushed the pr/fix-content-edit branch from ddcd694 to faec0d3 Compare June 25, 2022 03:27
junnplus
junnplus approved these changes Jun 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@junnplus junnplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

This problem is related to vim/nvim's writebackup mechanism. On non-tmpfs filesystem, vim first writes to a temporary file and then rename it. The practice of reopening the file is more general.

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Jun 26, 2022

filed issue #7104 to track this flaky case.

fuweid
fuweid approved these changes Jun 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

The change is not related to CI failure. Re-run several times but still failed. :(

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Jun 28, 2022

@zhanghe9702 would you mind to rebase? the CI has been fixed~

@zhanghe9702 zhanghe9702 force-pushed the pr/fix-content-edit branch from faec0d3 to 8b95c0b Compare June 28, 2022 02:49
@zhanghe9702
Copy link
Copy Markdown
Contributor Author

zhanghe9702 commented Jun 28, 2022

done!

fuweid
fuweid approved these changes Jun 28, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fuweid fuweid added the easy-to-review Easy to review label Jun 28, 2022
akhilerm
akhilerm reviewed Jul 1, 2022
View reviewed changes
Comment thread cmd/ctr/commands/content/content.go
@zhanghe9702 zhanghe9702 force-pushed the pr/fix-content-edit branch 3 times, most recently from d4bffff to 536c5e0 Compare July 4, 2022 03:33
@zhanghe9702 zhanghe9702 force-pushed the pr/fix-content-edit branch from 536c5e0 to b8bb33b Compare July 4, 2022 03:44
dcantah
dcantah reviewed Jul 6, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@dcantah dcantah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix LGTM but I'd like if the commit message/pr description described why this fixes the issue as well

@kzys kzys merged commit bfb316c into containerd:main Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dcantah dcantah dcantah left review comments

@kzys kzys kzys approved these changes

@akhilerm akhilerm akhilerm approved these changes

@fuweid fuweid fuweid approved these changes

+1 more reviewer

@junnplus junnplus junnplus approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

easy-to-review Easy to review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

can't edit object in content store

9 participants

@zhanghe9702 @k8s-ci-robot @dmcgowan @fuweid @kzys @akhilerm @junnplus @dcantah @estesp