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

Convert to use versioned api #36673

Merged
merged 27 commits into from
Nov 24, 2016

Conversation

caesarxuchao
Copy link
Member

@caesarxuchao caesarxuchao commented Nov 11, 2016

Unit/integration/submit-queue e2e/slow e2e/node e2e tests passed. The PR is based on the code one or two day after the code freeze. I'll rebase after the PR gets reviewed.

Fix #35159

What are converted to use versioned API

  • top level:

    • cmd/kube-controller-manager
    • cmd/kubelet
    • plugin/cmd/kube-scheduler
    • e2e tests, node e2e test, integration tests
    • cmd/kube-dns (minor changes)
    • cmd/kubeadm (minor changes)
  • dependencies:

    • pkg/client/cache
    • pkg/serviceaccount
    • pkg/fieldpath
    • pkg/cloudprovider
    • pkg/util/[ pod | replicaset | node ]
    • pkg/volume
    • pkg/security/apparmor
    • pkg/credentialprovider
    • pkg/securitycontext
    • pkg/dns
    • plugin/pkg/auth/[ authenticator | authorizor ]/token/webhook

What remain using internal API in purpose

  • apiserver, including the SelfClient used by the authorizer
  • pkg/registry
  • plugin/pkg/admission, because Attribute.GetObject() currently gets internal API object
  • plugin/pkg/auth (except the webhook part), this package could be converted to use versioned API, but we don't have a strong incentive, and we want to reduce the ripple caused by this PR
  • kubectl: by design, kubectl talks to multiple API servers serving different versions of API, so the API-specific logic in kubectl should use the "common language", the internal version. (IMO, kubectl should use dynamic client, and offload API-specific logic to the server.)

N.B.

  • shared informers (pkg/controller/informers): shared informers are converted to use versioned API. However, plugin/pkg/admission also depends on the shared informers, and it keeps using internal API. So I have to manually add some internal version informers/listers to the shared informers. @ncdc is working on generating the shared informers, with bother internal version and external versions.
  • pkg/quota: @derekwaynecarr @vishh this package is depended by both plugin/pkg/admission and the pkg/controller, so I had to make a duplicate package, pkg/quotainternal, to be imported by the admission. I haven't thought this through: if we can remove ResourceName and ResouceList out of the API, we probably can avoid the duplicate
  • kubectl: kubectl uses several pkg/controller functions, mainly in pkg/controller/deployment/util, which are migrated to use versioned API, while kubectl still uses internal API. I have call to the conversion functions several times, to keep using the deployment controller utility functions. @janetkuo @pwittrock
  • Init containers: v1.Pod.Spec.InitContainers and v1.Pod.Status.InitContainerStatuses are not serialized. It used to rely on the conversion mechanism to encode/decode to the annotation fields. Kubelet now operates on v1.Pod and doesn't do any conversion, so I have to let kubelet explicitly write/read the annotations (see the changes in pkg/kubelet/config/apiserver.go and pkg/kubelet/status/status_manager.go) cc @smarterclayton
  • Copied some validation functions (159 lines) to pkg/v1/validation.
    TODO
  • I duplicated pkg/api/[pod, endpoints, service] in pkg/api/v1, need to check if the pkg/api ones are still needed.

cc @liggitt @deads2k @bgrant0607 @mml @mbohlool @kubernetes/sig-api-machinery


This change is Reviewable

@caesarxuchao caesarxuchao added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Nov 11, 2016
@caesarxuchao caesarxuchao added this to the 1.6 milestone Nov 11, 2016
@caesarxuchao caesarxuchao self-assigned this Nov 11, 2016
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Nov 11, 2016
@saad-ali saad-ali modified the milestones: v1.6, 1.6 Nov 12, 2016
@caesarxuchao caesarxuchao force-pushed the convert-to-versioned-API branch 2 times, most recently from 8df32c2 to 08562cb Compare November 13, 2016 23:20
@caesarxuchao caesarxuchao changed the title [WIP] Convert to use versioned api Convert to use versioned api Nov 13, 2016
// TODO: remove this duplicate
// InternalExtractContainerResourceValue extracts the value of a resource
// in an already known container
func InternalExtractContainerResourceValue(fs *api.ResourceFieldSelector, container *api.Container) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ExtractContainerResourceValue method shouldn't even exist in this package - really surprised it got cloned into here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want me to move both functions to another package?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be best.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they belong to the pkg/api/util/resources to be created in #30152. I'll leave a TODO and comment on that PR.

