Apiserver flowcontrol api models #83671

yue9944882 · 2019-10-09T10:10:35Z

NONE

/sig api-machinery
/kind feature

yue9944882 · 2019-10-09T15:17:45Z

/hold cancel

staging/src/k8s.io/api/flowcontrol/v1alpha1/helpers.go

staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go

deads2k · 2019-10-10T13:08:18Z

staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go

+	Subjects []Subject `json:"subjects,omitempty" protobuf:"bytes,1,opt,name=subjects"`
+	// `rule` is the target verb, resource or the subresource the rule cares about. APIGroups, Resources, etc.
+	// Required.
+	Rule PolicyRule `json:"rule" protobuf:"bytes,2,opt,name=rule"`


in RBAC we chose to use a slice of these because you want to make sure the group and resource tuple expansions are correct. You don't want to match deployments.rbac.authorization.k8s.io for instance.

We are keeping group and resource together, they are both in PolicyRule (copied from rbac).
What is different in this API is how rules are associated with subjects. In this API it is this struct (PolicyRuleWithSubjects) that does that binding. Where rbac has a slice of PolicyRule, this API has a slice of PolicyRuleWithSubjects.

Also, the field tag here is inconsistent between json and protobuf regarding optionality.

That structure will require that subjects be defined multiple times to be able to grant access to resources in different groups. That is overly onerous.

staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go

deads2k · 2019-10-10T13:09:32Z

staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go

+	// Rules can either apply to API resources (such as "pods" or "secrets") or non-resource URL paths (such as "/api"),  but not both.
+	// NonResourceAll represents all non-resource urls.
+	// +optional
+	NonResourceURLs []string `json:"nonResourceURLs,omitempty" protobuf:"bytes,6,rep,name=nonResourceURLs"`


I think this indicates we should have a union type for representing the rule. Back when I first wrote these, we hadn't formalized best practices.

And if we do not make that change then my comment on Resources applies here too.

The comment asserts more flexible patterns than the code implements.
The field tag does not say this is optional in protobuf.

staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go

deads2k · 2019-10-10T13:19:20Z

staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go

+	// `queues` is a number of queues that belong to a non-exempt PriorityLevelConfiguration object. The queues exist
+	// independently at each apiserver. The value must be positive for a non-exempt priority level and setting it to 1
+	// disables shufflesharding and makes the distinguisher method irrelevant.
+	// TODO: sugguest a default or a way of deciding on a value.


You must describe what happens if this is left at zero. What is a "good" value? People don't understand what this means, but my memory is that choosing 1 defeats the purpose of doing this.

@deads2k : You raise multiple points.

"what happens if this is left at zero". I do not understand why you raise this question. The comment says "The value must be positive for a non-exempt priority level". Isn't it normally implied that when a constraint is stated in a comment, it is checked in validation?

"What is a "good" value?" Do you mean what is a good default value (still thinking about the possiblity that this field can be omitted)? Or are you saying this comment should give the user guidance on how to choose a value for this field? If the former then this is moot since the field is not optional. If the latter then this makes some sense to me, but the question of guidance is not unique to this field and not even something that neatly divides among fields. The guidance we can give is the design discussion we have been having, and it is not small enough and final enough to put in an API file. I think we need to give guidance in the user-facing documentation and rely on people to look there. I think we have to settle for making the API define precisely what each field means.

"People don't understand what this means, but my memory is that choosing 1 defeats the purpose of doing this". No, a value of 1 does not defeat the purpose of queuing or having a priority level, it merely says that everything of this priority level goes in the same queue. The comment already makes related points: "setting it to 1 disables shufflesharding and makes the distinguisher method irrelevant", relying on the reader to figure out that a value of 1 means one queue for the priority level. I do not understand what is difficult about that. Would the following rewording help?

Setting this field to 1 means that all requests of this priority level go in the same queue,
and this means there is no shuffle-sharding and the flow distinguisher method is moot.

It's optional in your spec here, which means zero is allowed and you have to decide how to interpret it.

I don't think that a queue size of one by default is a good value.

deads2k · 2019-10-10T13:19:55Z

staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go

+	Queues int32 `json:"queues,omitempty" protobuf:"varint,2,opt,name=queues"`
+	// `handSize` is a small positive number for applying shuffle sharding. When a request arrives at an apiserver the
+	// request flow identifier’s string pair is hashed and the hash value is used to shuffle the queue indices and deal
+	// a hand of the size specified here. If empty, the hand size will the be set to 1.


With hand size one, don't I defeat the purpose of the shuffle? It seems like you'd want a percentage of the queues.

Yes, a hand size of one means there is no shuffle sharding. I think it is a poor default value. In fact, I think there is no good default value. Have a look at #76846 (comment) and propose a specific default value or formula for computing one, if you think there is a good way to compute a default value.

