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.

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

mark stargz configuration as experimental #346

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

changweige
Copy link
Member

@changweige changweige commented Feb 6, 2023

As the stargz compatiblity feature is not production ready
and reliable enought

Signed-off-by: Changwei Ge gechangwei@bytedance.com

@codecov-commenter
Copy link

Codecov Report

Base: 28.25% // Head: 28.25% // No change to project coverage 👍

Coverage data is based on head (1b50dfe) compared to base (13a9e05).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #346   +/-   ##
=======================================
  Coverage   28.25%   28.25%           
=======================================
  Files          37       37           
  Lines        3667     3667           
=======================================
  Hits         1036     1036           
  Misses       2510     2510           
  Partials      121      121           
Impacted Files Coverage Δ
config/config.go 19.29% <ø> (ø)

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.

config/config.go Outdated
@@ -175,6 +178,7 @@ type SnapshotterConfig struct {
ImageConfig ImageConfig `toml:"image"`
CacheManagerConfig CacheManagerConfig `toml:"cache_manager"`
LoggingConfig LoggingConfig `toml:"log"`
Experimental Experimental `toml:"experimental"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will it move out once the feature becomes ready?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. We will move it out when it's ready. In fact, the stargz compibility feature has malfunctioned for quite a time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How do we ensure the CLI flags compatibility when moving it out?

Copy link
Member Author

Choose a reason for hiding this comment

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

CLI does not has --enable-stargz flag now. and stargz feature has malfunctioned for a long time with stable nydusd release. So I suppose we don't have to worry about compatiblity.

@@ -93,6 +93,10 @@ const (
FsDriverFscache string = "fscache"
)

type Experimental struct {
Copy link
Member

@sctb512 sctb512 Feb 6, 2023

Choose a reason for hiding this comment

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

Could we add a description of experimental in the related document and an example in toml?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, already changed the example toml configuraion file

Copy link
Member

@sctb512 sctb512 left a comment

Choose a reason for hiding this comment

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

LGTM

@imeoer
Copy link
Collaborator

imeoer commented Feb 7, 2023

Need to be rebased.

As the stargz compatiblity feature is not production ready
 and reliable enought.

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>
@changweige
Copy link
Member Author

Need to be rebased.

Rebased. Thanks.

@imeoer imeoer merged commit 2d9c187 into containerd:main Feb 8, 2023
@changweige changweige deleted the mark-stargz-as-experimental branch February 27, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants