prow: core: one CRD Spec field fulfilled by many controllers makes Status muddy

[stevekuznetsov]

Right now we have been kind of shoving k8s Pod concepts into the Jenkins operator and it's showing in the ProwJob.Status fields. The ProwJob.Satus.PodName is just a nonsense field for ProwJobs fulfilled by Jenkins job builds; the ProwJob.Status.BuildID for pods is prescriptive from Prow (through tot) whereas the same field for Jenkins builds is observational (from Jenkins). We could use some RawExtension magic to keep some part of the ProwJob.Status extensible for whatever controller is fulfilling the specific ProwJob, or we need to think about some other structure. The current status is not going to look nice when we work on implementing the CircleCI controller.

/cc @Kargakis @cjwagner @BenTheElder @fejta @spxtr
/area prow
/kind bug
/kind question

[BenTheElder]

xref: #6853 ?

[stevekuznetsov]

Yeah that PR just made it worse :|

[0xmichalis]

We could use some RawExtension magic to keep some part of the ProwJob.Status extensible for whatever controller is fulfilling the specific ProwJob

You would have to sync with @kubernetes/sig-api-machinery-misc on whether this is a sane thing to do. In the meantime, I would prefer to see us extending status via annotations (status updates that include annotations updates are acceptable; see kubernetes/kubernetes#20978).

[stevekuznetsov]

I talked with David, his thoughts were, as I remember them:

• don't have one CRD for many controllers
• embed the Spec field into many CRDs for many controllers
• RawExtension is less preferrable to just having top-level fields that are empty
• data in annotations is not type-safe and should be an absolute last resort

If we have to parse out data/json from annotations we're in a worse spot than RawExtension IMO

[0xmichalis]

I don't think we will need to put JSON in an annotation. This definitely depends on the target agent API but at least in the case of Jenkins, all we need is an extra string. Do we expect something different in the case of CircleCI? If we can get away with a couple of annotations, it is going to be much simpler than requiring separate CRDs per agent or doing raw extensions.

Also, on the separate CRD comment, I think it's unlike we will ever have a separate top-level CRD in prow, because then you would have to make every prow component pluggable, whereas today you only need to provide an agent operator and stuff like hook or horologium work ootb. Maybe we can have children CRDs that use owner references to link back to ProwJobs but again at least in the Jenkins case, we don't really need them.

[stevekuznetsov]

Agreed -- the simplest solution by far is still to have one Status object with disjoint sets of fields and clients that know which fields to look at. Still messy and seems to break some of the core assumptions about Spec/Status and controller ownership in core k8s and behind k8s-style APIs.

[0xmichalis]

That's why I prefer annotations over disjoint fields. It's a cleaner solution and it promotes a pattern for building controllers out of tree, even though I think it's nice to keep all implementations in test-infra for discoverability. We rarely break fields and I am not worried about compatibility with annotations. That being said, I am not crazy about it, the current approach works.

[stevekuznetsov]

Yeah, just feels bad to stringly type by indexing into an annotations map.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[stevekuznetsov]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[stevekuznetsov]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

[stevekuznetsov]

/reopen
/remove-lifecycle rotten

[k8s-ci-robot]

@stevekuznetsov: Reopened this issue.

In response to this:

/reopen
/remove-lifecycle rotten

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.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.