MikeSpreitzer

Some comments in-line.

staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go

MikeSpreitzer · 2019-10-25T17:29:44Z

staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go

+	// +listType=associative
+	// +listMapKey=type
+	// +optional
+	Conditions []FlowSchemaCondition `json:"conditions,omitempty" protobuf:"bytes,1,rep,name=conditions"`


Shouldn't there be list metadata in the field tag too?

can you elaborate what you mean by "list metadata"?

If you look at Pods, for example, you see stuff like this:

type PodSpec struct { // List of volumes that can be mounted by containers belonging to the pod. // More info: https://kubernetes.io/docs/concepts/storage/volumes // +optional // +patchMergeKey=name // +patchStrategy=merge,retainKeys Volumes []Volume `json:"volumes,omitempty" patchStrategy:"merge,retainKeys" patchMergeKey:"name" protobuf:"bytes,1,rep,name=volumes"` ...

(1) the comments use a different convention than we use, I do not understand what's up with that.

(2) the field tags include some of the same information as the comments about how the list is to be interpreted.

See also https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/0006-apply.md#api-topology

MikeSpreitzer · 2019-10-25T17:33:36Z

pkg/apis/flowcontrol/validation/validation.go

+		for _, msg := range apimachineryvalidation.ValidateNamespaceName(subject.Namespace, false) {
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("namespace"), subject.Namespace, msg))
+		}
+	} else {


It could be more uniform: test len(...) == 0 for both fields, or test len(...) > 0 for both fields. Also, I see the error produced in the zero length case is a little different.

MikeSpreitzer · 2019-10-25T17:35:32Z

pkg/apis/flowcontrol/validation/validation.go

+}
+
+// ValidateFlowSchemaNonResourcePolicyRule validates non-resource policy-rule in the flow-schema.
+func ValidateFlowSchemaNonResourcePolicyRule(rule *flowcontrol.NonResourcePolicyRule, fldPath *field.Path) field.ErrorList {


Are there difficult issues in enforcing the comments?

We can leave some non-interface-changing work for later PRs, but I think we will not be done until the comments match the validation logic and the handling by the implementation.

pkg/apis/flowcontrol/validation/validation.go

MikeSpreitzer · 2019-10-28T18:10:44Z

pkg/apis/flowcontrol/validation/validation.go

+			allErrs = append(allErrs, field.NotSupported(fldPath.Child("distinguisherMethod").Child("type"), spec.DistinguisherMethod, supportedDistinguisherMethods.List()))
+		}
+	}
+	if len(spec.PriorityLevelConfiguration.Name) > 0 {


Don't we have to check for spec.PriorityLevelConfiguration != nil before dereferencing it?

the field PriorityLevelConfiguration is a composed struct instead of a pointer. there wont be nil panic here.

versioned external external external external

rule list rule rule 2

internal internal internal internal internal

MikeSpreitzer · 2019-10-29T04:57:49Z

pkg/apis/flowcontrol/validation/validation.go

+	string(flowcontrol.FlowDistinguisherMethodByUserType),
+)
+
+var priorityLevelConfigurationQueuingMaxQueues int32 = 10 * 1000 * 1000 // 10^7


I suggest a comment along the lines of

// priorityLevelConfigurationQueuingMaxQueues is a limit that we impose
// so that shufflesharding.RequiredEntropyBits does not suffer overflow.
// The value here is the largest power of 10 that does this.

validation test validation test validation adding validation for fs/pl status validation validation validation replace < with <= validation test

generated generated generated rule generated generated

MikeSpreitzer

I think this is good enough to merge and unblock dependent work.
There are a few things remaining to clean up.

list metadata in field tags in the external types
English language nits in the comments
Full validation of non-resource URL patterns

/LGTM

lavalamp · 2019-11-01T18:31:31Z

Spoke offline, both @deads2k and I agree the best way forward is to merge this and take some followups.

/approve

k8s-ci-robot · 2019-11-01T18:32:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, yue9944882

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:

~~api/OWNERS~~ [lavalamp]
~~build/OWNERS~~ [lavalamp]
~~hack/OWNERS~~ [lavalamp]
~~pkg/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/api/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apiserver/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/client-go/OWNERS~~ [lavalamp]
~~test/integration/apiserver/OWNERS~~ [lavalamp]
~~vendor/OWNERS~~ [lavalamp]

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

fejta-bot · 2019-11-02T00:07:57Z

/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.

yue9944882 · 2019-11-02T03:46:51Z

/retest

k8s-ci-robot requested review from deads2k and fejta October 9, 2019 10:11

k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 9, 2019

yue9944882 force-pushed the flow-control-api-model branch from 77244cf to 679ec70 Compare October 9, 2019 15:17

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2019