fix for #19066 Print warnings when deprecated options are configured in config file #19148

mansoor17syed · 2025-01-09T08:43:35Z

fix for #19066 Print warnings when deprecated options are configured in config file

Issue Details
Add warnings when deprecated options are configured
Problem: Currently, etcd doesn't warn users when they configure deprecated options
Goal: Improve user experience by warning about deprecated configurations before they're removed
Solution Overview

Implementation Location: server/config/config.go

k8s-ci-robot · 2025-01-09T08:43:45Z

Hi @mansoor17syed. 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.

serathius · 2025-01-09T09:51:31Z

/ok-to-test

codecov · 2025-01-09T10:09:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.89%. Comparing base (d5231c7) to head (5c0f709).
Report is 39 commits behind head on main.

Additional details and impacted files

Files with missing lines	Coverage Δ
server/etcdmain/config.go	`71.05% <100.00%> (+3.28%)`	⬆️

... and 32 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19148      +/-   ##
==========================================
+ Coverage   68.77%   68.89%   +0.12%     
==========================================
  Files         420      420              
  Lines       35640    35641       +1     
==========================================
+ Hits        24510    24556      +46     
+ Misses       9704     9667      -37     
+ Partials     1426     1418       -8

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 3388e2b...5c0f709. Read the comment docs.

ahrtr · 2025-01-09T14:09:24Z

This PR doesn't fix #19066 . Please follow #18998 (comment)

Thanks anyway

mansoor17syed · 2025-01-09T14:28:49Z

Hi @ahrtr

Thank you for the review and feedback!

I initially assumed that the comment in the earlier PR referred specifically to their changes. Could you please confirm if the fix for issue #19066 is intended to address the comment you provided here : #18998 (comment)

Appreciate your clarification!

ahrtr · 2025-01-09T14:41:37Z

Could you please confirm if the fix for issue #19066 is intended to address the comment you provided here : #18998 (comment)

YES

ahrtr · 2025-01-09T15:20:25Z

I just realised that we can iterate FlagsExplicitlySet directly in parse(). Both configFromFile and configFromCmdLine populate the data structure with all flags,

etcd/server/embed/config.go

Line 486 in 75f2ae1

FlagsExplicitlySet map[string]bool

sorry for the back and forth.

mansoor17syed · 2025-01-09T17:40:28Z

@ahrtr

Can you Please have a look once now if I have done the right changes
I see one tests failing , but I guess the failure is not because of my changes

Still open for feedback and suggestions
do let me know

ahrtr · 2025-01-09T17:44:49Z

@mansoor17syed The change looks good. Please squash the commits

ahrtr · 2025-01-09T17:46:19Z

Could you please also add a test to cover the config file case?

mansoor17syed · 2025-01-10T06:11:58Z

@ahrtr

can chk now I have added the UT but some seems failing I doubt its because of my changes.
Still open for feedback and suggestions
do let me know

server/etcdmain/config_test.go

ahrtr · 2025-01-10T10:06:23Z

Please squash the commits.

mansoor17syed · 2025-01-10T11:58:36Z

@ahrtr @serathius
can you Please have a look into it now, addressed all the review comments and squashed the commits.

ahrtr · 2025-01-10T12:27:15Z

/test pull-etcd-robustness-arm64

mansoor17syed · 2025-01-10T13:19:59Z

I don’t seem to have the permissions to merge this. @ahrtr Could you kindly take care of it?
Thanks so much for all your support and valuable feedback! 😊

ahrtr · 2025-01-10T13:31:37Z

Usually a PR needs two approval to be merged. Let's wait one of other maintainers or reviewers to take a look. thx

serathius · 2025-01-10T15:01:39Z

server/etcdmain/config_test.go

