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 statefulset conversion #87706

Merged
merged 2 commits into from Feb 1, 2020

Conversation

@liggitt
Copy link
Member

liggitt commented Jan 30, 2020

What type of PR is this?
/kind bug
/priority critical-urgent

What this PR does / why we need it:
Fixes apiVersion/kind defaulting of the PVC object nested within statefulsets to match what conversion accomplished for us in 1.16.

Also updated local-up-cluster defaults to match kube-apiserver defaults (this problem was only reproducible with a storage type of protobuf)

Which issue(s) this PR fixes:
Fixes #87583

Does this PR introduce a user-facing change?:

Fix regression in statefulset conversion which prevented applying a statefulset multiple times.

/cc @apelisse @wojtek-t
/sig apps

@apelisse

This comment has been minimized.

Copy link
Member

apelisse commented Jan 30, 2020

/lgtm

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Jan 30, 2020

Why did proto storage have a different behavior?

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 30, 2020

@smarterclayton can confirm, but my understanding is that JSON encoding is self-identifying. Proto storage encodes apiVersion and kind separately from the rest of the object (and therefore omits encoding apiVersion/kind fields directly) so the correct type can be determined to decode the rest of the bytes to. The handling of that is done specially for top-level items, and that special handling wasn't in place for this nested type.

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Jan 30, 2020

Ah, we tell the proto encoder to ignore those fields.

We probably need to have a different top level object for embedding; this change fixes up the current data but isn't future proof if these fields could hold more than one value.

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 30, 2020

this change fixes up the current data but isn't future proof if these fields could hold more than one value.

actually, I think I need to rework this to be a custom conversion to get round-tripping happy again, and they actually cannot hold more than one value... these are embedded v1 PersistentVolumeClaim objects, so their apiVersion/kind must always be v1/PersistentVolumeClaim

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 30, 2020

there are a few things that caused this that we'll want to avoid:

  • embedding top-level (typemeta-containing) types inside individual object schemas (this was the only one I found in a sweep)
  • using reflect-based conversion which has these unpleasant magical side-effects (@wojtek-t and @smarterclayton are looking at excising that)
@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Jan 30, 2020

@liggitt liggitt force-pushed the liggitt:fix-statefulset-conversion branch from 13af5d9 to 22c8a6c Jan 31, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm label Jan 31, 2020
@liggitt liggitt changed the title Fix statefulset conversion WIP - Fix statefulset conversion Jan 31, 2020
@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 31, 2020

Switched to conversion-based approach, round-trip tests pass. Will add unit tests specifically around this issue, then will be ready for review

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 31, 2020

added an integration test to exercise the whole decoding/defaulting/conversion/storage stack.

on master:

--- FAIL: TestVolumeTemplateNoopUpdate (3.24s)
    testserver.go:181: runtime-config=map[api/all:true]
    testserver.go:182: Starting kube-apiserver on port 50401...
    testserver.go:198: Waiting for /healthz to be ok...
    statefulset_test.go:106: StatefulSet.apps "web" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', and 'updateStrategy' are forbidden

with this fix:

--- PASS: TestVolumeTemplateNoopUpdate (3.77s)
    testserver.go:181: runtime-config=map[api/all:true]
    testserver.go:182: Starting kube-apiserver on port 50544...
    testserver.go:198: Waiting for /healthz to be ok...
@liggitt liggitt force-pushed the liggitt:fix-statefulset-conversion branch from 22c8a6c to 20ced42 Jan 31, 2020
@liggitt liggitt changed the title WIP - Fix statefulset conversion Fix statefulset conversion Jan 31, 2020
@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 31, 2020

/retest

@wojtek-t

This comment has been minimized.

Copy link
Member

wojtek-t commented Jan 31, 2020

using reflect-based conversion which has these unpleasant magical side-effects (@wojtek-t and @smarterclayton are looking at excising that)

Yeah - I'm trying to get rid of it completely and I'm actually not that far from it. Though I'm always lacking time for it...

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 31, 2020

/retest

@liggitt liggitt force-pushed the liggitt:fix-statefulset-conversion branch from 20ced42 to 82107ff Jan 31, 2020
@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 31, 2020

fixed staticcheck error in test file

@wojtek-t

This comment has been minimized.

Copy link
Member

wojtek-t commented Jan 31, 2020

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 31, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, 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:

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

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 31, 2020

/retest

1 similar comment
@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 31, 2020

/retest

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 31, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 536c2c8 into kubernetes:master Feb 1, 2020
18 checks passed
18 checks passed
cla/linuxfoundation liggitt authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-kind-ipv6-parallel Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-e2e-kind-ipv6 Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Feb 1, 2020
@liggitt liggitt deleted the liggitt:fix-statefulset-conversion branch Feb 3, 2020
k8s-ci-robot added a commit that referenced this pull request Feb 4, 2020
…6-upstream-release-1.17

Automated cherry pick of #87706: Fix statefulset conversion #87706
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.