Fixed issue with duration strings used in yaml config file #20360

BBQing · 2025-07-18T14:25:20Z

Relates: #20342

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

This is a proposal on how to fix the issue. the problem is, that for the parameter --watch-progress-notify-interval, there is a difference in the behavior. While it is accepted as duration string from CLI, when provided in config_file, it has to be a number, that then gets converted into nanoseconds so for example for 1s you have to use 1000000000. Although it works well for machines it's bad for humans.

I have wrapped the time.duration, so that I can implement the UnmarshalJSON interface and also I have implemented flag.Value interface. And applied it where necessary.
I have also fixed some tests.

The fix supports both numbers and duration string from config file and in the future there can be issued warning, in case it will be decided to abandon the possibility to use numbers in the configuration.

It might be necessary to do this for every duration parameter in the future.
I guess this concerns etcdserver.

k8s-ci-robot · 2025-07-18T14:25:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BBQing
Once this PR has been reviewed and has the lgtm label, please assign fuweid for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-07-18T14:25:30Z

Hi @BBQing. Thanks for your PR.

I'm waiting for a etcd-io 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.

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-sigs/prow repository.

server/embed/config.go

BBQing · 2025-07-20T16:07:28Z

@ivanvc could you check it?

ivanvc · 2025-07-25T00:27:34Z

Please refer to my comment in #20342 (comment).

server/embed/config.go

ivanvc · 2025-07-25T22:20:53Z

/ok-to-test

ivanvc · 2025-07-25T22:36:16Z

server/embed/config.go

@@ -156,6 +156,46 @@ func init() {
 	defaultHostname, defaultHostStatus = netutil.GetDefaultHost()
 }

