Skip to content
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

Fix watch event on case of delete #396

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mhmxs
Copy link

@mhmxs mhmxs commented Jan 10, 2025

This change targets two small but significant issues. I discovered this by running wide variety of Kubernetes e2e tests and dag into cache how it works.

  • Kubernetes doesn't like nil Kv or PreKv, except for deletion where it is mandatory to be nil
  • In case of deletion it is looking for PrevKv, but log structured sends Kv. I know it is possible to fix this at logstructured.go but in this way Kine should guarantee the right behavior without depending on plugin correctness.

WDYT?

@mhmxs mhmxs requested a review from a team as a code owner January 10, 2025 10:12
@brandond
Copy link
Member

Can you open an issue describing what specific issues you saw and how to reproduce them?

@mhmxs
Copy link
Author

mhmxs commented Jan 10, 2025

Can you open an issue describing what specific issues you saw and how to reproduce them?

@brandond sorry my memories are ... so the problem is not in logstructured.go.

I created a driver implementation based on

type Backend struct {

kubernetes e2e tests failed, and after some debugging, i found this:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher.go#L693

So current object is not necessary, old object is mandatory, otherwise it fails to detect event type here:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher.go#L626

Two things came into my mind:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants