feat(client/v3/naming): add Attributes into Endpoint and deprecate Metadata

[amosehiguese]

This implementation adds the new Attributes *attributes.Attributes into Endpoint and deprecate Metadata accordingly. The metadata field is still supported for backward compatibility

Fixes #19706

cc: @ahrtr @siyuanfoundation

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

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amosehiguese
Once this PR has been reviewed and has the lgtm label, please assign serathius 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]

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

[ahrtr]

please squash the commits.

We also need to update the doc https://etcd.io/docs/v3.5/dev-guide/grpc_naming/

• the Metadata has deprecated, use Attributes instead
• Mark the gRPC naming and discovery feature as experimental, depending on how the resolver package in grpc-go evolves.

[amosehiguese]

Alright. I'll do just that.

[ahrtr]

• the Metadata has deprecated, use Attributes instead

let's update this first together with this PR.

• after this PR gets merged, backport it to 3.6 and 3.5.
• raise a PR in etcd-io/website to update the doc

• Mark the gRPC naming and discovery feature as experimental, depending on how the resolver package in grpc-go evolves.

we will discuss & resolve this separately.

cc @dfawley @fuweid @serathius @ivanvc

[ahrtr]

/ok-to-test

[ahrtr]

@dfawley

Probably eventually we might want to use Endpoint.Attributes instead of Address.Attributes, as we only care about the LB policy?