@caesarxuchao caesarxuchao force-pushed the convert-to-versioned-API branch 3 times, most recently from bc6e87e to 9d0c855 Compare November 15, 2016 19:17
@@ -974,6 +980,40 @@ func Convert_api_ContainerStatus_To_v1_ContainerStatus(in *api.ContainerStatus,
return autoConvert_api_ContainerStatus_To_v1_ContainerStatus(in, out, s)
}

func autoConvert_v1_ConversionError_To_api_ConversionError(in *ConversionError, out *api.ConversionError, s conversion.Scope) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't expect this type to be converted (or deep copied) since it's not for external use.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not sure how this change slip in. I guess the conversion generator generates for all types in pkg/api/v1.

@@ -149,7 +149,7 @@ var _ = framework.KubeDescribe("NodeProblemDetector", func() {
"involvedObject.name": node.Name,
"involvedObject.namespace": v1.NamespaceAll,
"source": source,
}.AsSelector()
}.AsSelector().String()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not having any place to store the compiled field selector in v1 makes this uglier than it should be. We don't need to change it here, but I hit this when moving ListOptions to meta.k8s.io/v1, and felt that it should easier / possible to deal in v1 without this. Not sure what the best approach is though.

@smarterclayton
Copy link
Contributor

Looked at all the manual commits, and they look ok (ugly to copy some of them, but not against them).

Looking through the generated code now.

extensions "k8s.io/kubernetes/pkg/client/clientset_generated/release_1_5/typed/extensions/v1beta1"
)

func versionedCliensetForDeployment(internalClient internalclientset.Interface) externalclientset.Interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@@ -659,6 +661,24 @@ func TestSimpleStop(t *testing.T) {
}
}

func getPodTemplateSpecHash(template api.PodTemplateSpec) uint32 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might just be cleaner to put this deploymentutil as GetPodTemplateSpecHashV1(...) which keeps this logic together. It's not as if we're avoiding having to import deploymentutil.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same for the other deployment util methods like this).