+			}
+
+			// Compare sets of flags
+			if !reflect.DeepEqual(foundFlags, tc.expectedFlags) {


Why not use assert.Equal?

serathius · 2025-01-10T15:02:38Z

server/etcdmain/config_test.go

+
+			// Special check for experimental-warning-unary-request-duration
+			if tc.configFileYAML.ExperimentalWarningUnaryRequestDuration != 0 {
+				t.Log("Note: experimental-warning-unary-request-duration deprecation is handled separately")


Why log here? if we want to leave context a comment would be enough.

Ok will chk this

mansoor17syed · 2025-01-10T16:18:59Z

@serathius

can you have a look now ?
Do let me know for any rework

k8s-ci-robot · 2025-01-13T08:32:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, mansoor17syed, serathius

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ahrtr,serathius]

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

serathius · 2025-01-13T08:33:13Z

etcdmain/config_test.go:30: File is not `goimports`-ed with -local go.etcd.io (goimports)
	"github.com/stretchr/testify/assert"
etcdmain/config_test.go:734:4: SA9003: empty branch (staticcheck)
			if tc.configFileYAML.ExperimentalWarningUnaryRequestDuration != 0 {
			^
etcdmain/config_test.go:730:4: formatter: use assert.Equalf (testifylint)
			assert.Equal(t, tc.expectedFlags, foundFlags, "deprecated flags mismatch")

Use minimal config struct for YAML marshaling. Replace custom mapToSortedSlice with standard library functions. Fix flag verification for deprecated experimental options. Fixes etcd-io#19066 Signed-off-by: mansoora <[email protected]>

mansoor17syed · 2025-01-13T10:32:17Z

@serathius

I have addressed the review comments and fixed the issues. However, it seems some tests are still failing, although they don’t appear to be related to my changes. I'm open to any feedback and willing to work on it further if needed. Let me know your thoughts!

serathius · 2025-01-13T11:13:28Z

/retest

mansoor17syed · 2025-01-13T12:57:42Z

I don’t seem to have the permissions to merge this. @serathius Could you kindly take care of it?
Thanks so much for all your support and valuable feedback! 😊

k8s-ci-robot added the needs-ok-to-test label Jan 9, 2025

k8s-ci-robot added the size/M label Jan 9, 2025

mansoor17syed force-pushed the issue_19066 branch from 810eed4 to f5cdee6 Compare January 9, 2025 08:48

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jan 9, 2025

mansoor17syed force-pushed the issue_19066 branch from 56f54b1 to f929a61 Compare January 9, 2025 15:14

k8s-ci-robot added size/XS size/S and removed size/M size/XS labels Jan 9, 2025

mansoor17syed force-pushed the issue_19066 branch from f340ceb to 1d4f997 Compare January 9, 2025 16:58

k8s-ci-robot added size/L and removed size/S labels Jan 9, 2025

mansoor17syed force-pushed the issue_19066 branch 3 times, most recently from 98f23c0 to 70b0e50 Compare January 10, 2025 05:14

ahrtr reviewed Jan 10, 2025

View reviewed changes

server/etcdmain/config_test.go Outdated Show resolved Hide resolved

server/etcdmain/config_test.go Outdated Show resolved Hide resolved

server/etcdmain/config_test.go Outdated Show resolved Hide resolved

mansoor17syed force-pushed the issue_19066 branch from 2fca19d to d83f391 Compare January 10, 2025 11:25

ahrtr approved these changes Jan 10, 2025

View reviewed changes

k8s-ci-robot added the approved label Jan 10, 2025

serathius reviewed Jan 10, 2025

View reviewed changes

mansoor17syed force-pushed the issue_19066 branch from f8c0bf3 to d398945 Compare January 11, 2025 04:41

serathius approved these changes Jan 13, 2025

View reviewed changes

test: fix TestConfigFileDeprecatedOptions

5c0f709

Use minimal config struct for YAML marshaling. Replace custom mapToSortedSlice with standard library functions. Fix flag verification for deprecated experimental options. Fixes etcd-io#19066 Signed-off-by: mansoora <[email protected]>

mansoor17syed force-pushed the issue_19066 branch from df09737 to 5c0f709 Compare January 13, 2025 09:43

serathius merged commit 5890801 into etcd-io:main Jan 13, 2025
34 checks passed

ahrtr mentioned this pull request Jan 13, 2025

migrate flag experimental-corrupt-check-time to use corrupt-check-time #19156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for #19066 Print warnings when deprecated options are configured in config file #19148

fix for #19066 Print warnings when deprecated options are configured in config file #19148

mansoor17syed commented Jan 9, 2025

k8s-ci-robot commented Jan 9, 2025

serathius commented Jan 9, 2025

codecov bot commented Jan 9, 2025 •

edited

Loading

ahrtr commented Jan 9, 2025

mansoor17syed commented Jan 9, 2025

ahrtr commented Jan 9, 2025

ahrtr commented Jan 9, 2025

mansoor17syed commented Jan 9, 2025

ahrtr commented Jan 9, 2025

ahrtr commented Jan 9, 2025

mansoor17syed commented Jan 10, 2025

ahrtr commented Jan 10, 2025

mansoor17syed commented Jan 10, 2025

ahrtr commented Jan 10, 2025

mansoor17syed commented Jan 10, 2025

ahrtr commented Jan 10, 2025

serathius Jan 10, 2025

serathius Jan 10, 2025

mansoor17syed Jan 10, 2025

mansoor17syed commented Jan 10, 2025

k8s-ci-robot commented Jan 13, 2025

serathius commented Jan 13, 2025

mansoor17syed commented Jan 13, 2025

serathius commented Jan 13, 2025

mansoor17syed commented Jan 13, 2025

fix for #19066 Print warnings when deprecated options are configured in config file #19148

fix for #19066 Print warnings when deprecated options are configured in config file #19148

Conversation

mansoor17syed commented Jan 9, 2025

k8s-ci-robot commented Jan 9, 2025

serathius commented Jan 9, 2025

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

ahrtr commented Jan 9, 2025

mansoor17syed commented Jan 9, 2025

ahrtr commented Jan 9, 2025

ahrtr commented Jan 9, 2025

mansoor17syed commented Jan 9, 2025

ahrtr commented Jan 9, 2025

ahrtr commented Jan 9, 2025

mansoor17syed commented Jan 10, 2025

ahrtr commented Jan 10, 2025

mansoor17syed commented Jan 10, 2025

ahrtr commented Jan 10, 2025

mansoor17syed commented Jan 10, 2025

ahrtr commented Jan 10, 2025

serathius Jan 10, 2025

Choose a reason for hiding this comment

serathius Jan 10, 2025

Choose a reason for hiding this comment

mansoor17syed Jan 10, 2025

Choose a reason for hiding this comment

mansoor17syed commented Jan 10, 2025

k8s-ci-robot commented Jan 13, 2025

serathius commented Jan 13, 2025

mansoor17syed commented Jan 13, 2025

serathius commented Jan 13, 2025

mansoor17syed commented Jan 13, 2025

codecov bot commented Jan 9, 2025 •

edited

Loading