-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Fixes a default config bug of gc scheduler #2214
Conversation
f94b6bd
to
40f80d3
Compare
gc/scheduler/scheduler.go
Outdated
type duration time.Duration | ||
type duration struct { | ||
time.Duration | ||
} |
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.
Why was this change necessary? Can't you just add MarshalText
?
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.
Why was this change necessary?
It can avoid type conversion like time.Duration(cfg.ScheduleDelay)
.
Can't you just add
MarshalText
?
UnmarshalText
is not added by me.
Isn't it used when loading config file? I will verify it.
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.
@dnephin We need UnmarshalText
when loading config file, otherwise containerd service can not start with the error log:
containerd: toml: type mismatch for scheduler.duration: expected table but found string
But I modify UnmarshalText
like:
func (d duration) UnmarshalText(_ []byte) error {
return nil
}
Still works well, don't know why! 😅
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.
return nil
is probably ignoring the problem, it suspect it will serialize to nothing.
My comment wasn't about UnarmshalText
. I think it was correct and doesn't need to change.
I was suggesting it would be better to leave the type as type duration time.Duration
and remove the other changes from this PR. I don't think the type conversion is a problem. The only thing that needs to change is adding MarshalText
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.
@dnephin Thanks for review, have address your comment.
79034af
to
4206a00
Compare
Signed-off-by: Yanqiang Miao <miao.yanqiang@zte.com.cn>
4206a00
to
d465f85
Compare
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.
Thanks. A unit test that show the the roundtrip of values (Marshal, then Unmarshal duration) would be great, especially the value that you found was a problem before.
LGTM @dmcgowan PTAL |
LGTM |
I ran the
containerd
service with the default config, the service failed when loading config file with error log:That's because
toml.Decode
can not decode100000000
totime.Duration
variable.We should encode
time.Duration
variable to string with unit (e.g.:100000000
->"100ms"
) when print the the default config withcontainerd config default
.This pr fixes the bug.
Signed-off-by: Yanqiang Miao miao.yanqiang@zte.com.cn