+type Duration struct {
+	Duration time.Duration


I left a comment here before, but I don't know what happened as it seems like GitHub dropped it 🤔.

I suggested changing this to an anonymous field. I believe it would be transparent in cases like:

WatchProgressNotifyInterval: cfg.WatchProgressNotifyInterval.Duration,

We could revert this change.

Suggested change

Duration time.Duration

time.Duration

I initially had it anonymous, but then I have decided to be more explicit - I have reverted the change

Updated tests to work with wrapped duration Signed-off-by: sprochazka <[email protected]>

ivanvc

Thanks for the changes, @BBQing. I must have been more explicit before with my suggestion. Apologies, again, I'm not sure why GitHub sent my comment to the void 😅

ivanvc · 2025-07-25T22:46:04Z

server/embed/etcd.go

@@ -224,7 +224,7 @@ func StartEtcd(inCfg *Config) (e *Etcd, err error) {
 		UnsafeNoFsync:                     cfg.UnsafeNoFsync,
 		CompactionBatchLimit:              cfg.CompactionBatchLimit,
 		CompactionSleepInterval:           cfg.CompactionSleepInterval,
-		WatchProgressNotifyInterval:       cfg.WatchProgressNotifyInterval,
+		WatchProgressNotifyInterval:       cfg.WatchProgressNotifyInterval.Duration,


With the anonymous time.Duration field, I believe this change is no longer necessary.

I have tried to use embed.Duration(current location) instead of time.Duration in the ServerConfig and it failed on the circular imports - I am thinking if the Duration should be placed in the pkg directory in its own package (or in some already existing package).
I'll continue with changes tomorrow. But thank you for the feedback - I am learning a lot.

Thanks, @BBQing. I'll keep an eye on this pull request.

ivanvc · 2025-07-25T22:46:32Z

server/embed/config.go

+func (d Duration) String() string {
+	return d.Duration.String()
+}


And this is not required, either. As it would call String() from d.Duration.

codecov · 2025-07-25T23:05:06Z

Codecov Report

❌ Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.20%. Comparing base (0888376) to head (567d35a).
⚠️ Report is 234 commits behind head on main.

Files with missing lines	Patch %	Lines
server/embed/config.go	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
server/embed/config_duration.go	`100.00% <100.00%> (ø)`
server/embed/config.go	`79.16% <0.00%> (+50.06%)`	⬆️

... and 163 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #20360      +/-   ##
==========================================
+ Coverage   62.94%   69.20%   +6.26%     
==========================================
  Files         386      417      +31     
  Lines       32989    34724    +1735     
==========================================
+ Hits        20765    24031    +3266     
+ Misses      10667     9300    -1367     
+ Partials     1557     1393     -164

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0888376...567d35a. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

…sing Signed-off-by: sprochazka <[email protected]>

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

ivanvc

Thanks for the updates, @BBQing. Please take a look at my comments :)

ivanvc · 2025-07-30T23:11:46Z

server/embed/config.go

+	if cfg.WatchProgressNotifyIntervalJSON.Duration != 0 {
+		cfg.WatchProgressNotifyInterval = cfg.WatchProgressNotifyIntervalJSON.Duration
+	}


Suggestion to drop the .Duration

Suggested change

if cfg.WatchProgressNotifyIntervalJSON.Duration != 0 {

cfg.WatchProgressNotifyInterval = cfg.WatchProgressNotifyIntervalJSON.Duration

}

if cfg.WatchProgressNotifyIntervalJSON != 0 {

cfg.WatchProgressNotifyInterval = cfg.WatchProgressNotifyIntervalJSON

}

Thanks, I missed the casting. Thanks for adding that.

ivanvc · 2025-07-30T23:13:04Z

tests/framework/e2e/cluster.go

-	return func(c *EtcdProcessClusterConfig) { c.ServerConfig.WatchProgressNotifyInterval = interval }
+	return func(c *EtcdProcessClusterConfig) {
+		c.ServerConfig.WatchProgressNotifyInterval = interval
+	}


Could you please revert this change?

ivanvc · 2025-07-30T23:13:55Z

server/embed/config_duration.go

+	"time"
+)
+
+type Duration struct {


This should be named ConfigDuration if the filename is config_duration.go.

I'll change that

ivanvc · 2025-07-30T23:14:51Z

server/embed/config.go

+	WatchProgressNotifyInterval     time.Duration
+	WatchProgressNotifyIntervalJSON Duration `json:"watch-progress-notify-interval"`


What was the issue of keeping a single field?

The problem for me was, that in the downstream the value is compared to some time.Duration constant (minimal watch progress interval - I could have retyped the constant into the new Duration type, but I think since the functionality of the wrapper type is related to configuration, it should remain there and not leak outside. So I have decided to follow approach used for different configuration fields, that also require some special treatment. beyond the standard unmarshalling

Makes sense, thanks for the clarification. Let's see what other contributors think :)

…d Duration to ConfigDuration Signed-off-by: sprochazka <[email protected]>

ivanvc · 2025-08-01T06:41:07Z

This looks good to me. It fixes the issue for watch-progress-notify-interval. Ideally, after this pull request, we should have a follow-up to address the remaining duration fields.

However, I'm not an expert on etcdserver. @ahrtr, @fuweid, would you be able to please take a look at this? Thanks.

BBQing · 2025-08-05T21:55:18Z

@ahrtr @fuweid Could you please check this PR? Thank you

ahrtr · 2025-08-10T12:12:24Z

Thanks for the contribution, but please see my comment #20342 (comment)

k8s-ci-robot added the area/robustness-testing label Jul 18, 2025

k8s-ci-robot added area/testing needs-ok-to-test labels Jul 18, 2025

k8s-ci-robot added the size/M label Jul 18, 2025

BBQing force-pushed the unmarshall_duration branch from 86887a2 to 46a9ba2 Compare July 18, 2025 14:41

BBQing commented Jul 18, 2025

View reviewed changes

server/embed/config.go Outdated Show resolved Hide resolved

BBQing commented Jul 18, 2025

View reviewed changes

server/embed/config.go Outdated Show resolved Hide resolved

ivanvc mentioned this pull request Jul 25, 2025

--watch-progress-notify-interval doesn't work through JSON config #20342

Open

4 tasks

ivanvc reviewed Jul 25, 2025

View reviewed changes

server/embed/config.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jul 25, 2025

BBQing force-pushed the unmarshall_duration branch from 46a9ba2 to b0edd91 Compare July 25, 2025 22:27

ivanvc reviewed Jul 25, 2025

View reviewed changes

Wrapped time.Duration and extended with json unmarshalling

519616b

Updated tests to work with wrapped duration Signed-off-by: sprochazka <[email protected]>

BBQing force-pushed the unmarshall_duration branch from b0edd91 to 519616b Compare July 25, 2025 22:42

ivanvc reviewed Jul 25, 2025

View reviewed changes

Etcdserver: Moved duration into separate package timeutil

fdbe0a9

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

k8s-ci-robot added size/L size/M and removed size/M size/L labels Jul 30, 2025

BBQing force-pushed the unmarshall_duration branch from 867e141 to 2bcd25a Compare July 30, 2025 07:30

EtcdServer: config from file - separate variable is used for json par…

d7ec16e

…sing Signed-off-by: sprochazka <[email protected]>

BBQing force-pushed the unmarshall_duration branch from 2bcd25a to d7ec16e Compare July 30, 2025 07:48

Etcdserver: Added license headers to new config_duration files.

a635dc9

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

BBQing force-pushed the unmarshall_duration branch from c83387a to a635dc9 Compare July 30, 2025 08:08

k8s-ci-robot added size/L and removed size/M labels Jul 30, 2025

Etcdserver: Added testcases for duration

2e78494

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

BBQing force-pushed the unmarshall_duration branch from 1c07ac7 to 2e78494 Compare July 30, 2025 12:59

ivanvc reviewed Jul 30, 2025

View reviewed changes

EtcdServer: used plain type to wrap instead of struct, renamed wrappe…

567d35a

…d Duration to ConfigDuration Signed-off-by: sprochazka <[email protected]>

BBQing mentioned this pull request Aug 2, 2025

parameter no effect #20429

Open

4 tasks

ahrtr self-requested a review August 8, 2025 10:52

		WatchProgressNotifyInterval time.Duration
		WatchProgressNotifyIntervalJSON Duration `json:"watch-progress-notify-interval"`

Fixed issue with duration strings used in yaml config file #20360

Are you sure you want to change the base?

Fixed issue with duration strings used in yaml config file #20360

Uh oh!

Conversation

BBQing commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jul 18, 2025

Uh oh!

k8s-ci-robot commented Jul 18, 2025

Uh oh!

Uh oh!

Uh oh!

BBQing commented Jul 20, 2025

Uh oh!

ivanvc commented Jul 25, 2025

Uh oh!

Uh oh!

ivanvc commented Jul 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanvc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ivanvc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanvc commented Aug 1, 2025

Uh oh!

BBQing commented Aug 5, 2025

Uh oh!

ahrtr commented Aug 10, 2025

Uh oh!

Uh oh!

BBQing commented Jul 18, 2025 •

edited

Loading

codecov bot commented Jul 25, 2025 •

edited

Loading