Allow human readable duration strings in DecorationConfig duration fields.

[cjwagner]

This PR adds a new Duration type that can parse either 'integer number of nanoseconds' or 'duration string' format and serializes to the 'duration string' format. This type is used to replace the existing DecorationConfig duration fields with backwards compatibility for existing values. Now those fields can be specified in config like "1h15m" instead of 4500000000000 (ns units).

/cc @stevekuznetsov @sebastienvas

[sebastienvas]

thanks
/lgtm
/hold

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: b69f654312f6b0b733348257dbdbfd51ce4090cd

[Katharine]

I think the _readable suffix is pretty awkward, and likely to be confusing to users.

Couldn't we just make timeout and grace_period accept either? At this point you already have to post-process the config, so you could just make them strings (or something else that can take either), try parsing a duration, then try parsing a number.

[stevekuznetsov]

Are we adding a deprecation warning?

[stevekuznetsov]

+1 to Katharine's comment

[cjwagner]

I got rid of the _readable fields and switched to using a new Duration type that can parse either 'integer number of nanoseconds' or 'duration string' format and serializes to the 'duration string' format.
The API server shouldn't care that we are changing the type of this field right? I think it is sufficient that our components parse both the existing and new formats properly.

[stevekuznetsov]

Yeah the type is irrelevant I think, don't think etcd cares about how to store the record. Only one way to find out ;)

[cjwagner]

Passes tests now. (My editor's go formatting disagrees with go fmt...) PTAL.

[stevekuznetsov]

The changes LGTM but can you try installing the current CRD, creating some, then changing the CRD and seeing if any of our clients fall over on a test cluster since the failure mode there is pretty rough?

/hold

[stevekuznetsov]

nit: do we need the 0s here? That's odd

[cjwagner]

Unfortunately that is how duration strings are printed: https://play.golang.org/p/xyrKD-34gfP

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 8693cfde7fd607c1379eb68f9bab621592b3cb62

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, stevekuznetsov

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:

• config/OWNERS [cjwagner,stevekuznetsov]
• prow/OWNERS [cjwagner,stevekuznetsov]

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

[cjwagner]

The changes LGTM but can you try installing the current CRD, creating some, then changing the CRD and seeing if any of our clients fall over on a test cluster since the failure mode there is pretty rough?

I thought that is what we discussed here #12100 (comment)?

I agree that the failure mode could be nasty if I made a mistake and if there is a better way to test this I'm eager to do so, but the ProwJob CRD did not change at all, so I'm not sure what you are suggesting I do to test this.

[stevekuznetsov]

Eek sorry, poor English -- I meant to install the CRD, create some ProwJobs (creating some) and edit the ProwJobs ("changing the CRD") with the new clients to see it function. But if you want to just roll it out to prow.k8s.io and roll back in case things get weird that also SGTM

[cjwagner]

Deploying from this PR or merging this PR then deploying master should be equivalent, but deploying to prod from a PR seems like bad practice so lets merge this and deploy from master.
If something is broken reverting is cheap.
/hold cancel

[k8s-ci-robot]

@cjwagner: Updated the job-config configmap in namespace default using the following files:

• key generated-security-jobs.yaml using file config/jobs/kubernetes-security/generated-security-jobs.yaml

In response to this:

This PR adds a new Duration type that can parse either 'integer number of nanoseconds' or 'duration string' format and serializes to the 'duration string' format. This type is used to replace the existing DecorationConfig duration fields with backwards compatibility for existing values. Now those fields can be specified in config like "1h15m" instead of 4500000000000 (ns units).

/cc @stevekuznetsov @sebastienvas

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.

[cjwagner]

This appears to be working as intended. Our currently deployed config contains duration values in both formats and everything is being serialized to the human readable strings: https://prow.k8s.io/config