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

Crd versioning with nop Conversion #63830

Merged
merged 5 commits into from May 23, 2018

Conversation

@mbohlool
Copy link
Member

mbohlool commented May 14, 2018

Implements Custom Resource Definition versioning according to design doc.

Note: I recreated this PR instead of #63518. Huge number of comments there broke github.

Follow-ups: #64136

@sttts @nikhita @deads2k @liggitt @lavalamp

Add CRD Versioning with NOP converter
@@ -25,6 +25,9 @@ type CustomResourceDefinitionSpec struct {
// Group is the group this resource belongs in
Group string
// Version is the version this resource belongs in
// Should be always first item in Versions field if provided.
// Optional, but at least one of Version or Versions must be set.
// Deprecated: Please use `Versions`.

This comment has been minimized.

@sttts

sttts May 15, 2018

Contributor

commented before: why do we keep this field in the internal types? Let's use conversion to recreate it it from the Versions slice

This comment has been minimized.

@sttts

sttts May 15, 2018

Contributor

We discussed offline: this is needed because validation is done on internal types. Without Version here we would have to validate in conversion that it matches the first item in Versions.

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented May 15, 2018

Second commit has a misleading commit msg.


unstructOut.SetUnstructuredContent(unstructIn.UnstructuredContent())

_, err := c.ConvertToVersion(unstructOut, unstructOut.GroupVersionKind().GroupVersion())

This comment has been minimized.

@sttts

sttts May 15, 2018

Contributor

it looks wrong: we cannot expect that the out object has a group version set, can we?

This comment has been minimized.

@mbohlool

mbohlool May 15, 2018

Author Member

We just set the content of the out from in object. if In object has gv set, the out would have it too, right?

This comment has been minimized.

@sttts

sttts May 15, 2018

Contributor

Isn't this a noop then if input and output GV matches?

This comment has been minimized.

@liggitt

liggitt May 15, 2018

Member

Isn't this a noop then if input and output GV matches?

yes, that's a bug. any info we expect to get from unstructOut must be retrieved before we stomp it with unstructOut.SetUnstructuredContent(unstructIn.UnstructuredContent())

if we can't determine the target group version, we have to return an error

at least in some cases, the context arg is used to pass version info, though I'm not sure how normative that is:

if err := c.convertor.Convert(obj, into, c.decodeVersion); err != nil {
return nil, gvk, err
}

This comment has been minimized.

@sttts

sttts May 15, 2018

Contributor

That line is the only one calling ObjectConverter.Convert (outside of parameter codecs). So we know pretty well what the context is there.

This comment has been minimized.

@mbohlool

mbohlool May 15, 2018

Author Member

Isn't this a noop then if input and output GV matches?

I think the assumption (at least from UnstructuredConverter implementation) is they are the same gvk. I am calling ConvertToVersion to take care of the list case where GVKs are the same but list items may not.

func (c *nopConverter) ConvertToVersion(in runtime.Object, target runtime.GroupVersioner) (runtime.Object, error) {
var err error
// Run the converter on the list items instead of list itself
if meta.IsListType(in) {

This comment has been minimized.

@sttts

sttts May 15, 2018

Contributor

this returns true as soon as there is a items part of a CR. Use a cast to UnstructuredList here instead as @lavalamp proposed before.

This comment has been minimized.

@mbohlool

mbohlool May 15, 2018

Author Member

Oh, I should use the cast to unstructuredList here too.

}

func (c *nopConverter) convertToVersion(in runtime.Object, target runtime.GroupVersioner) error {
if kind := in.GetObjectKind().GroupVersionKind(); !kind.Empty() {

This comment has been minimized.

@sttts

sttts May 15, 2018

Contributor

why is it correct to do nothing in case of an empty kind?

This comment has been minimized.

@sttts

sttts May 15, 2018

Contributor

Compare mbohlool#5.

At https://github.com/sttts/kubernetes/blob/2c1a689952ec34e3f9ecb7bcd1772c3fa35c9597/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go#L80 the patch endpoint code call the NewFunc of the CR store which is an empty Unstructure in this case. This ends up here in the converter. To not fall over, we have this if clause.

This comment has been minimized.

@mbohlool

mbohlool May 15, 2018

Author Member

ref: #63880

This comment has been minimized.

@sttts

sttts May 16, 2018

Contributor

#63880 has merged. The empty kind exception here can go.

@@ -30,6 +30,7 @@ import (
"github.com/go-openapi/validate"
"github.com/golang/glog"

"k8s.io/apiextensions-apiserver/pkg/apiserver/conversion"

This comment has been minimized.

@sttts

sttts May 15, 2018

Contributor

import order is wrong

@@ -640,3 +683,18 @@ func (in crdStorageMap) clone() crdStorageMap {
}
return out
}

type CrdConversionRESTOptionGetter struct {

This comment has been minimized.

@sttts

sttts May 15, 2018

Contributor

this can be private. Also it deserves a go doc.

cast := obj.(*apiextensions.CustomResourceDefinition)
c.enqueueCRD(cast)
UpdateFunc: func(oldObj, newObj interface{}) {
c.enqueueCRD(oldObj.(*apiextensions.CustomResourceDefinition))

This comment has been minimized.

@liggitt

liggitt May 15, 2018

Member

probably worth a comment why we're enqueuing both (to make sure we process removed versions correctly)

Group: groupVersion.Group,
Version: groupVersion.Version,
GroupPriorityMinimum: 1000, // CRDs should have relatively low priority
VersionPriority: int32(100 + len(crd.Spec.Versions) - index), // CRDs should have relatively low priority

This comment has been minimized.

@liggitt

liggitt May 15, 2018

Member

we want this to result in 100 for CRDs with exactly one version to avoid dueling old API servers. This produces 101.

also, this comment was outstanding from the previous PR:

what happens if two CRDs define the same group versions, but in opposite order?

For example:

  • CRD A defines group=foo, versions=v1,v2
  • CRD B defines group=foo, versions=v2,v1

What should the versionpriority for foo/v1 and foo/v2 be? How do we ensure the version priority for the API service is deterministic no matter which CRD happens to come first when searching?

One possibility is to keep track of the version priority calculated for a particular CRD but keep searching through all the CRDs in case any others define it with a higher priority. Highest priority is used. That keeps it deterministic, and if all CRDs for a group define the same versions in the same order, that becomes the order in discovery

This comment has been minimized.

@mbohlool

mbohlool May 15, 2018

Author Member

I replied there for another suggestion. What if we add Priority field to CustomResourceDefinitionVersion type? If the user does not provide Priority then we can fill it for them (if that is an accepted pattern in our APIs). By having Priority field, there is a way for our users to keep order in any way they want.

This comment has been minimized.

@liggitt

liggitt May 15, 2018

Member

What if we add Priority field to CustomResourceDefinitionVersion type?

whether it is computed or explicitly set, there is still the possibility of the two versions having flipped priorities in two CRDs. we need to make the outcome of that situation deterministic.

This comment has been minimized.

@sttts

sttts May 16, 2018

Contributor

Let's step back a bit here:

Why can different CRD resources have different version orders?

I want to argue that we should not support this. The same way that names must not overlap between different CRD objects, neither should versions have a mismatch (= being in different order; of course it is fine that different CRDs have different version subsets, but the order should match).

Any algorithm to create a stable order is wrong for the CRD author in certain cases: the order will potentially not match his intent.

With names we postponed the verification of the not-overlapping invariant into the NamingController which sets the NamesAccepted condition. Clearly, it can only use a temporary snapshots of the CRDs it knows (fromt the lister), so we don't have consistency there as we don't have transactions.

I want to argue further that version order is far less critical than names. Hence:

Why don't we add a VersionsAccepted condition from a VersionsController and enforce matching version orders?

This comment has been minimized.

@sttts

sttts May 16, 2018

Contributor

I'm fine with the controller as long as it's HA safe...

It is as safe as the existing naming controller (i.e. there is a race in HA). The assumption is that CRDs are statically created by developers. Even if in an HA environment it can go wrong (= a conflict is not detected and the new CRD versions are served) in 1 out of 100 cases, it will be noticed in 99 cases, and the developer will fix the order.

On the other hand, an algorithm will silently compute some order (which does not match the intend) and the developer of the API group (assuming it is one owner of the group) will not notice. I am not sure I want to trade HA consistency with having a behaviour that the user expects and understands.

This comment has been minimized.

@deads2k

deads2k May 16, 2018

Contributor

Stefan's makes a good point about a stable order algorithm leading to action at a distance. If you modify your CRD with another version and create a conflict, the stable ordering algorithm may make what you're looking at succeed while breaking something else.

By having a proper condition for it, the action happens for your change. You don't see what you expect, you check status and you see why.

It'll be fairly hard to take back the choice and fix behavior later (see TPR to CRD). I think we should learn from the naming problems and take the proper status an reconciliation approach.

@@ -56,7 +64,7 @@ func TestHandleVersionUpdate(t *testing.T) {
Group: "group.com",
Version: "v1",
GroupPriorityMinimum: 1000,
VersionPriority: 100,
VersionPriority: 101,

This comment has been minimized.

@liggitt

liggitt May 15, 2018

Member

this is demonstrating a change that is going to fight old API servers

// TODO: should this be a typed error?
return fmt.Errorf("%v is unstructured and is not suitable for converting to %q", kind, target)
}
if !apiextensions.HasCRDVersion(c.crd, gvk.Version) {

This comment has been minimized.

@liggitt

liggitt May 15, 2018

Member

a general pattern I expected to see was to do "expensive" work when setting up the convertor/handler in response to CRD changes, rather than in the actual Convert/Handle methods.

for example:

// NewCRDConverter returns a new CRD converter based on the conversion settings in crd object.
func NewCRDConverter(crd *apiextensions.CustomResourceDefinition) runtime.ObjectConvertor {
  validVersions := map[schema.GroupVersion]bool{}
  for _, version := range crd.Spec.Versions {
    validVersions[schema.GroupVersion{Group:crd.Spec.Group, Version: version.Name}] = true
  }

  return &nopConverter{
    clusterScoped: crd.Spec.Scope == apiextensions.ClusterScoped,
    validVersions: validVersions,
  }
}

and in the actual noop conversion:

inGV := in.GroupVersionKind().GroupVersion()
if _, ok := c.validVersions[inGV]; !ok {
  ... return unknown version error ...
}

outGV := out.GroupVersionKind().GroupVersion()
if _, ok := c.validVersions[outGV]; !ok {
  ... return unknown version error ...
}

... set content and stomp version ...
return nil

it looks like the handler setup was close to that already (set up maps from versions to storages, scopes, etc), which is good.

This comment has been minimized.

@mbohlool

mbohlool May 15, 2018

Author Member

I think the setup is already like that. This function call apiextensions.HasCRDVersion(c.crd, gvk.Version) is not expensive and also replaced another check existed before. We need to do that because the handler is for all CRDs with all versions. When we pass this, in the getCrdInfo function, we have the map and that map is pre-populated with all valid versions.

This comment has been minimized.

@liggitt

liggitt May 15, 2018

Member

structuring the convertor so that all in/out transformations are determined at construction time makes for O(1) lookups in Convert(), and as convertors become more complex (declarative transformation, webhook, etc), we'll only want to do that complex setup (building the transforming functions, the webhook client, etc) once at construction as well.

This comment has been minimized.

@mbohlool

mbohlool May 15, 2018

Author Member

Oh, I see. I assumed this line is in handler but it is in nop_converter. My bad. It make sense now.

This comment has been minimized.

@mbohlool

mbohlool May 15, 2018

Author Member

~~~ValidVersions's key does not need to include group though.~~~ Also we do not check outGV anywhere in nop_converter as the assumption is we just copy the whole content of in into out.

This comment has been minimized.

@liggitt

liggitt May 15, 2018

Member

we do not check outGV anywhere in nop_converter

we should. the convertor's job is to convert from inGV to outGV... making sure outGV is in the list of valid versions is part of that.

if !apiextensions.HasCRDVersion(c.crd, gvk.Version) {
return fmt.Errorf("request to convert CRD to an invalid version: %s", gvk.String())
}
in.GetObjectKind().SetGroupVersionKind(gvk)

This comment has been minimized.

@liggitt

liggitt May 15, 2018

Member

why are we changing the GVK of the incoming object?

This comment has been minimized.

@mbohlool

mbohlool May 15, 2018

Author Member

ObjectConverter.ConvertToVersion comment says:

// This method does not guarantee that the in object is not mutated.

leaving the door open to mutate in object and return it. Do you think it is better to duplicate it? We do not have ObjectCreator interface in converter but we can pass it here if we need to.

This comment has been minimized.

@sttts

sttts May 15, 2018

Contributor

This is the no-op conversion. Its only task is to set the GVK of an Unstructured. Both Convert and ConvetToVersion allow mutation of the incoming object.

This comment has been minimized.

@liggitt

liggitt May 15, 2018

Member

oh, I misread the diff and thought this was part of the Convert(in, out runtime.Object) method and was confused why we were messing with in, not out. this is fine

@@ -96,7 +96,21 @@ func (c *DiscoveryController) sync(version schema.GroupVersion) error {
Version: crd.Spec.Version,
})

if crd.Spec.Version != version.Version {
foundThisVersion := false
for _, v := range crd.Spec.Versions {

This comment has been minimized.

@liggitt

liggitt May 15, 2018

Member

if we're iterating over and adding all served versions, we can drop the append on line 94 of spec.Version, right?

This comment has been minimized.

@sttts

sttts May 15, 2018

Contributor

+1

}
}

if !foundThisVersion {
continue
}
foundVersion = true

This comment has been minimized.

@liggitt

liggitt May 15, 2018

Member

with multiple versions supported, this statement is no longer true:

Versions: apiVersionsForDiscovery,
// the preferred versions for a group is arbitrary since there cannot be duplicate resources
PreferredVersion: apiVersionsForDiscovery[0],

the ordering of apiVersionsForDiscovery needs to reflect the ordering of the versions in the CRDs for the group (and the open question about conflicting order needs resolving).

c.queue.Add(schema.GroupVersion{Group: obj.Spec.Group, Version: obj.Spec.Version})
for _, v := range obj.Spec.Versions {
c.queue.Add(schema.GroupVersion{Group: obj.Spec.Group, Version: v.Name})
}
}

func (c *DiscoveryController) addCustomResourceDefinition(obj interface{}) {

This comment has been minimized.

@liggitt

liggitt May 15, 2018

Member

same change needed to enqueue both old and new in updateCustomResourceDefinition?

mbohlool added some commits May 7, 2018

Do not bypass same version unstructed conversion if it is a list
This give the converter a chance to convert list items to the requested version.

@mbohlool mbohlool force-pushed the mbohlool:crd_versioning_nop branch from 2b7d8db to c25514a May 22, 2018

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented May 22, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 22, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 22, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mbohlool, sttts

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

@mbohlool

This comment has been minimized.

Copy link
Member Author

mbohlool commented May 22, 2018

/retest

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented May 22, 2018

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented May 22, 2018

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 22, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@liggitt @mbohlool

Pull Request Labels
  • sig/api-machinery: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/feature: New functionality.
Help
@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 23, 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 May 23, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 45c94a1 into kubernetes:master May 23, 2018

15 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation mbohlool authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@ayj ayj referenced this pull request Jun 27, 2018

Closed

Move networking APIs to beta1 #6613

@liggitt liggitt referenced this pull request Aug 24, 2018

Merged

CRD webhook conversion #67006

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.