KEP-3926: refactor: extract etcd3 store decode functions into an interface #127982

tkashem · 2024-10-10T14:37:59Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Extract the decode and decodeListItem functions of the ectd3 store implementation as is into an interface:

type Decoder interface {
	Decode(codec runtime.Codec, versioner Versioner, value []byte, objPtr runtime.Object, rev int64) error

	DecodeListItem(ctx context.Context, data []byte, rev uint64, codec runtime.Codec, versioner Versioner, newItemFunc func() runtime.Object) (runtime.Object, error)
}

This is a refactor-only PR, no behavioral change.
Promoting the functions to an interface will allow us to wrap it so we can do proper error chaining without making inline changes to the store implementation code.

See #127513 to see the unsafe deletion flow

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

KEP: kubernetes/enhancements#3927

k8s-ci-robot · 2024-10-10T14:38:10Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

tkashem · 2024-10-10T14:39:01Z

/assign @sttts

sttts · 2024-10-10T16:34:40Z

staging/src/k8s.io/apiserver/pkg/storage/interfaces.go

+	// Decode decodes value of bytes into object. It will also
+	// set the object resource version to rev.
+	// On success, objPtr would be set to the object.
+	Decode(codec runtime.Codec, versioner Versioner, value []byte, objPtr runtime.Object, rev int64) error


how generic is that really with a int64 rv? do we have this convention elsewhere yet on the storage level?

Alternative: make rev a string, and let the etcd3 store do the conversion.

and then this is really the resource version, not an etcd rev.

and then we are not far from moving the while default decoder here 🤔

it's not generic, it applies to etcd implementation only, I have put the interface definition in the storage folder for convenience, maybe we can put the interface definition in the storage/etcd3 folder, let me look into it.

okay now it's etcd3.Decoder, not storage.Decoder

tkashem · 2024-10-10T19:12:21Z

staging/src/k8s.io/apiserver/pkg/storage/etcd3/decoder.go

+var _ Decoder = defaultDecoder{}
+
+type defaultDecoder struct{}
+


moved from store.go as is

Jefftree · 2024-10-10T20:07:54Z

/remove-sig api-machinery

sttts · 2024-10-11T11:03:18Z

Can you point me to the PR that uses this? Am unclear why we need a custom decoder.

aojea · 2024-10-11T12:11:45Z

/assign @serathius
/cc @wojtek-t

tkashem · 2024-10-11T12:39:41Z

Can you point me to the PR that uses this? Am unclear why we need a custom decoder.

not a custom decoder, but i need to decorate the current/default one in order to wrap error
491c5cf#diff-1fbc86162ac0d13aef102d5145a4705624bed4e51bd1674850f5aaf24e073334R117-R139

I am doing the same for the transformer which is already interfaced
491c5cf#diff-1fbc86162ac0d13aef102d5145a4705624bed4e51bd1674850f5aaf24e073334R142-R154

Otherwise, i have to find every code site where these functions are invoked and make change inline. So no change in terms of actual decoding

sttts · 2024-10-11T16:58:29Z

Sgtm.

Waiting for @serathius and @wojtek-t to have the chance for feedback.

wojtek-t · 2024-10-14T09:29:13Z

staging/src/k8s.io/apiserver/pkg/storage/etcd3/decoder.go

+	// Decode decodes value of bytes into object. It will also
+	// set the object resource version to rev.
+	// On success, objPtr would be set to the object.
+	Decode(codec runtime.Codec, versioner storage.Versioner, value []byte, objPtr runtime.Object, rev int64) error


The fact that we pass codec as a parameter to Decoder seems a bit weird to me (same for DecodeListItems below).

Shouldn't codec be passed on initialization of Decoder (i.e. codec as a field of defaultDecoder struct) ?

yes, in contrast to the request scoped parameters, both codec and versioner are out of band and shared, so they can be promoted to member variables. Do you want me to add versioner as a field of defaultDecoder as well?

Yeah - versioner should be there too.

okay, done, please take a look when you have a moment

wojtek-t · 2024-10-14T09:31:13Z

Waiting for @serathius and @wojtek-t to have the chance for feedback.

I have one comment about the interface itself: https://github.com/kubernetes/kubernetes/pull/127982/files#r1799072481

tkashem · 2024-10-14T14:19:41Z

/remove-sig api-machinery

tkashem · 2024-10-14T17:22:12Z

/retest

wojtek-t · 2024-10-15T07:26:54Z

