Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support shared domain #162

Merged
merged 1 commit into from
Oct 11, 2022
Merged

Support shared domain #162

merged 1 commit into from
Oct 11, 2022

Conversation

userzj
Copy link

@userzj userzj commented Sep 7, 2022

@codecov-commenter
Copy link

codecov-commenter commented Sep 7, 2022 •
edited
Loading

Codecov Report

Base: 28.87% // Head: 29.13% // Increases project coverage by +0.26% 🎉

Coverage data is based on head (f2f36e9) compared to base (09ba87a).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #162      +/-   ##
==========================================
+ Coverage   28.87%   29.13%   +0.26%     
==========================================
  Files          27       27              
  Lines        2293     2296       +3     
==========================================
+ Hits          662      669       +7     
+ Misses       1537     1533       -4     
  Partials       94       94
Impacted Files Coverage Δ
config/daemonconfig.go 0.00% <0.00%> (ø)
pkg/filesystem/fs/blob_manager.go 44.44% <0.00%> (-11.12%) ⬇️
config/config.go 0.00% <0.00%> (ø)
pkg/filesystem/fs/fs.go 1.10% <0.00%> (ø)
pkg/filesystem/fs/config.go 4.34% <0.00%> (+0.59%) ⬆️
pkg/auth/kubesecret.go 34.02% <0.00%> (+10.30%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hsiangkao
Copy link
Contributor

hsiangkao commented Sep 9, 2022 •
edited
Loading 

golangci-lint run
pkg/utils/erofs/erofs_linux.go:21: File is not `gofmt`-ed with `-s` (gofmt)
		if err := mount("erofs", mountPoint, "erofs", 0, "fsid=" + fsID); err != nil {

@userzj userzj marked this pull request as ready for review October 9, 2022 03:45
@userzj userzj changed the title [WIP] Support shared domain Support shared domain Oct 9, 2022
changweige
changweige reviewed Oct 9, 2022
View reviewed changes
config/daemonconfig.go Outdated
@@ -161,7 +161,7 @@ func NewDaemonConfig(fsDriver string, cfg DaemonConfig, imageID, snapshotID stri
backendConfig = &cfg.Config.BackendConfig
fscacheID := erofs.FscacheID(snapshotID)
cfg.ID = fscacheID
cfg.DomainID = fscacheID
cfg.DomainID = "global-domain"
Copy link
Member

Choose a reason for hiding this comment

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

Is domain ID specific to kernel/erofs? In other words, is it a predefined keyword in the Kernel?

Copy link
Author

Choose a reason for hiding this comment

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

Yup, @domain_id is a mount option of EROFS.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I mean "global-domain" :-)

Copy link
Author

Choose a reason for hiding this comment

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

'global-domain' is not a keyword in the kernel. This is just a name(id) specified by us. In future work, 'domain_id' will be specified by the user.

changweige
changweige reviewed Oct 9, 2022
View reviewed changes
pkg/utils/erofs/erofs_linux.go Outdated
logrus.Infof("Mount erofs to %s with options %s", mountPoint, opts)
if err := mount("erofs", mountPoint, "erofs", 0, opts); err != nil {
return errors.Wrapf(err, "failed to mount erofs")
if err := mount("erofs", mountPoint, "erofs", 0, "fsid="+fsID); err != nil {
return errors.Wrapf(err, "failed to mount erofs")
Copy link
Member

Choose a reason for hiding this comment

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

Why do you retry another mount if the L20 mount operation fails? It's better to add a comment to tell necessity and intention.

Copy link
Author

Choose a reason for hiding this comment

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

If a mount with the option "domain_id" fails, this indicates that the kernel may not support EROFS shared domain, and will fall back to normal mount. I'll add a comment about that. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Can it check return error code like unsupported or invalid ?

changweige
changweige reviewed Oct 9, 2022
View reviewed changes
pkg/utils/erofs/erofs_linux.go Outdated
@@ -15,10 +15,12 @@ import (
func Mount(bootstrapPath, fsID, mountPoint string) error {
mount := unix.Mount

opts := "fsid=" + fsID
opts := fmt.Sprintf("domain_id=%s,fsid=%s", "global-domain", fsID)
Copy link
Member

Choose a reason for hiding this comment

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

It's better to make "global-domain" as a global constant

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I'll revise it.

@userzj userzj force-pushed the zhujia/domain-upstream branch 3 times, most recently from 5ba09a4 to c304447 Compare October 10, 2022 03:25
changweige
changweige reviewed Oct 10, 2022
View reviewed changes
pkg/utils/erofs/erofs_linux.go Outdated
@@ -12,15 +12,24 @@ import (
"golang.org/x/sys/unix"
)

const globalDomainName = "global-domain"
Copy link
Member

Choose a reason for hiding this comment

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

Um. erofs package should import the constant from daemonconfig.go

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, will fix it.

changweige
changweige reviewed Oct 10, 2022
View reviewed changes
pkg/utils/erofs/erofs_linux.go Outdated
logrus.Infof("Mount erofs to %s with options %s", mountPoint, opts)
if err := mount("erofs", mountPoint, "erofs", 0, opts); err != nil {
err = mount("erofs", mountPoint, "erofs", 0, opts)
Copy link
Member

Choose a reason for hiding this comment

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

We still can improve the code like:

if err := mount("erofs", mountPoint, "erofs", 0, opts); err!=nil {
	if errors.Is(err, unix.EINVAL) {
		err = mount("erofs", mountPoint, "erofs", 0, "fsid="+fsID)
                 if err != nil {
                      return errr
}
	}

         return errors.Wrapf(err, "failed to mount erofs")
}

Copy link
Author

Choose a reason for hiding this comment

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

Thanks.

@userzj userzj force-pushed the zhujia/domain-upstream branch 3 times, most recently from 14621e2 to b685756 Compare October 10, 2022 11:27
support global shared domain
f2f36e9 
Use a global domain for all mountpoints.
If kernel doesn't support shared domain, will fallback to
the mount without 'domain_id' option.

Signed-off-by: Jia Zhu <zhujia.zj@bytedance.com>
changweige
changweige approved these changes Oct 11, 2022
View reviewed changes
Copy link
Member

@changweige changweige left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@changweige changweige merged commit ddd543e into containerd:main Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@changweige changweige changweige approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@userzj @codecov-commenter @hsiangkao @changweige