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

feat: add querying functions to event expansion. #83065

Open

Conversation

@jfbai
Copy link
Contributor

jfbai commented Sep 24, 2019

What type of PR is this?

/kind feature

What this PR does / why we need it:

Fix TODO: implement query functions to event expansion, which is similar to coreV1/event_expansion.

@yastij Is there any other requirements to the querying function?

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?:

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

/assign @yastij
/sig api-machinery

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Sep 24, 2019

Hi @jfbai. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jfbai jfbai force-pushed the jfbai:fix-todo-add-querying-functions-to-event-expansion branch from 7465a93 to 1c7c937 Sep 24, 2019
@k8s-ci-robot k8s-ci-robot requested review from caesarxuchao and liggitt Sep 24, 2019
// Returns the appropriate field selector based on the API version being used to communicate with the server.
// The returned field selector can be used with List and Watch to filter desired events.
func (e *events) GetFieldSelector(regardingName, regardingNamespace, regardingKind, regardingUID *string) fields.Selector {
apiVersion := e.client.APIVersion().String()

This comment has been minimized.

Copy link
@liggitt

liggitt Sep 24, 2019

Member

this is strange... this is a v1beta1 client... why do we need to interrogate the version string?


// Returns the appropriate field selector based on the API version being used to communicate with the server.
// The returned field selector can be used with List and Watch to filter desired events.
func (e *events) GetFieldSelector(regardingName, regardingNamespace, regardingKind, regardingUID *string) fields.Selector {

This comment has been minimized.

Copy link
@liggitt

liggitt Sep 24, 2019

Member

I'm having a hard time understanding why this needs to be a method on the event client, and why it can't be a v1beta1 event helper

This comment has been minimized.

Copy link
@liggitt

liggitt Sep 24, 2019

Member

I'd also recommend not exposing this as public API until needed

This comment has been minimized.

Copy link
@yastij
apiVersion := e.client.APIVersion().String()
field := fields.Set{}
if regardingName != nil {
field[GetRegardingNameFieldLabel(apiVersion)] = *regardingName

This comment has been minimized.

Copy link
@liggitt

liggitt Sep 24, 2019

Member

why is this calling to a function for the field path and not inlined?

This comment has been minimized.

Copy link
@yastij

yastij Sep 24, 2019

Member

+1 should be inlined

// Search finds events about the specified object. The namespace of the
// object must match this event's client namespace unless the event client
// was made with the "" namespace.
func (e *events) Search(scheme *runtime.Scheme, objOrRef runtime.Object) (*v1beta1.EventList, error) {

This comment has been minimized.

Copy link
@liggitt

liggitt Sep 24, 2019

Member

rather than plumbing scheme and object through this method, a helper function that takes an ObjectReference and an event client seems like it would be simpler:

func SearchEvents(client EventsV1beta1Interface, ref *v1.ObjectReference) (*v1beta1.EventList, error)

callers could get an object reference however they wanted (calling ref.GetReference if desired, or via some other method)

This comment has been minimized.

Copy link
@yastij

yastij Sep 24, 2019

Member

That could work too

This comment has been minimized.

Copy link
@jfbai

jfbai Sep 25, 2019

Author Contributor

@yastij @liggitt May I ask in which directory it would be better to add these helper functions? Thanks a lot.

This comment has been minimized.

Copy link
@liggitt

liggitt Oct 4, 2019

Member

alongside the client-go events tools, maybe? k8s.io/client-go/tools/events?

This comment has been minimized.

Copy link
@yastij

yastij Oct 4, 2019

Member

sounds reasonable

This comment has been minimized.

Copy link
@jfbai

jfbai Oct 6, 2019

Author Contributor

Thanks a lot. :)

// Search finds events about the specified object. The namespace of the
// object must match this event's client namespace unless the event client
// was made with the "" namespace.
func (e *events) Search(scheme *runtime.Scheme, objOrRef runtime.Object) (*v1beta1.EventList, error) {

This comment has been minimized.

Copy link
@yastij

yastij Sep 24, 2019

Member

That could work too


// Returns the appropriate field selector based on the API version being used to communicate with the server.
// The returned field selector can be used with List and Watch to filter desired events.
func (e *events) GetFieldSelector(regardingName, regardingNamespace, regardingKind, regardingUID *string) fields.Selector {

This comment has been minimized.

Copy link
@yastij
}
stringRefKind := string(ref.Kind)
var refKind *string
if stringRefKind != "" {

This comment has been minimized.

Copy link
@yastij

yastij Sep 24, 2019

Member

len(stringRefKind) > 0

}
stringRefUID := string(ref.UID)
var refUID *string
if stringRefUID != "" {

This comment has been minimized.

Copy link
@yastij
apiVersion := e.client.APIVersion().String()
field := fields.Set{}
if regardingName != nil {
field[GetRegardingNameFieldLabel(apiVersion)] = *regardingName

This comment has been minimized.

Copy link
@yastij

yastij Sep 24, 2019

Member

+1 should be inlined

@jfbai jfbai force-pushed the jfbai:fix-todo-add-querying-functions-to-event-expansion branch from 1c7c937 to bea8877 Oct 8, 2019
@jfbai

This comment has been minimized.

Copy link
Contributor Author

jfbai commented Oct 8, 2019

@liggitt @yastij Your comments have been addressed, PTAL, thanks a lot. :)

@jfbai jfbai force-pushed the jfbai:fix-todo-add-querying-functions-to-event-expansion branch from bea8877 to dc12e63 Oct 14, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Oct 14, 2019
@jfbai

This comment has been minimized.

Copy link
Contributor Author

jfbai commented Oct 14, 2019

@liggitt your comments have been fixed, PTAL, thanks a lot. :)

@jfbai jfbai requested a review from liggitt Oct 14, 2019
func GetFieldSelector(groupVersion schema.GroupVersion, regardingName, regardingNamespace, regardingKind, regardingUID string) fields.Selector {
field := fields.Set{}

if groupVersion.Group == v1beta1.GroupName {

This comment has been minimized.

Copy link
@liggitt

liggitt Oct 14, 2019

Member

a switch statement might be simpler to follow (as well as returning errors if an unknown version is requested):

switch groupVersion {
  case v1beta1.SchemeGroupVersion:
    ...
  case v1.SchemeGroupVersion:
    ...
  default:
    return field, fmt.Error("unknown version %v", groupVersion)
}

This comment has been minimized.

Copy link
@jfbai

jfbai Oct 14, 2019

Author Contributor

@liggitt thanks a lot, fixed. :)

@jfbai jfbai force-pushed the jfbai:fix-todo-add-querying-functions-to-event-expansion branch from dc12e63 to 08a3a47 Oct 14, 2019
@jfbai jfbai requested a review from liggitt Oct 14, 2019
@jfbai jfbai force-pushed the jfbai:fix-todo-add-querying-functions-to-event-expansion branch from 08a3a47 to ae5c7ef Oct 15, 2019
@k8s-ci-robot k8s-ci-robot added size/M and removed size/L labels Oct 15, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 15, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jfbai
To complete the pull request process, please assign lavalamp
You can assign the PR to them by writing /assign @lavalamp in a comment when ready.

The full list of commands accepted by this bot can be found 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

@jfbai jfbai force-pushed the jfbai:fix-todo-add-querying-functions-to-event-expansion branch from ae5c7ef to 97002b3 Oct 15, 2019
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Oct 15, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@jfbai jfbai requested a review from liggitt Oct 15, 2019
@jfbai

This comment has been minimized.

Copy link
Contributor Author

jfbai commented Oct 24, 2019

@liggitt PTAL, thanks a lot. :)

Copy link
Member

liggitt left a comment

thanks, an integration test would be good to prove this works, and would like @wojtek-t to take a look

func AddFieldLabelConversionsForEvent(scheme *runtime.Scheme) error {
return scheme.AddFieldLabelConversionFunc(SchemeGroupVersion.WithKind("Event"),
func(label, value string) (string, string, error) {
switch label {

This comment has been minimized.

Copy link
@liggitt

liggitt Nov 6, 2019

Member

@wojtek-t, PTAL to make sure these make sense

This comment has been minimized.

Copy link
@wojtek-t

// GetFieldSelector returns the appropriate field selector based on the API version being used to communicate with the server.
// The returned field selector can be used with List and Watch to filter desired events.
func GetFieldSelector(groupVersion schema.GroupVersion, regardingName, regardingNamespace, regardingKind, regardingUID string) (fields.Selector, error) {

This comment has been minimized.

Copy link
@liggitt

liggitt Nov 6, 2019

Member

can you add a test to test/integration/events that creates an event for an object, and queries each of these attributes via the events.k8s.io/v1beta1 and v1 APIs, then each of these attributes with a non-matching value, to make sure these are wired up properly?

This comment has been minimized.

Copy link
@jfbai

jfbai Nov 6, 2019

Author Contributor

Sure.

Copy link
Member

yastij left a comment

/cc @wojtek-t

return "involvedObject.resourceVersion", value, nil
case "regarding.fieldPath":
return "involvedObject.fieldPath", value, nil
case "reportingController":

This comment has been minimized.

Copy link
@yastij

yastij Nov 6, 2019

Member

reportingController is always set with Component so this is fine, but ReportingInstance Exists in core too and v1beta1 we might want to add it

@k8s-ci-robot k8s-ci-robot requested a review from wojtek-t Nov 6, 2019
func AddFieldLabelConversionsForEvent(scheme *runtime.Scheme) error {
return scheme.AddFieldLabelConversionFunc(SchemeGroupVersion.WithKind("Event"),
func(label, value string) (string, string, error) {
switch label {

This comment has been minimized.

Copy link
@wojtek-t
return "involvedObject.resourceVersion", value, nil
case "regarding.fieldPath":
return "involvedObject.fieldPath", value, nil
case "reportingController":

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Dec 2, 2019

Member

reportingController is not supported in core/v1:
https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/core/event/strategy.go#L102

@liggitt - shouldn't this be changed to source?

This comment has been minimized.

Copy link
@liggitt

liggitt Dec 2, 2019

Member

probably so? I'm less familiar with how the v1 fields map to the v1beta1 fields. This does demonstrate the need for a test covering each field

This comment has been minimized.

Copy link
@yastij

yastij Dec 2, 2019

Member

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/events/event_recorder.go#L88 I added to be able to convert back as this was making some of tests fail during kube-scheduler's migration

@jfbai jfbai force-pushed the jfbai:fix-todo-add-querying-functions-to-event-expansion branch from c955a4b to 43fbfa3 Dec 4, 2019
@jfbai jfbai force-pushed the jfbai:fix-todo-add-querying-functions-to-event-expansion branch from 43fbfa3 to 231301a Dec 4, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 4, 2019

@jfbai: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce 231301a link /test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

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.