Modify ImageContentSourcePolicy to distinguish between sources (pull specs) and mirrors#384
Conversation
openshift-ci-robot
added
the
size/S
|
/hold ... for discussion. @smarterclayton @umohnani8 @abhinavdahiya PTAL. This is a follow-up to #354. |
openshift-ci-robot
added
the
do-not-merge/hold
| // +required | ||
| Source string `json:"source"` | ||
| // mirrors is one or more repositories that may also contain the same images. | ||
| // Each mirror is tried in the order specified here* , and before the original “source”, which is contacted last. |
There was a problem hiding this comment.
I don't want to enshrine the "contacted last" into the API guarantee. You need to weaken this statement a bit, possibly:
The order of mirrors in this list is treated as the user's desired priority, while source is by default considered lower priority than all mirrors. Other cluster configuration may impact the exact order mirrors are contacted in, or some mirrors may be contacted in parallel, so this should be considered a preference rather than a guarantee of ordering.
There was a problem hiding this comment.
Updated to that text (and weaved in a note about conflicting repositoryDigestMirrors).
mtrmac
force-pushed
the
icsp-explicit-source
branch
from
f371d86 to
88da2fa
Compare
|
This looks good to me (see my comment). It's ok to break this field but we may need to stage the changes with installer (or have installer temporarily disable setting this field). Will let others review but I think this is a better API than before, and I would expect it to be mirrored into install-config unless issues are discovered that would counter that (let's have that discussion here) |
|
Note that if we do merge this we need to communicate to everyone that we will be breaking the API within the master branch and the timeframe for the fixes landing, so that teams aren't held up by being unable to vendor their code. |
mtrmac
force-pushed
the
icsp-explicit-source
branch
from
88da2fa to
5a97de7
Compare
openshift-ci-robot
added
size/M
|
👍 |
|
Waiting for Abhinav feedback |
mtrmac
force-pushed
the
icsp-explicit-source
branch
from
5a97de7 to
7076b89
Compare
…specs) and mirrors Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
The API looks sound. I can stage changes in installer based on this bump... /lgtm |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/approve |
|
@abhinavdahiya @smarterclayton We need approve from you :) |
|
LGTM. @smarterclayton needs an approve label |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, mrunalp, mtrmac, smarterclayton The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
Generate operator/v1alpha1 CRDs, update for openshift/api#384
7 participants
Note: This modifies an already committed type. Is that OK?
The current ImageContentSourcePolicy does not scale well when a large set of sources are all mirrored into a single mirror repository; in that case they are all considered to be possible mirrors of each other (in some order), and referring any of them could cause all of the others to be accessed (taking a long time to search through them, especially if they were firewall off and had to time out).
Instead, make an explicit distinction between sources (where images come from, and which appear in pull specs) and mirrors (where images are copied to, an infrastructure concept invisible to most of the cluster, which only refers to the sources).