[RHCLOUD-19399] Move to GVK queries and add a schematized query factory method

[adamrdrew]

We infer GVK for queries from a provided Object but in testing this seems unreliable. For example, Deployment often has an empty GVK - I don't know why. This refactor uses GVK instead, but provides a public type that has common GVKs in it.

Creating this in draft for testing in Clowder and Front End.

[adamrdrew]

@psav This PR undoes something we did during the discussion on #6 - We made the query system accept a type specimen object, like a Deployment or Kafka, and then the ResourceCounter grabbed the GVK off that and performed the queries. Turns out this isn't reliable. For reasons I don't understand an Object may have a GVK filled with empty strings. For example, creating a &apps.Deployment{} and then getting the GVK from it will have group, version, and kind of "" - I don't know why this is. It seems reliable with the strimzi stuff, but not with the core k8s types. So, I decided to unroll this and go back to an explicit GVK query because it works 100% of the time. That said I did provide a CommonGVKs type that holds the values for common GVKs. This may not be as clever (or possibly idiomatic, I don't know) as using inference here, but its more reliable.

[adamrdrew]

Got some review from @psav and we landed on the idea of using scheme to look up GVK for the type specimen client.Object. I pushed up a change that adds a new factory method called MakeQuery that accepts a scheme and type specimen along with a UID and namespace array and returns a ResourceCounterQuery - this keeps the ResourceCounter API stable but adds some extra functionality for GVK inference.

[adamrdrew]

Oh, bummer worth noting: MakeQuery is a public method with no tests. This is, like, not cool. But I don't see a way to test it that isn't actually just testing k8s. If we trust a runtime.Scheme knows how to look up a GVK for a client.Object then I mean there's not really anything else this method does.

[psav]

I see what you've done here and though I like it, the common way that k8s uses to pass around types for the client, is to pass empty objects. When we you set up watches etc, you pass in empty types that it internally converts to GVKs and uses. While there is an efficiency there, we have used this same paradigm in multiple places through the client facing code, and I think I'd prefer to stick with it, thoughts?

[adamrdrew]

Hackathon, been off the grid - coming back to this now.

I think MakeQuery does exactly what you're looking for:

query, _ := resources.MakeQuery(&apps.Deployment{}, *scheme, namespaces, o.GetUID())

counter := resources.ResourceCounter{
    Query: query,
    ReadyRequirements: []resources.ResourceConditionReadyRequirements{{
        Type:   "Available",
        Status: "True",
    }},

Its been a while since we walked about this, but I think that's what you suggested I implement: a factory method that takes a type specimen and a scheme and then constructs the query for you. I could make the factory method return the ResourceCounter instead of the query, I just felt that was a bit more work than a single method should do, but I can do it.

[psav]

I'm happy with submitting the Query separately, but I'd like to keep the interface of passing in the specimen so that it is aligned with the other tools in the framework.

[adamrdrew]

Thanks for the input. Unfortunately, there's a communication / understanding gap here. I am obviously misunderstanding what you are asking of me. Could you provide me an example of what you want this to look like? Then I'll implement it that way. The way I read your input I've already done what you asked, so I'm not sure what to do without an example. Sorry :(

[adamrdrew]

I pushed up a commit that attempts to do what I think you might be asking me to do. It takes the pattern seen above, MakeQuery and then create a ResourceCounter and abstracts it behind a single method. That's my best guess for what you are asking me for here. If I got it right then excellent, if not then let's have a chat so I can see what I'm not getting here. Thanks!

[psav]

We also need to protect against the fact that we could have a nil length without an err. This would blow up here without it.

[adamrdrew]

Would it actually be possible to end up with nil there? We're passing in a client.Object and then using runtime.Scheme to get its GVK. That whole code path is in kubernetes world - assuming this works and an error isn't raised:

	gvk, _, err := scheme.ObjectKinds(typeSpecimen)
	if err != nil {
		return ResourceCounterQuery{}, err
	}

I wouldn't think we could ever get nil from gvk[0] on the next line. I'd figure if anything we'd end up bailing in that err condition. You've been doing this longer than me tell me what you think.

[psav]

There is the possibility that the specimen is provided but isn't present in the schema, I think that would cause the nil length. - Thoughts?

[adamrdrew]

I'm not sure - I assumed passing something that isn't in the schema to scheme.ObjectKinds would result in err not nil. I'll do some testing and see what I end up with.

[adamrdrew]

I read the implementation https://github.com/kubernetes/apimachinery/blob/master/pkg/runtime/scheme.go#L243 and it looks like we can't ever get nil there. But I decided to test a bit for sure. If you pass in a type that isn't registered to the scheme you get a not registered error, so I think we're good here. I put my findings into two new unit tests: one for MakeQuery with a registered type and one without. Thanks!

[psav]

OK no worries

[psav]

A couple replies...