Currently we assume each endpoint has only one address, but actually it may contain multiple addresses (we won't change the behavior for now).

[amosehiguese]

Yeah, I saw it and then realised that the attributes field in the Endpoint was specific to LB-Policy which is the usecase for etcd and wondered why that wasn't used instead. But I have clarity on that now. Thanks

[ahrtr]

Probably eventually we might want to use Endpoint.Attributes instead of Address.Attributes, as we only care about the LB policy?

It seems that this comment hasn't been resolved yet.

[amosehiguese]

We'll have to wait on @dfawley, I guess

[dfawley]

The attributes on the address are passed to our code that creates and manages the connection to that address. This means if there are two Addresses between name resolver updates with the same IP address but non-equal attributes, we will consider them different and reconnect.

The attributes on the endpoint are for anything else you might want to do. gRPC won't look at your attributes.

One important thing that is still not clear to me: what is your use case for attributes? Why are you adding them into gRPC endpoints/addresses?

[amosehiguese]

Let's do not add Attributes for now...

Given that the current relevance of this field is unclear ATM. Not adding it is the only reasonable call to make.

Just as @dfawley said, "our current philosophy is that if nobody asks for a thing, then we generally don't export it."

So, since etcd is reliant on grpc, taking a similar stance would be advised.

That said, I will kindly leave the deprecated metadata field as is but revert all attempts to introduce the Attributes field.

cc: @ahrtr

[ahrtr]

Yes, this PR should be as simple as just adding a deprecation notice for the Metadata field.

[amosehiguese]

I love the sound of that. On it!

[dfawley]

Is there a path to deleting it from etcd, though? Marking it as deprecated is good, but we ultimately want to delete it. It would be great if there was some way to find out who might be using it, if anyone, and talk with them.

I guess as an intermediate step for gRPC, we could stop reading the field entirely (e.g. clear it as soon as it comes out of the resolver)? Maybe with an environment variable to re-enable the behavior in case of emergency?

[ahrtr]

We will remove the field in 3.7 (main branch). After we backport the change to 3.6 and 3.5, we will remove it from main branch.

I guess as an intermediate step for gRPC, we could stop reading the field entirely (e.g. clear it as soon as it comes out of the resolver)? Maybe with an environment variable to re-enable the behavior in case of emergency?

I think we are good as long as it doesn't cause any compiling error.

[codecov]

Codecov Report

Attention: Patch coverage is 94.44444% with 3 lines in your changes missing coverage. Please review.

Project coverage is 68.25%. Comparing base (27ff3ec) to head (fc5bed3).
Report is 39 commits behind head on main.

❗ Current head fc5bed3 differs from pull request most recent head 2232cb5

Please upload reports for the commit 2232cb5 to get more accurate results.

Files with missing lines	Patch %	Lines
client/v3/naming/resolver/resolver.go	84.21%	2 Missing and 1 partial ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
client/v3/internal/resolver/resolver.go	85.18% <100.00%> (+1.18%)	⬆️
client/v3/naming/endpoints/endpoints.go	100.00% <ø> (ø)
client/v3/naming/endpoints/endpoints_impl.go	75.20% <100.00%> (+3.50%)	⬆️
server/proxy/grpcproxy/register.go	82.35% <100.00%> (+15.68%)	⬆️
client/v3/naming/resolver/resolver.go	77.77% <84.21%> (-1.47%)	⬇️

... and 73 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19785      +/-   ##
==========================================
- Coverage   68.70%   68.25%   -0.45%     
==========================================
  Files         421      406      -15     
  Lines       35858    35465     -393     
==========================================
- Hits        24635    24206     -429     
- Misses       9789     9843      +54     
+ Partials     1434     1416      -18     

---

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 27ff3ec...2232cb5. Read the comment docs.

🚀 New features to boost your workflow:

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

[ahrtr]

You also need to persist the new added attributes (line 41 in internal/update.go), and read it back in List,

etcd/client/v3/naming/endpoints/endpoints_impl.go

Lines 175 to 192 in 8f933a5

	func (m *endpointManager) List(ctx context.Context) (Key2EndpointMap, error) {
	key := m.target + "/"
	resp, err := m.client.Get(ctx, key, clientv3.WithPrefix(), clientv3.WithSerializable())
	if err != nil {
	return nil, err
	}
	eps := make(Key2EndpointMap)
	for _, kv := range resp.Kvs {
	var iup internal.Update
	if err := json.Unmarshal(kv.Value, &iup); err != nil {
	continue
	}
	eps[string(kv.Key)] = Endpoint{Addr: iup.Addr, Metadata: iup.Metadata}
	}
	return eps, nil
	}

[amosehiguese]

let's update this first together with this PR.

Alright.

after this PR gets merged, backport it to 3.6 and 3.5.

I'm on standby.

raise a PR in etcd-io/website to update the doc

I will hold off on this then

[ahrtr]

You might also need to update grpcproxy

etcd/server/proxy/grpcproxy/register.go

Line 75 in 8f933a5

endpoint := endpoints.Endpoint{Addr: addr, Metadata: getMeta()}

[amosehiguese]

You also need to persist the new added attributes (line 41 in internal/update.go), and read it back in List,

etcd/client/v3/naming/endpoints/endpoints_impl.go

Lines 175 to 192 in 8f933a5

	func (m *endpointManager) List(ctx context.Context) (Key2EndpointMap, error) {
	key := m.target + "/"
	resp, err := m.client.Get(ctx, key, clientv3.WithPrefix(), clientv3.WithSerializable())
	if err != nil {
	return nil, err
	}
	eps := make(Key2EndpointMap)
	for _, kv := range resp.Kvs {
	var iup internal.Update
	if err := json.Unmarshal(kv.Value, &iup); err != nil {
	continue
	}
	eps[string(kv.Key)] = Endpoint{Addr: iup.Addr, Metadata: iup.Metadata}
	}
	return eps, nil
	}

Got it.

[amosehiguese]

You might also need to update grpcproxy

Pretty much adjust every other usage of the endpoint struct to account for the new Attributes field.

Will do that.

[amosehiguese]

Two files may also need adjustments but I thought to run it by you first.

👉 endpoints_test.go [integration tests]

etcd/tests/integration/clientv3/naming/endpoints_test.go

Line 46 in 8f933a5

e1 := endpoints.Endpoint{Addr: "127.0.0.1", Metadata: "metadata"}

👉 cluster.go [grpc-proxy]
In MembersList, If proxies are registered, it will lists them using their metadata and endpoint addresses.

etcd/server/proxy/grpcproxy/cluster.go

Line 146 in 8f933a5

m, err := decodeMeta(fmt.Sprint(upt.Metadata))

What would you have me do to these files?

[ahrtr]

I think

• we need update (or add) the test to cover the new field Attributes
• we shouldn't break any usage on the existing field Metadata.

[amosehiguese]

I think

• we need update (or add) the test to cover the new field Attributes
• we shouldn't break any usage on the existing field Metadata.

Alright then.

[ahrtr]

I just mark this as a priority/important-soon task.

As mentioned above #19785 (comment), I think we need to backport this PR to release-3.6 and release-3.5, of course we will evaluate the impact. The goal is to get it included in 3.6.0, so that we can remove it in 3.7; otherwise, we will have to completely get rid of the deprecated Metadata field in 3.8 (~6 years later based on current cadence)

cc @fuweid @serathius @ivanvc @jmhbnz

[ahrtr]

also cc grpc-go tech-lead @dfawley, thx for all the help so far.

[amosehiguese]

@ahrtr
Quick question. Are flaky tests expected in the end-to-end test suite? I’ve been running make test-e2e and sometimes it passes, but other times it fails without me changing anything. Just want to be sure if that’s expected or if something else is going on, because it’s been slowing down my push.

...and those tests take quite a bit of time to run

[amosehiguese]

I just mark this as a priority/important-soon task.

I will be done with this from my end once I'm able to resolve whether or not flaky tests are expected in the e2e suite

[ahrtr]

Quick question. Are flaky tests expected in the end-to-end test suite?

You need to ensure the test failures are not caused by your changes.

[amosehiguese]

Quick question. Are flaky tests expected in the end-to-end test suite?

You need to ensure the test failures are not caused by your changes.

I think I’ll have to run it against the main branch a couple of times to be sure.

[amosehiguese]

Same error occured on the main branch after running successfully in the previous run. Seems to look like a connection timeout.

What do you reckon I do? push?

cc: @ahrtr

[ahrtr]

Same error occured on the main branch after running successfully in the previous run. Seems to look like a connection timeout.

Windows is only tier 3 support, please try to test it on a linux platform.

What do you reckon I do? push?

Pushing to your dev branch won't break anything. Please review your PR yourself first to ensure it's the best you can deliver.

[amosehiguese]

Oh, I am actually working on a linux machine running on Azure cloud. This is just my backup pc, so I am forced to doing everything on the cloud.

Pushing to your dev branch won't break anything. Please review your PR yourself first to ensure it's the best you can deliver.

I definitely will. Thanks for the feedback

[amosehiguese]

@dfawley

Just a bit of clarification in your docs. This line to be precise. Is this meant to be, "Is it impossible..." or "It is impossible"?

https://github.com/grpc/grpc-go/blob/030938e543b4a721cd426d15611d50ecf097dcf3/attributes/attributes.go#L137

cc: @ahrtr

[amosehiguese]

I think

• we need update (or add) the test to cover the new field Attributes

Attempting to do this would break these test cases.

👇

etcd/tests/integration/clientv3/naming/endpoints_test.go

Line 29 in f5dbde1

func TestEndpointManager(t *testing.T) {

👇

etcd/tests/integration/clientv3/naming/endpoints_test.go

Line 120 in f5dbde1

func TestEndpointManagerCRUD(t *testing.T) {

Mostly because of the design decision by grpc on the Attributes type to make it impossible to unmarshal attributes from a JSON representation

cc: @ahrtr

[k8s-ci-robot]

PR needs rebase.

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.

[k8s-ci-robot]

@amosehiguese: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-etcd-unit-test-arm64	2232cb5	link	true	/test pull-etcd-unit-test-arm64
pull-etcd-integration-1-cpu-arm64	2232cb5	link	true	/test pull-etcd-integration-1-cpu-arm64
pull-etcd-release-tests	2232cb5	link	true	/test pull-etcd-release-tests
pull-etcd-e2e-arm64	2232cb5	link	true	/test pull-etcd-e2e-arm64
pull-etcd-unit-test-386	2232cb5	link	true	/test pull-etcd-unit-test-386
pull-etcd-integration-2-cpu-arm64	2232cb5	link	true	/test pull-etcd-integration-2-cpu-arm64
pull-etcd-govulncheck	2232cb5	link	true	/test pull-etcd-govulncheck
pull-etcd-e2e-amd64	2232cb5	link	true	/test pull-etcd-e2e-amd64
pull-etcd-fuzzing-v3rpc	2232cb5	link	true	/test pull-etcd-fuzzing-v3rpc
pull-etcd-integration-2-cpu-amd64	2232cb5	link	true	/test pull-etcd-integration-2-cpu-amd64
pull-etcd-verify	2232cb5	link	true	/test pull-etcd-verify
pull-etcd-integration-4-cpu-amd64	2232cb5	link	true	/test pull-etcd-integration-4-cpu-amd64
pull-etcd-grpcproxy-integration-amd64	2232cb5	link	true	/test pull-etcd-grpcproxy-integration-amd64
pull-etcd-e2e-386	2232cb5	link	true	/test pull-etcd-e2e-386
pull-etcd-contrib-mixin	2232cb5	link	true	/test pull-etcd-contrib-mixin
pull-etcd-build	2232cb5	link	true	/test pull-etcd-build
pull-etcd-integration-1-cpu-amd64	2232cb5	link	true	/test pull-etcd-integration-1-cpu-amd64
pull-etcd-unit-test-amd64	2232cb5	link	true	/test pull-etcd-unit-test-amd64
pull-etcd-coverage-report	2232cb5	link	true	/test pull-etcd-coverage-report
pull-etcd-integration-4-cpu-arm64	2232cb5	link	true	/test pull-etcd-integration-4-cpu-arm64
pull-etcd-robustness-arm64	2232cb5	link	true	/test pull-etcd-robustness-arm64
pull-etcd-grpcproxy-e2e-amd64	2232cb5	link	true	/test pull-etcd-grpcproxy-e2e-amd64
pull-etcd-grpcproxy-e2e-arm64	2232cb5	link	true	/test pull-etcd-grpcproxy-e2e-arm64
pull-etcd-grpcproxy-integration-arm64	2232cb5	link	true	/test pull-etcd-grpcproxy-integration-arm64
pull-etcd-robustness-amd64	2232cb5	link	true	/test pull-etcd-robustness-amd64

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.