@@ -31,16 +32,16 @@ import (
type ListerWatcher interface {
// List should return a list type object; the Items field will be extracted, and the
// ResourceVersion field will be used to start the watch in the right place.
List(options api.ListOptions) (runtime.Object, error)
List(options v1.ListOptions) (runtime.Object, error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct. It should stay internal version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it stays internal version, then we'll need to convert the ListOptions to v1 before passing it to the client's List/Watch methods. Is using v1 wrong?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically all of the discussions boil down to:

Our internal code mode only works if the same object can be represented in all versions with no loss of data. By definition, we try to avoid losing data. When we do have that potential, we create new objects (ReplicaSet vs ReplicationController) and don't use object versioning.

So v1 is no worse than the internal version, except that right now we're introducing a change from internal to versioned that ripples out into other code before we're ready for it.

I'd rather preserve the internal api.ListOptions for now until we solve the label selector / field selector string representation in v1 (which causes other code changes) and do the conversion inside the list watcher impl. The v1 here shouldn't be core API v1 - it should be meta.k8s.io/v1 and we don't have that yet.

We can always come back to this particular change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

except that right now we're introducing a change from internal to versioned that ripples out into other code

IIUC, "ripples" refers to all the calls to selector.String()? Keeping ListWatcher interface taking api.ListOptions will only reduce very limited amount of the ripples, because most ripples are not caused by it, but rather because the versioned clientset's List/Watch methods take v1.ListOptions. Only a few ListFunc/WatchFunc definitions refer to the selectors, like this one.

So if we really want to reduce the ripples, we need to revert #31994. Otherwise there's not point keeping this interface using api.ListOptions, it will just introduce calls to conversion functions in all ListFunc/WatchFunc definitions.

return v1.ResourceList{}
}

func PodUsageFunc(obj runtime.Object) api.ResourceList {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outcome object is a lot harder to read. I'm kind of concerned that this change is just moving the complexity we have in conversion around in some other spot. It's possible these usage functions should only work on a single type each and we should register different ones for different internal types. Having two types in the same function seems prone to error and drift, and makes quota much more complex.

What other options do we have for these methods? @derekwaynecarr ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @vishh

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can create a v1,pod evaluator, that's easier to reason about, but it also duplicates much code, will be hard to maintain.

Copy link
Contributor

@smarterclayton smarterclayton Nov 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So changing admission to be versioned is out of scope for now (for lots of reasons).

I'm primarily concerned that the function is now much harder to read than it was before. Readability of this code is crucial. The maintenance of the switch is the problem. A better path may just to do:

func PodUsageFunc(obj runtime.Object) api.ResourceList {
  switch t := obj.(type) {
  case *api.Pod:
    converted := &v1.Pod{}
    if err := v1.Convert_api_Pod_to_v1_Pod(...); err != nil {
      panic("impossible conversion")
    }
    obj = converted
  }
  // rest of method, v1 adapted
  ...
}

Eating errors here is terrifying because this is a security / control code path. That would be simplest and ensures that the code is exactly the same. Admission can be slightly slower than it is today.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we need the conversion the other way around (v1 to api), because all the ResourceList logic is defined on api.ResourceList. So it's the efficiency of the resoucequota controller that gets affected. Is that acceptable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching up here, I refactored evaluator some in #34554 and so i am trying to mentally adjust some... I think if I understand Clayton's comment, in admission, we will always convert to v1, but in the controller side, there would be no conversion, but the core evaluators would then just forever be v1 based.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, after looking at the code change, I prefer Clayton's recommendation in pseudo code above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to migrate all the ResoureList utility functions to v1? If not, the conversion will be the other way around.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or the other way around (v1 -> internal). I'm fine with either, my recommendation was mostly based on the fact that admission does update less often (so internal) than the controllers recalculate usage, so avoiding some incremental effort. I'd say whatever is less code and complexity for the rest of the change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fact that admission does update less often (so internal) than the controllers recalculate usage,

Yeah, I thought of that, too. Then I thought doing the conversion for resourcequota controller is no worse than today, so I found my peace.

@@ -115,7 +116,11 @@ func NewResourceQuotaController(options *ResourceQuotaControllerOptions) *Resour
// responsible for enqueue of all resource quotas when doing a full resync (enqueueAll)
oldResourceQuota := old.(*v1.ResourceQuota)
curResourceQuota := cur.(*v1.ResourceQuota)
if quota.Equals(curResourceQuota.Spec.Hard, oldResourceQuota.Spec.Hard) {
internalOld := &api.ResourceQuota{}
v1.Convert_v1_ResourceQuota_To_api_ResourceQuota(oldResourceQuota, internalOld, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not checking errors on these is scary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can duplicate the Equals function. pkg/quota and pkg/controller/deployment/util are the two most tricky packages in this PR.

@smarterclayton
Copy link
Contributor

smarterclayton commented Nov 15, 2016

Looking through this code:

  • kubelet, kubeadm, and kube-proxy are all straight up better with this change
  • some of the controllers didn't change much
  • deployment and quota seem much scarier (partially because there is more logic that depends on them)
  • internal versions really help when reasoning about complex code (having two copies of complex code is bad)
  • kubectl changes seem like they don't make kubectl much better
  • sticking direct conversion functions (v1.Convert_X_to_Y) in lots of places (sometimes in places where we can't check errors) is incredibly worrying - that's a place where we probably don't want to be doing the conversion in the first place (or do it earlier).
  • event changes are ok - I don't really care when writing an event

Do we need kubectl as part of this change? What can we do to make deployment and quota easier to reason about?

EDIT: with a few more thoughts

@smarterclayton
Copy link
Contributor

If we get rid of internal versions, some objects are more painful to deal with unless we have "not serialized" fields (which makes the objects more complicated). Examples are init containers, label and field selectors on ListOptions. For RawExtension we solved that just by having two fields, one that is set in preference to the other. Not sure if that's something we should be doing here.

@smarterclayton
Copy link
Contributor

Yes. If you want to tie to particular internal interfaces, vendor and pay the rebase cost. We can't get to the multiple repo model without causing some internal churn.

@justinsb
Copy link
Member

We can't succeed in the multi-repo model without everyone doing their bit. I think it's reasonable to ask people to open an issue in the other repos, if you're making a change that requires you to go through the kubernetes/kubernetes codebase and fix any issues you find. It's all kubernetes, even if we shard because github.

@liggitt
Copy link
Member

liggitt commented Jan 13, 2017

if you're making a change that requires you to go through the kubernetes/kubernetes codebase and fix any issues you find

isn't that every single PR?

@justinsb
Copy link
Member

isn't that every single PR?

We're being very literal today ;-) I guess the point is if you have to go through the repo and looks for usages that have been silently broken (i.e. the compiler isn't making them obvious), and you find them, that "must" be notified to sharded repos. I think if the compiler flags them and it's an easy fix we can let it slide. But if it's a non-trivial change (either in complexity or volume), it's good manners to compile some notes for the external repos, so that they can make the required changes in a time-efficient manner, and posting an issue seems like the most efficient way to communicate that.

And yes, bring on the shared repos with tighter API guarantees so this ceases to be a problem.

@liggitt
Copy link
Member

liggitt commented Jan 13, 2017

A common scenario is that we find a class of bug in-tree, and sweep for and fix it. A PSA email to kubernetes-dev would be a good idea in cases like that, but I don't think spawning issues to all sub-repos is scalable for every bug that requires a sweep

@justinsb
Copy link
Member

Yes, exactly what we're discussing here. So a vote for option 3 :-)

@bgrant0607
Copy link
Member

@justinsb The internal API has no stability guarantee, has been changed several times, and is "internal", so is not breaking. It really should only be used by apiserver, which is the state we're trying to get to. It was never supposed to be serialized, since the beginning. This can be traced back into 2014 from #3933. The tags were copied into api/types.go originally in order to make that file easier to diff with the latest version. It may not have been sufficiently documented, but we've tried to remove these tags multiple times.

As for the broader issue, tools for searching multiple repos would be a good first step. Are you aware of any?

@bgrant0607
Copy link
Member

https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md does document that the versioned types should be used exclusively, and that breaking changes should be emailed to kubernetes-dev. That list has far lower traffic than github notifications.

@@ -2313,8 +2313,9 @@ __EOF__
kubectl-with-retry rollout resume deployment nginx "${kube_flags[@]}"
# The resumed deployment can now be rolled back
kubectl rollout undo deployment nginx "${kube_flags[@]}"
# Check that the new replica set (nginx-618515232) has all old revisions stored in an annotation
kubectl get rs nginx-618515232 -o yaml | grep "deployment.kubernetes.io/revision-history: 1,3"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the change to use versioned api going to change hashes in all pre-existing deployments? There was value in this test in that we ensured that the hash wouldn't change between updates. As a matter of fact we need such a test and I created one in #40854.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the change to use versioned api going to change hashes in all pre-existing deployments?

Yes. I discussed with @janetkuo back then, IIRC, deployment controller shouldn't depend on the stable hash, because any changes to the API will alter the hash, like when we added ObjectMeta.OwnerReferences to the API.

@0xmichalis
Copy link
Contributor

0xmichalis commented Feb 4, 2017 via email

@liggitt
Copy link
Member

liggitt commented Feb 4, 2017

Changes to the API (specifically the PodTemplateSpec) should be
backwards-compatible, usually by introducing the new field as a pointer.
Then, new API additions shouldn't result in new hashes.

That's not what a pointer field gets you. That makes to detect when it was previously unspecific (nil instead of trying to detect zero-values of structs) so you can default to appropriately. Defaulting a new, nil, pointer field can still result in a hash change

@0xmichalis
Copy link
Contributor

0xmichalis commented Feb 4, 2017 via email

@liggitt
Copy link
Member

liggitt commented Feb 4, 2017

DeepEqual is orthogonal to defaulting

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 4, 2017 via email

@0xmichalis
Copy link
Contributor

0xmichalis commented Feb 4, 2017 via email

@0xmichalis
Copy link
Contributor

DeepEqual is orthogonal to defaulting

Sigh, nevermind. Bad day. How expensive does converting to YAML before hashing sound? It would eliminate hash updates between upgrades.

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 5, 2017 via email

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 5, 2017 via email

@0xmichalis
Copy link
Contributor

0xmichalis commented Feb 6, 2017 via email

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 6, 2017 via email

@gmarek
Copy link
Contributor

gmarek commented May 16, 2017

@caesarxuchao @davidopp - is there a reason why "InodePressure" constant is defined only in api/v1/types.go, and not in api/types.go?

@caesarxuchao
Copy link
Member Author

In general the constants should be duplicated in both internal version and external version.

InodePressure might be different, i vaguely remembered discussed it with @dashpole, and the conclusion was that field was deprecated?

@dashpole
Copy link
Contributor

InodePressure should be removed. It was initially added and removed within the same release cycle. I wasnt aware that it was added to api/v1/types.go...

@gmarek
Copy link
Contributor

gmarek commented Jun 13, 2017

@dashpole - I'm not sure we can do that, even if it's unused. It would be a breaking API change. @kubernetes/api-reviewers

@smarterclayton
Copy link
Contributor

You mean for client-go consumers?

@gmarek
Copy link
Contributor

gmarek commented Jun 19, 2017

Yup.

dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet