Bug 1876500
| Summary: | [OCP 3.11] lastTriggeredImageID not honored when applying manifest | ||
|---|---|---|---|
| Product: | OpenShift Container Platform | Reporter: | Mario Abajo <mabajodu> |
| Component: | Build | Assignee: | Adam Kaplan <adam.kaplan> |
| Status: | CLOSED DEFERRED | QA Contact: | wewang <wewang> |
| Severity: | medium | Docs Contact: | |
| Priority: | high | ||
| Version: | 3.11.0 | CC: | adam.kaplan, aos-bugs, gmontero, joboyer, mfiedler, openshift-bugs-escalate, ppostler, rdlugyhe, sttts, wzheng |
| Target Milestone: | --- | ||
| Target Release: | 4.7.0 | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | If docs needed, set a value | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2020-10-22 18:11:58 UTC | Type: | Bug |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
Mario Abajo
2020-09-07 10:33:37 UTC
This bug is related to the upstream GitHub issue. Fixing it requires an API change that we probably won't be able to backport (we need to move the `lastTriggeredImageID` to a status field). Naively running `oc apply -f` against a BuildConfig YAML that has image change triggers does not work because the image change trigger adds `lastImageChangeTriggeredID: <sha>` to the BuildConfig spec. Because of the way the image change triggers API is implemented, the `lastImageChangeTriggeredID` is removed if a subsequent call to `oc apply -f` is made and the BuildConfig YAML does not contain the `lastImageChangeTriggeredID`. In my investigation, the best options to work around this issue are as follows: 1. Write a "pre-process" script which updates the BuildConfig triggers YAML in git with the `lastImageChangeTriggeredID` in from the cluster, and incorporate this into the GitOps workflow. 2. Disable the ImageChange trigger and use a combination of scheduled imagestream imports [1] and a CronJob to run builds on a regular interval with the latest imported images. The CronJob could be added to the cluster-wide project template to ensure all namespaces run builds at regular intervals [2]. 3. Use `oc patch` commands to reconcile the state of the BuildConfig. The following repo utilizes a script and proposed workflow to accomplish this task [3]. [1] https://docs.openshift.com/container-platform/4.5/openshift_images/image-streams-manage.html#images-imagestreams-import_image-streams-managing [2] https://docs.openshift.com/container-platform/4.5/applications/projects/configuring-project-creation.html [3] https://github.com/adambkaplan/openshift-gitops-imagechange-triggers After much discussion, the engineering team has determined that we do not have a feasible means of fixing this issue. The triggers API field is problematic for two reasons: 1. `triggers:` is a set-like array that does not define a patchMergeKey [1]. Updating this array via `oc apply -f` therefore fully replaces the contents of the array, not individual elements. 2. `lastTriggeredImageID` is a data element that belongs in the BuildConfig `status` subresource. `spec` is not an appropriate place to store information that represents cluster state. Fixing 1 requires us to introduce a field that acts as the merge key. To maintain backwards compatibility, this field would need to be optional. Technically this is allowed, but a) could lead to buggy behavior, and b) would not work with server-side apply, which is currently in beta [2]. Therefore, engineering is not in favor of introducing this API element. Fixing 2 likewise introduces behavior that could break backwards compatibility. In this scenario we would deprecate the `lastTriggeredImageID` field in `spec`, record this information in both `spec` and `status`, and then moving forward use the data in `status` to trigger builds. Unfortunately, the `lastTriggeredImageID` field has a peculiar feature where if it is removed or set to the empty string, a new build is created (basically, a forced image change trigger). We lose this capability if we only use data in `status` to drive image change triggers in builds. Therefore, we have decided to close this issue as WONTFIX. The best option to work around this issue for this particular GitOps scenario is to add jobs/scripts that update the BuildConfig manifests in git with the lastTriggeredImageIDs found on the cluster. [1] https://kubernetes.io/docs/tasks/manage-kubernetes-objects/declarative-config/#merge-individual-elements-of-a-list-of-complex-elements [2] https://kubernetes.io/docs/reference/using-api/api-concepts/#server-side-apply There has been further discussion among the engineering team, and we have reversed course on our decision. Our review revealed that the behavior identified in comment 10 is not officially documented [1]. The API godoc in fact notes that this field is meant for internal purposes only [2]. We are therefore going to deprecate the `lastTriggeredImageId` field in `spec` and move it to a new `status` field. This will be done in two phases: 1. Phase 1 - officially deprecate the field, and begin recording the trigger IDs in status [3]. 2. Phase 2 - drive ImageChange triggers from status only. Phase 1 will include the official deprecation notice, as well as an alert if deprecated behavior is detected on the cluster. This work will be tracked on issues.redhat.com, under the issues referenced below. This bug will remain closed, but with the DEFERRED status. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1876500#c10 [2] https://github.com/openshift/api/blob/master/build/v1/types.go#L993-L994 [3] https://issues.redhat.com/browse/BUILD-189 [4] https://issues.redhat.com/browse/BUILD-190 Provided a release note in https://issues.redhat.com/browse/BUILD-186 "builds: Record Last Triggered Image ID in status" |