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

Remove manually written typed registries #66041

Merged
merged 3 commits into from Jul 16, 2018

Conversation

@liggitt
Copy link
Member

liggitt commented Jul 10, 2018

These were only used in a handful of places, and were not consistently available for all types.

They add a lot of call sites for PRs like #65105 and are not generally useful (very few callers have the ability to construct the underlying store).

This PR switches the scale subresources to use the underlying store directly (like the status subresources already were), and removes the manually written Registry impls.

/sig api-machinery
/kind cleanup
/assign @deads2k

/hold
will hold for #65105 and rebase after that

NONE
@@ -119,16 +120,16 @@ func NewLeases(storage storage.Interface, baseKey string, leaseTime time.Duratio
}

type leaseEndpointReconciler struct {
endpointRegistry endpoint.Registry
endpointStorage *genericregistry.Store

This comment has been minimized.

@jennybuckley

jennybuckley Jul 11, 2018

Contributor

maybe this could be a rest.StandardStorage instead of *genericregistry.Store and then the registrytest could just change its signatures slightly to get the tests to compile

This comment has been minimized.

@liggitt

liggitt Jul 13, 2018

Author Member

sounds good

@liggitt liggitt force-pushed the liggitt:manual-registries branch from c674709 to 1a43e9b Jul 13, 2018

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jul 13, 2018

rebased
/hold cancel

liggitt added some commits Jul 10, 2018

@liggitt liggitt force-pushed the liggitt:manual-registries branch from 1a43e9b to 652e2dc Jul 13, 2018

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jul 13, 2018

/retest

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Jul 16, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 16, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 16, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt

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:

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 16, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 16, 2018

Automatic merge from submit-queue (batch tested with PRs 66158, 66041, 66210). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 43b801d into kubernetes:master Jul 16, 2018

6 of 17 checks passed

pull-kubernetes-bazel-build Job triggered.
Details
pull-kubernetes-bazel-test Job triggered.
Details
pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job triggered.
Details
pull-kubernetes-e2e-kops-aws Job triggered.
Details
pull-kubernetes-integration Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
pull-kubernetes-node-e2e Job triggered.
Details
pull-kubernetes-typecheck Job triggered.
Details
pull-kubernetes-verify Job triggered.
Details
Submit Queue Running github e2e tests a second time.
Details
cla/linuxfoundation liggitt authorized
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 16, 2018

@liggitt: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-integration 652e2dc link /test pull-kubernetes-integration
pull-kubernetes-e2e-kops-aws 652e2dc link /test pull-kubernetes-e2e-kops-aws
pull-kubernetes-e2e-gce 652e2dc link /test pull-kubernetes-e2e-gce
pull-kubernetes-kubemark-e2e-gce-big 652e2dc link /test pull-kubernetes-kubemark-e2e-gce-big

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

@liggitt liggitt deleted the liggitt:manual-registries branch Jul 20, 2018

@@ -200,10 +201,11 @@ func (r *ScaleREST) Update(ctx context.Context, name string, objInfo rest.Update

rs.Spec.Replicas = scale.Spec.Replicas
rs.ResourceVersion = scale.ResourceVersion
rs, err = r.registry.UpdateReplicaSet(ctx, rs, createValidation, updateValidation, options)
obj, _, err = r.store.Update(ctx, rs.Name, rest.DefaultUpdatedObjectInfo(rs), createValidation, updateValidation, false, options)

This comment has been minimized.

@sttts

sttts Aug 30, 2018

Contributor

@liggitt isn't here a race between multiple updating actors?

Assume:

  • the request comes in with scale resourceVersion=50 (maybe because I am faking the resourceVersion)
  • L174: r.store.Get returns resourceVersion=42
  • L203: overrides rs.resourceVersion=50 which is actually based on resourceVesion=42
  • meanwhile there were actual updates, reaching the real resourceVersion=50
  • L204: update succeeds, wiping all changes out since resourceVersion=42.

This comment has been minimized.

@liggitt

liggitt Aug 30, 2018

Author Member

The incoming request faking the resource version and happening to coincide with the actual resourceVersion isn't a scenario we care about. The same issue would exist with any resource.

This comment has been minimized.

@liggitt

liggitt Aug 30, 2018

Author Member

A more interesting case is when the scale resource version is not set. Stepping back, I think updating the rs from the incoming scale object should happen inside an updatedObjectInfo implementation, and the rs resourceVersion be left as is if the incoming scale object didn't set it. See a similar approach described at #59416 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.