start serving customresourcedefinition based on status #45838

deads2k · 2017-05-15T20:31:46Z

This exposes the customresourcedefinition/status endpoint, wires a controller to drive NameConflict conditions, and serves discovery from status, not spec.

Next steps after this include wiring the conditions into handling and reswizzling the handling chain to be cleaner now that we have a custom mux.

deads2k · 2017-05-15T20:32:22Z

The first commit is just the renaming commit, so this isn't as enormous as it seems.

k8s-reviewable · 2017-05-15T21:38:07Z

This change is

enisoc · 2017-05-15T22:33:07Z

Reviewed 66 of 66 files at r1, 12 of 12 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/helpers.go, line 58 at r2 (raw file):

func IsCustomResourceDefinitionCondition(customResourceDefinition *CustomResourceDefinition, conditionType CustomResourceDefinitionConditionType, status ConditionStatus) bool {
	for _, condition := range customResourceDefinition.Status.Conditions {
		if condition.Type == conditionType && condition.Status == status {

Could you short-circuit here? I'm assuming there can only be one condition of a given type?

if condition.Type == conditionType {
  return condition.Status == status
}

staging/src/k8s.io/kube-apiextensions-server/pkg/apiserver/customresource_discovery_controller.go, line 85 at r2 (raw file):

	foundGroup := false
	for _, customResourceDefinition := range customResourceDefinitions {
		// TODO add status checking

Is there enough plumbing now to add something here? If there is a name conflict, we don't want to list the resource in discovery, right?

staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go, line 84 at r2 (raw file):

	allKinds = sets.String{}

	list, err := c.customResourceDefinitionLister.List(labels.Everything())

Informers are not write-through, right? Even though this is single-worker, our cache may not yet reflect updates we made in a previous sync(), so we could hand out names twice.

staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go, line 228 at r2 (raw file):

	}

	_, err = c.customResourceDefinitionClient.CustomResourceDefinitions().UpdateStatus(customResourceDefinition)

It doesn't matter yet with only one worker, but I would expect the UpdateStatus step to come before "enqueue others who may care about the new status". Is there any reason not to use that order now so it doesn't bite us later?

Comments from Reviewable

deads2k · 2017-05-15T23:33:10Z

Informers are not write-through, right? Even though this is single-worker, our cache may not yet reflect updates we made in a previous sync(), so we could hand out names twice.

Good catch. I'll upstream origin's mutation filter.

Is there enough plumbing now to add something here? If there is a name conflict, we don't want to list the resource in discovery, right?

Yes. I was going to overhaul it separately, but I'll see how invasive the things I want to do are.

It doesn't matter yet with only one worker, but I would expect the UpdateStatus step to come before "enqueue others who may care about the new status". Is there any reason not to use that order now so it doesn't bite us later?

Sure. I don't know that I see us multithreading this in the near term though.

sttts · 2017-05-16T06:17:30Z

staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/helpers.go

+
+// SetCustomResourceDefinitionCondition sets the status condition.  It either overwrites the existing one or
+// creates a new one
+func SetCustomResourceDefinitionCondition(customResourceDefinition *CustomResourceDefinition, newCondition CustomResourceDefinitionCondition) {


can we at least use short parameter names if types and func names are that long?

sttts · 2017-05-16T06:22:12Z

staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go

+// This controller is reserving names. To avoid conflicts, be sure to run only one instance of the worker at a time.
+// This could eventually be lifted, but starting simple.
+type NamingConditionController struct {
+	customResourceDefinitionClient client.CustomResourceDefinitionsGetter


sttts · 2017-05-16T06:23:45Z

staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go

+	return c
+}
+
+func (c *NamingConditionController) getNamesForGroup(group string) (allResources sets.String, allKinds sets.String) {


getAcceptedNamesForGroup

We are using names for two things in this file: for the Names type and for a set of strings.

sttts · 2017-05-16T06:28:54Z

staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go

+	return newNames, condition
+}
+
+func checkName(requestedName, acceptedName string, usedNames sets.String) error {


s/checkName/equalToAcceptedOrFresh/

sttts · 2017-05-16T06:32:41Z

staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go

+	} else {
+		newNames.Singular = requestedNames.Singular
+	}
+	if !reflect.DeepEqual(requestedNames.ShortNames, acceptedNames.ShortNames) {


~~where is the sorting? If there is one, please add a comment there.~~

sttts · 2017-05-16T06:39:01Z

staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go

+
+	// Check each name for mismatches.  If there's a mismatch between spec and status, then try to deconflict.
+	// Continue on errors so that the status is the best match possible
+	if err := checkName(requestedNames.Plural, acceptedNames.Plural, allResources); err != nil {


Where is it checked that singular, plural and shortnames do not overlap? I haven't seen this in the validation either.

Where is it checked that singular, plural and shortnames do not overlap?

Inside of a single resource, they should be able to since the parsing would still be unambiguous for intent. Think endpoints and endpoints. They point to the same spot.

sttts · 2017-05-16T06:44:17Z

staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go

+		condition.Message = "no conflicts found"
+	}
+
+	return newNames, condition


Would the semantics be much simpler if we either accept all requested names or none? Now it can happen that the plural changes, but the singular has a conflicts. Don't like that very much.

Would the semantics be much simpler if we either accept all requested names or none? Now it can happen that the plural changes, but the singular has a conflicts. Don't like that very much.

Instead of reserving the names that are valid? The change ought to be straightforward, but I think we actually serve with some names conflicting.

Instead of reserving the names that are valid? The change ought to be straightforward, but I think we actually serve with some names conflicting.

I guess I wouldn't want to explain this to anyone. You sure we don't want to reserve the names that were ok?

What if you notice that there is a conflict, you do kubectl-edit and some of the names are already accepted. There is not way to unaccept them, right? I would like to be able to edit all the names as long as the CRD is not ready. If I can do that even after parts were accepted, that's fine.

What if you notice that there is a conflict, you do kubectl-edit and some of the names are already accepted. There is not way to unaccept them, right? I would like to be able to edit all the names as long as the CRD is not ready. If I can do that even after parts were accepted, that's fine.

that is possible. There's even a test for it: "merge on conflict"

Ok, then it's fine. I had the impression it's not possible. But I trust the test ;)

sttts · 2017-05-16T06:45:44Z

staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go

+}
+
+func (c *NamingConditionController) sync(key string) error {
+	inCustomResourceDefinition, err := c.customResourceDefinitionLister.Get(key)


sttts · 2017-05-16T06:52:07Z

staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/helpers.go

+}
+
+// IsCustomResourceDefinitionEquivalent returns true if the lhs and rhs are equivalent except for times
+func IsCustomResourceDefinitionEquivalent(lhs, rhs *CustomResourceDefinitionCondition) bool {


s/IsCustomResourceDefinitionEquivalent/equivalentConditions/

s/IsCustomResourceDefinitionEquivalent/equivalentConditions/

No guarantee that this group doesn't grow more types with other conditions.

equivalentCRDConditions. The main point is that "Conditions" is missing in the name now.

sttts · 2017-05-16T06:53:12Z

staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/helpers.go

+}
+
+// IsCustomResourceDefinitionCondition indicates if the condition is present and equal to the arg
+func IsCustomResourceDefinitionCondition(customResourceDefinition *CustomResourceDefinition, conditionType CustomResourceDefinitionConditionType, status ConditionStatus) bool {


conditionPresentAndEqual

sttts · 2017-05-16T06:53:39Z

staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/helpers.go

+}
+
+// IsCustomResourceDefinitionConditionTrue indicates if the condition is present and strictly true
+func IsCustomResourceDefinitionConditionTrue(customResourceDefinition *CustomResourceDefinition, conditionType CustomResourceDefinitionConditionType) bool {


conditionTrue

sttts · 2017-05-16T06:54:05Z

staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/helpers.go

+}
+
+// GetCustomResourceDefinitionCondition returns the condition you're looking for or nil
+func GetCustomResourceDefinitionCondition(customResourceDefinition *CustomResourceDefinition, conditionType CustomResourceDefinitionConditionType) *CustomResourceDefinitionCondition {


findCondition

sttts · 2017-05-16T06:54:21Z

staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/helpers.go

+
+// SetCustomResourceDefinitionCondition sets the status condition.  It either overwrites the existing one or
+// creates a new one
+func SetCustomResourceDefinitionCondition(customResourceDefinition *CustomResourceDefinition, newCondition CustomResourceDefinitionCondition) {


setCondition

sttts · 2017-05-16T06:57:22Z

staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go

+		return err
+	}
+
+	acceptedNames, namingCondition := c.calculateNames(inCustomResourceDefinition)


// nothing to do if accepted names and NameConflict condition didn't change

sttts · 2017-05-16T06:59:27Z

staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go

+	// if we haven't changed the condition, then our names must be good.
+	if condition.Status == apiextensions.ConditionUnknown {
+		condition.Status = apiextensions.ConditionFalse
+		condition.Reason = "Passed"


s/Passed/NoConflicts/

sttts · 2017-05-16T07:02:05Z

staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go

+		}
+	}
+
+	_, err = c.customResourceDefinitionClient.CustomResourceDefinitions().UpdateStatus(customResourceDefinition)


Do we know that the customResourceDefinitionLister is up-to-date immediately? I.e. do we see a consistent group state for the next item in the queue?

Do we know that the customResourceDefinitionLister is up-to-date immediately? I.e. do we see a consistent group state for the next item in the queue?

pulling in a mutation cache filter from openshift.

deads2k · 2017-05-16T12:57:58Z

Rebased, added a mutation cache, addressed comments in a separate commit.

deads2k · 2017-05-16T15:45:11Z

@k8s-bot kops aws e2e test this
@k8s-bot gce etcd3 e2e test this

deads2k · 2017-05-16T17:07:05Z

changed to IsCRDConditionEquivalent

…

On Tue, May 16, 2017 at 12:32 PM, Dr. Stefan Schimanski < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In staging/src/k8s.io/kube-apiextensions-server/pkg/apis/ apiextensions/helpers.go <#45838 (comment)> : > +func IsCustomResourceDefinitionConditionTrue(customResourceDefinition *CustomResourceDefinition, conditionType CustomResourceDefinitionConditionType) bool { + return IsCustomResourceDefinitionCondition(customResourceDefinition, conditionType, ConditionTrue) +} + +// IsCustomResourceDefinitionCondition indicates if the condition is present and equal to the arg +func IsCustomResourceDefinitionCondition(customResourceDefinition *CustomResourceDefinition, conditionType CustomResourceDefinitionConditionType, status ConditionStatus) bool { + for _, condition := range customResourceDefinition.Status.Conditions { + if condition.Type == conditionType && condition.Status == status { + return true + } + } + return false +} + +// IsCustomResourceDefinitionEquivalent returns true if the lhs and rhs are equivalent except for times +func IsCustomResourceDefinitionEquivalent(lhs, rhs *CustomResourceDefinitionCondition) bool { equivalentCRDConditions. The main point is that "Conditions" is missing in the name now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45838 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH2BSvyGkUXToFUrjMa3474XY1AcqoG_ks5r6c-1gaJpZM4Nbp3M> .

enisoc · 2017-05-16T18:59:59Z

LGTM overall but it's unfortunate that the mutation cache only mostly prevents name conflicts. What's the worst case scenario if we hand out a name twice? Perhaps the apiextensions-server crashloops?

Is it infeasible to switch to an uncached listing of all CRDs upon each sync, until and unless it is shown to be a performance problem? Or, is there a way to do something like WaitForCache on each sync to make sure it's caught up to see any updates from the previous sync?

If there's no easy win, I'm ok with a TODO to revisit this if conflicts are found to occur in the wild.

deads2k · 2017-05-16T19:19:15Z

Is it infeasible to switch to an uncached listing of all CRDs upon each sync, until and unless it is shown to be a performance problem? Or, is there a way to do something like WaitForCache on each sync to make sure it's caught up to see any updates from the previous sync?

If there's no easy win, I'm ok with a TODO to revisit this if conflicts are found to occur in the wild.

It's an n^2 lookup without any server-side filtering (though the server side filtering could be done with more work). I'd rather bump the size of the LRU cache before I tried to go out to the server for every check. I suspect we're o(ones) to start, and then stabilize o(dozens) or maybe o(hundreds). Getting to o(thousands) and having a controller so fast it doesn't observe the update before its dropped from the cache seems unlikely.

The worst case isn't a crash loop, it's serving inconsistent information (same as can already happen today).

enisoc · 2017-05-16T19:24:26Z

/lgtm

k8s-github-robot · 2017-05-16T19:25:19Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [deads2k]
~~staging/src/k8s.io/kube-apiextensions-server/OWNERS~~ [deads2k]
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

deads2k · 2017-05-16T20:09:37Z

squashed, retagging.

deads2k · 2017-05-17T00:00:02Z

@k8s-bot kops aws e2e test this

k8s-github-robot · 2017-05-17T04:27:57Z

Automatic merge from submit-queue (batch tested with PRs 44520, 45253, 45838, 44685, 45901)

@ncdc

Automatic merge from submit-queue Move the remaining controllers to shared informers Completes work done in 1.6 to move the last two hold outs to shared informers - tokens controller and scheduler. Adds a few more tools to allow informer reuse (like filtering the informer, or maintaining a mutation cache). The mutation cache is identical to #45838 and will be removed when that merges @ncdc @deads2k extracted from openshift/origin#14086

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 15, 2017

k8s-github-robot assigned enisoc and sttts May 15, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 15, 2017

sttts reviewed May 16, 2017

View reviewed changes

deads2k force-pushed the tpr-15-status branch from 32cbbc2 to 9a35b25 Compare May 16, 2017 12:57

deads2k mentioned this pull request May 16, 2017

Use shared informers for secrets, services, and service accounts openshift/origin#14086

Merged

deads2k force-pushed the tpr-15-status branch from 9a35b25 to 06b681b Compare May 16, 2017 12:59

deads2k force-pushed the tpr-15-status branch 2 times, most recently from 3cb6eee to 40eff82 Compare May 16, 2017 14:33

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2017

add mutation cache filter

f88c772

deads2k force-pushed the tpr-15-status branch from 40eff82 to a54eb03 Compare May 16, 2017 14:41

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2017

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 16, 2017

start using customresourcedefinition.status

cb604f7

deads2k force-pushed the tpr-15-status branch from a54eb03 to cb604f7 Compare May 16, 2017 20:09

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2017

smarterclayton mentioned this pull request May 17, 2017

Move the remaining controllers to shared informers #45933

Merged

k8s-github-robot merged commit ae045a7 into kubernetes:master May 17, 2017

deads2k deleted the tpr-15-status branch August 3, 2017 20:09

start serving customresourcedefinition based on status #45838

start serving customresourcedefinition based on status #45838

Conversation

deads2k commented May 15, 2017

deads2k commented May 15, 2017

k8s-reviewable commented May 15, 2017

enisoc commented May 15, 2017

deads2k commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts May 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts May 16, 2017 • edited

Choose a reason for hiding this comment

sttts May 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts May 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented May 16, 2017

deads2k commented May 16, 2017

deads2k commented May 16, 2017 via email

enisoc commented May 16, 2017 • edited

deads2k commented May 16, 2017

enisoc commented May 16, 2017

k8s-github-robot commented May 16, 2017

deads2k commented May 16, 2017

deads2k commented May 17, 2017

k8s-github-robot commented May 17, 2017

sttts May 16, 2017 •

edited

sttts May 16, 2017 •

edited

sttts May 16, 2017 •

edited

sttts May 16, 2017 •

edited

enisoc commented May 16, 2017 •

edited