/lgtm
/approve

k8s-ci-robot · 2024-10-15T07:26:59Z

LGTM label has been added.

Git tree hash: a6ada18819576457982883234dece962e11d936f

k8s-ci-robot · 2024-10-15T07:27:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tkashem, wojtek-t

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:

~~staging/src/k8s.io/apiserver/pkg/storage/OWNERS~~ [wojtek-t]

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 added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 10, 2024

k8s-ci-robot assigned sttts Oct 10, 2024

k8s-ci-robot requested review from serathius and smarterclayton October 10, 2024 14:40

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/etcd Categorizes an issue or PR as relevant to SIG Etcd. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 10, 2024

sttts reviewed Oct 10, 2024

View reviewed changes

tkashem force-pushed the refactor-store-decoder branch from b5e9090 to ab600da Compare October 10, 2024 19:10

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 10, 2024

tkashem commented Oct 10, 2024

View reviewed changes

tkashem force-pushed the refactor-store-decoder branch from ab600da to 359c3ab Compare October 10, 2024 19:38

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 10, 2024

aaron-prindle mentioned this pull request Oct 10, 2024

Currently a number of files have OWNERS mapped to sig-api-machinery but they should be mapped to new sig-etcd #127336

Open

k8s-ci-robot assigned serathius Oct 11, 2024

k8s-ci-robot requested a review from wojtek-t October 11, 2024 12:11

wojtek-t reviewed Oct 14, 2024

View reviewed changes

wojtek-t self-assigned this Oct 14, 2024

refactor: extract decode functions into an interface for etcd3 store

1d1a656

tkashem force-pushed the refactor-store-decoder branch from 359c3ab to 1d1a656 Compare October 14, 2024 14:05

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 14, 2024

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 14, 2024

tkashem changed the title ~~KEP-4795: refactor: extract etcd3 store decode functions into an interface~~ KEP-3926: refactor: extract etcd3 store decode functions into an interface Oct 14, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2024

k8s-ci-robot merged commit d32e9b0 into kubernetes:master Oct 15, 2024
14 checks passed

k8s-ci-robot added this to the v1.32 milestone Oct 15, 2024

tkashem mentioned this pull request Oct 28, 2024

Handling undecryptable resources kubernetes/enhancements#3926

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-3926: refactor: extract etcd3 store decode functions into an interface #127982

KEP-3926: refactor: extract etcd3 store decode functions into an interface #127982

tkashem commented Oct 10, 2024 •

edited

Loading

k8s-ci-robot commented Oct 10, 2024

tkashem commented Oct 10, 2024

sttts Oct 10, 2024

sttts Oct 10, 2024

sttts Oct 10, 2024

sttts Oct 10, 2024

tkashem Oct 10, 2024 •

edited

Loading

tkashem Oct 10, 2024

tkashem Oct 10, 2024

Jefftree commented Oct 10, 2024

sttts commented Oct 11, 2024

aojea commented Oct 11, 2024

tkashem commented Oct 11, 2024 •

edited

Loading

sttts commented Oct 11, 2024

wojtek-t Oct 14, 2024

tkashem Oct 14, 2024

wojtek-t Oct 14, 2024

tkashem Oct 14, 2024

wojtek-t commented Oct 14, 2024

tkashem commented Oct 14, 2024

tkashem commented Oct 14, 2024

wojtek-t commented Oct 15, 2024

k8s-ci-robot commented Oct 15, 2024

k8s-ci-robot commented Oct 15, 2024

		var _ Decoder = defaultDecoder{}

		type defaultDecoder struct{}

KEP-3926: refactor: extract etcd3 store decode functions into an interface #127982

KEP-3926: refactor: extract etcd3 store decode functions into an interface #127982

Conversation

tkashem commented Oct 10, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Oct 10, 2024

tkashem commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkashem Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jefftree commented Oct 10, 2024

sttts commented Oct 11, 2024

aojea commented Oct 11, 2024

tkashem commented Oct 11, 2024 • edited Loading

sttts commented Oct 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Oct 14, 2024

tkashem commented Oct 14, 2024

tkashem commented Oct 14, 2024

wojtek-t commented Oct 15, 2024

k8s-ci-robot commented Oct 15, 2024

k8s-ci-robot commented Oct 15, 2024

tkashem commented Oct 10, 2024 •

edited

Loading

tkashem Oct 10, 2024 •

edited

Loading

tkashem commented Oct 11, 2024 •

edited

Loading