Add capabiltity to specify sources for release image content by abhinavdahiya · Pull Request #1910 · openshift/installer

abhinavdahiya · 2019-06-27T00:32:04Z

add new fields to allow specifying the sources for release image

type InstallConfig struct {
    ///
    // ReleaseImageSources is the list of sources/repositories for pulling the release-image
    // content.
    // Any of the source/repository can be used to pull the release-image content.
    // The order is the list has no meaning in terms of preference.
    ReleaseImageSources []ReleaseImageSource `json:"releaseImageSources"`
    ///
}
// ReleaseImageSource defines a source for pulling the release-image content.
type ReleaseImageSource struct {
    Repository string `json:"repository"`
}

setup the bootstrap node with mirrors `/etc/containers/registries.conf`

Adds a new asset releaseimage.Source that uses the ReleaseImageSources to calculate the sources for release-image.
Adds the containers-registries.conf template to create the registry sections [1], when more than one sources have been configured for the release-image.
For a list of sources, the containers-registries.conf is setup so that each source can use all the other sources in the list as the mirror.
for example, if the list of sources is a, b, c, d, then the containers-registries.conf is setup like:
registry = a, mirrors = b, c, d
regsitry = b, mirrors = a, c, d
registry = c, mirrors = a, b, d
registry = d, mirrors = a, b, c

For each registry only mirror-by-digest is set to true.

push the image content source policy object for release image

Create a ImageContentSourcePolicy cluster scoped object with name release-image
Uses the releaseimage.Sources asset to generate the ImageContentSourcePolicy's RepositoryDigestMirrors for the release-image.

smarterclayton · 2019-06-27T19:40:19Z

docs/dev/alternative_release_image_sources.md

Note that today the release image and the release image content are not in the same repository (quay.io/openshift-release-dev/ocp-release and ocp-4.0-art-dev-latest). In the future we anticipate moving the content to other locations transparently as we update (to different repos or registries), and we may want to change that after install time, rather than before (i.e. we may not want to hardcode that in the installer, since that is within the payload).

smarterclayton · 2019-06-27T19:41:12Z

docs/dev/alternative_release_image_sources.md

Maybe add a section in here "How to populate the ReleaseImageSources field", and my suggestion would be that we have the mirror command create the correct values that a user cuts and pastes. That allows release mirror to calculate all the values.

Implementing the changes to the mirror command here openshift/origin#23381

smarterclayton · 2019-06-27T19:42:05Z

docs/dev/alternative_release_image_sources.md

Should we consider calling this ReleaseImageSourceRepositories or is that too wordy?

umohnani8 · 2019-06-28T16:23:48Z

docs/dev/alternative_release_image_sources.md

If this is an ordered list, don't we want the primary repository to be listed at the end so that the user knows that the mirrors will be tried first and primary last.
Also, since repositoryDigestMirrors is just a list of sources, we don't really know what the primary repo is, does it mean registries.conf should have an entry for each source listed as the primary? Something like if repoistoryDigestMirrors: A, B, C, D then registries.conf will have the following entries

[[registry]]location = A; mirrors = B, C, D [[registry]]location = B; mirrors = A, C, D [[registry]]location = C; mirrors = A, B, D [[registry]]location = D; mirrors = A, B, C

cc @mtrmac @mrunalp

Actually, if repositoryDigestMirrors is supposed to be a consistent order, the mirrors list would also be the same, and include the primary one:

[[registry]]location = A; mirrors = A, B, C, D [[registry]]location = B; mirrors = A, B, C, D [[registry]]location = C; mirrors = A, B, C, D [[registry]]location = D; mirrors = A, B, C, D

(and registries.conf would be modified not to contact the primary one again if it is already listed in mirrors.)

WRT this PR:

ReleaseImageSources should document whether repository or sources.repository is supposed to be accessed first. Assuming repository is the internet-accessible pull spec and sources.repository is the list of. mirrors, presumably the mirrors should be tried first.

The example of generated repositoryDigestMirrors (and the code generating it) should generate an array of repositories in the order in which they should be attempted; Again, assuming repository is the internet-accessible pull spec and sources.repository is the list of. mirrors, and that the mirrors should be tried first, the example above should have local.registry.com entries before q.io entries in each sources array.

I think in general mirrors should be preferred (we get an advantage in both cases) when specified.

umohnani8 · 2019-06-28T16:25:32Z

docs/dev/alternative_release_image_sources.md

should be "continues to be controlled"

umohnani8 · 2019-06-28T16:33:28Z

docs/dev/alternative_release_image_sources.md

remove "a" after "mirrors the"

abhinavdahiya · 2019-06-28T23:24:10Z

[core@adahiya-0-mbz67-bootstrap ~]$ ls -lah /etc/containers/registries.conf
-rw-------. 1 root root 924 Jun 28 23:14 /etc/containers/registries.conf

[core@adahiya-0-mbz67-bootstrap ~]$ cat /etc/containers/registries.conf
[[registry]]
location = "local.registry.com/ocp/release"
insecure = false
mirror-by-digest-only = true

[[registry.mirror]]
location = "registry.svc.ci.openshift.org/origin/release"
insecure = false
[[registry.mirror]]
location = "registry.svc.ci.openshift.org/origin/4.1-2019-06-27-223758"
insecure = false
[[registry]]
location = "registry.svc.ci.openshift.org/origin/release"
insecure = false
mirror-by-digest-only = true

[[registry.mirror]]
location = "local.registry.com/ocp/release"
insecure = false
[[registry.mirror]]
location = "registry.svc.ci.openshift.org/origin/4.1-2019-06-27-223758"
insecure = false
[[registry]]
location = "registry.svc.ci.openshift.org/origin/4.1-2019-06-27-223758"
insecure = false
mirror-by-digest-only = true

[[registry.mirror]]
location = "local.registry.com/ocp/release"
insecure = false
[[registry.mirror]]
location = "registry.svc.ci.openshift.org/origin/release"
insecure = false

@umohnani8

does the permissions on that file look all right?
does the content look alright?

[core@adahiya-0-mbz67-bootstrap ~]$ journalctl -fu bootkube
-- Logs begin at Fri 2019-06-28 23:13:34 UTC. --
Jun 28 23:16:06 adahiya-0-mbz67-bootstrap systemd[1]: bootkube.service: Main process exited, code=exited, status=125/n/a
Jun 28 23:16:06 adahiya-0-mbz67-bootstrap systemd[1]: bootkube.service: Failed with result 'exit-code'.
Jun 28 23:16:12 adahiya-0-mbz67-bootstrap systemd[1]: bootkube.service: Service RestartSec=5s expired, scheduling restart.
Jun 28 23:16:12 adahiya-0-mbz67-bootstrap systemd[1]: bootkube.service: Scheduled restart job, restart counter is at 21.
Jun 28 23:16:12 adahiya-0-mbz67-bootstrap systemd[1]: Stopped Bootstrap a Kubernetes cluster.
Jun 28 23:16:12 adahiya-0-mbz67-bootstrap systemd[1]: Started Bootstrap a Kubernetes cluster.
Jun 28 23:16:12 adahiya-0-mbz67-bootstrap bootkube.sh[2799]: Pulling release image...
Jun 28 23:16:12 adahiya-0-mbz67-bootstrap bootkube.sh[2799]: error pulling image "registry.svc.ci.openshift.org/origin/release:4.2": unable to pull registry.svc.ci.openshift.org/origin/release:4.2: unable to pull image: Error initializing source docker://registry.svc.ci.openshift.org/origin/release:4.2: error loading registries: invalid URL: cannot be empty

[core@adahiya-0-mbz67-bootstrap ~]$ crio --version
crio version 1.13.9-1.rhaos4.1.gitd70609a.el8

@umohnani8
Seeing ^^ this error on the bootstrap node, do we need a specific version of the CRI-O for v2 registries?

umohnani8 · 2019-06-30T22:34:03Z

@abhinavdahiya v2 registry support was added in cri-o 1.14 cri-o/cri-o#2494. We released cri-o 1.14 for OpenShift last week, I believe it should be in 4.2 soon, if not there already. @mrunalp do you know the timeline for cri-o 1.14 showing up in 4.2 clusters?

mrunalp · 2019-06-30T22:45:07Z

It is blocked on a RCM update of a dependency. Hopefully should be done tomorrow.

…

On Jun 30, 2019, at 3:34 PM, Urvashi Mohnani ***@***.***> wrote: @abhinavdahiya v2 registry support was added in cri-o 1.14 cri-o/cri-o#2494. We released cri-o 1.14 for OpenShift last week, I believe it should be in 4.2 soon, if not there already. @mrunalp do you know the timeline for cri-o 1.14 showing up in 4.2 clusters? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mtrmac · 2019-07-01T16:50:27Z

pkg/types/validation/installconfig.go

This would be better with ParseNormalizedNamed. ParseNamed is ParseNormalizedNamed+ a check that the input is in one specified form (i.e. you must say docker.io/openshift/foo instead of openshift/foo).

Sure, assuming the content is not going to be hosted on docker.io, this does not matter all that much for this code.

It’s rare that you need to forbid the alternative forms, but copy&pasting code across projects could cause the ParseNamed calls to spread to other user-facing code that should accept other forms and use ParseNormalizednamed.

OTOH the tests do explicitly test for rejecting the implied-docker.io forms, so if this was intentional, fine.

BTW if this is going to be long-term supporting Windows/containerd / whole-registry mirrors, note that Parse{Normalized,}Named does not accept registry hostnames (without repository specification). Everything here says “repository”, so it’s OK right now.

So I think we do want to make sure people use complete names. no implicit names are allowed.

BTW if this is going to be long-term supporting Windows/containerd / whole-registry mirrors, note that Parse{Normalized,}Named does not accept registry hostnames (without repository specification)

can you expand a little with examples of how this will break for windows...?

reference.ParseNamed("a.example.com") interprets the input as docker.io/library/a.example.com and rejects it with "repository name must be canonical".

reference.ParseNamed("a.example.com") interprets the input as docker.io/library/a.example.com and rejects it with "repository name must be canonical".

I think it is rejecting it correctly right?

Yes, “correctly” in the sense that the code wanted a repository in canonical form, the input was not a repository in canonical form.

Just pointing out that eventually we might need to support hostnames as well — but the validation can certainly be relaxed (or a separate, differently-validated field, can be added) later when/if that arises.

… FWIW the context is that CRI-O does not currently run on Windows, and containerd’s mirroring configuration is only at the registry (= host:port) resolution. (Of course, this particular bit of configuration would be one of the smaller things that need changing, the whole mirroring process would need adapting for the idea that the mirror must mirror all of (the relevant parts of) the registry, not just a single repository.)

mtrmac · 2019-07-01T16:59:41Z

data/data/bootstrap/files/etc/containers/registries.conf.template

Couldn’t this just encode a sysregistriesv2.V2RegistriesConf object directly instead of hard-coding the current set of fields of $r?

… sorry, probably not trivially in the template language, without adding helpers at least.

umohnani8 · 2019-07-02T17:58:40Z

@abhinavdahiya cri-o 1.14 is in 4.2 now. Tests should pass now after rebase

abhinavdahiya · 2019-07-09T01:14:40Z

/retest

abhinavdahiya · 2019-07-15T19:12:50Z

/retest

smarterclayton · 2019-07-15T21:59:25Z

pkg/types/installconfig.go

Before we hit GA we should clean up this description, I think it's fine for now.

smarterclayton · 2019-07-15T22:00:30Z

API is fine, we can clean up godoc later. Do you need deeper review than that?

abhinavdahiya · 2019-07-15T22:09:59Z

API is fine, we can clean up godoc later. Do you need deeper review than that?

/hold cancel

umohnani8 · 2019-07-16T02:43:41Z

/retest

umohnani8 · 2019-07-16T14:51:32Z

@abhinavdahiya needs rebase.

The documents outlines the proposal and details for using alternate source/repositories for release-image. The proposal is driven by the fact that, only flows using the `oc adm release mirror` to create the alternate sources for release-image will be supported.

* ImageContentSources is the list of sources/repositories for pulling a content. * Adds the validation that all the repositories in the ImageContentSources must be fully-qualified names [1], and ensuring each group of sources is a disjoint set. [1]: https://github.com/containers/image/blob/abf32c4ea589cb8e96bdca5a478dba68f11980e5/docker/reference/regexp.go#L53-L56

…s.conf for bootstap * Creates a new asset releaseimage.Image for generating the release-image. * Updates the ignition.bootstrap to use the releaseimage.Image asset to calculate the release-image * also updates the `version` subcommand to find the embedded default release-image from new package. * creates the containers-registries.conf file from the ImageContentSources from InstallConfig

…for the release-image * Create a ImageContentSourcePolicy [1] cluster scoped object with name `release-image` * Uses the releaseimage.Sources asset to generate the `RepositoryDigestMirrors` [2] for the release-image [1]: https://github.com/openshift/api/blob/de5ca909c7322bb8d06fa5a9e5604491b373da52/operator/v1alpha1/types_image_content_source_policy.go#L11 [2]: https://github.com/openshift/api/blob/de5ca909c7322bb8d06fa5a9e5604491b373da52/operator/v1alpha1/types_image_content_source_policy.go#L35

…ub.com/openshift/api * github.com/containers/image : helps in validation of image references and creating the containers-registries.conf [1] for bootstrap node. * github.com/openshift/api : brings in the ImageContentSourcePolicy type [2] [1]: https://github.com/containers/image/blob/v2.0.0/docs/containers-registries.conf.5.md#name [2]: https://github.com/openshift/api/blob/de5ca909c7322bb8d06fa5a9e5604491b373da52/operator/v1alpha1/types_image_content_source_policy.go#L11

mtnbikenc · 2019-07-16T17:30:12Z

docs/dev/alternative_release_image_sources.md

+Release Image q.io/ocp/release-x.y@sha256:abcd was successfully mirrored to local.registry.com/ocp/release-x.y@sha256:abcd
+
+Following section can be added to the install-config.yaml to create a cluster using new repository:
+imageContentSources:


Should this be imageContentSource:? I'm seeing both in this code and I need to align with my PR for oc. openshift/origin#23381

i think it needs to be imageContentSources based on the json tag on the actual install-config
https://github.com/openshift/installer/blame/bea51930a0e49b5da2fb58f2184ba27e79ceeb6c/docs/dev/alternative_release_image_sources.md#L29

Thanks! I'll update the mirror command.

patrickdillon · 2019-07-16T19:26:54Z

/lgtm

openshift-ci-robot · 2019-07-16T19:27:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, patrickdillon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya]

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

umohnani8 · 2019-07-16T19:34:04Z

/retest

openshift-bot · 2019-07-16T19:58:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

patrickdillon · 2019-07-16T20:19:40Z

/skip

patrickdillon · 2019-07-16T22:56:53Z

/test e2e-aws

openshift-ci-robot · 2019-07-17T00:17:45Z

@abhinavdahiya: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-scaleup-rhel7	`bea5193`	link	`/test e2e-aws-scaleup-rhel7`
ci/prow/e2e-openstack	`bea5193`	link	`/test e2e-openstack`
ci/prow/e2e-aws	`bea5193`	link	`/test e2e-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

…sage This code-branch should be very rare, probably mostly internal testing. A chattier line in those situations isn't a big user-experience concern, and including the pullspec makes it easier for folks who are trying to understand internal-test results to figure out which pullspec was being used. The log line is originally from 456a373 (pkg: Use the sources from InstalConfig to create containers-registries.conf for bootstap, 2019-06-28, openshift#1910), and it was tweaked in cd0b70f (asset: Make warning message read for humans, 2019-09-15, openshift#2361), and neither of those make explicit arguments for why they didn't include the pullspec.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 27, 2019

openshift-ci-robot requested review from jcpowermac and jhixson74 June 27, 2019 00:32

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 27, 2019

abhinavdahiya force-pushed the release_image_sources branch from 493aaed to 0e20555 Compare June 27, 2019 19:33

smarterclayton reviewed Jun 27, 2019

View reviewed changes

abhinavdahiya force-pushed the release_image_sources branch 2 times, most recently from ce93e2d to 903b3f8 Compare June 28, 2019 01:12

umohnani8 reviewed Jun 28, 2019

View reviewed changes

docs/dev/alternative_release_image_sources.md Outdated

Copy link

Contributor

umohnani8 Jun 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be "continues to be controlled"

umohnani8 reviewed Jun 28, 2019

View reviewed changes

docs/dev/alternative_release_image_sources.md Outdated

Copy link

Contributor

umohnani8 Jun 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove "a" after "mirrors the"

abhinavdahiya force-pushed the release_image_sources branch from 903b3f8 to 7a63019 Compare June 28, 2019 23:00

abhinavdahiya changed the title ~~WIP: Add capabiltity to specify sources for release image content~~ Add capabiltity to specify sources for release image content Jun 28, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2019

mtrmac reviewed Jul 1, 2019

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2019

abhinavdahiya force-pushed the release_image_sources branch from 7a63019 to 4cdd053 Compare July 2, 2019 18:34

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2019

abhinavdahiya force-pushed the release_image_sources branch from 8e8f464 to 08cce62 Compare July 3, 2019 16:45

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2019

abhinavdahiya force-pushed the release_image_sources branch from 08cce62 to 5812499 Compare July 8, 2019 22:39

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2019

smarterclayton reviewed Jul 15, 2019

View reviewed changes

pkg/types/installconfig.go Outdated

Copy link

Contributor

smarterclayton Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we hit GA we should clean up this description, I think it's fine for now.

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2019

abhinavdahiya added 5 commits July 16, 2019 09:04

abhinavdahiya force-pushed the release_image_sources branch from 30c1084 to bea5193 Compare July 16, 2019 16:06

mtnbikenc reviewed Jul 16, 2019

View reviewed changes

openshift-ci-robot assigned patrickdillon Jul 16, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2019

mtnbikenc mentioned this pull request Jul 16, 2019

Add repo details for disconnected install/upgrade openshift/origin#23381

Closed

openshift-merge-robot merged commit 1679af7 into openshift:master Jul 16, 2019

patrickdillon mentioned this pull request Jul 17, 2019

Update docs for release mirrors. #2012

Merged

mtrmac mentioned this pull request Jul 18, 2019

release/mirror: Add examples for using the new mirror openshift/oc#6

Merged

stbenjam mentioned this pull request Aug 14, 2019

hack: remove compile time RELEASE_IMAGE override #2217

Merged

wking mentioned this pull request Oct 4, 2022

pkg/asset/releaseimage/pullspec: Include override pullspec in log message #6460

Merged

Conversation

abhinavdahiya commented Jun 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

add new fields to allow specifying the sources for release image

setup the bootstrap node with mirrors /etc/containers/registries.conf

push the image content source policy object for release image

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya commented Jun 28, 2019

Uh oh!

umohnani8 commented Jun 30, 2019

Uh oh!

mrunalp commented Jun 30, 2019 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

umohnani8 commented Jul 2, 2019

Uh oh!

abhinavdahiya commented Jul 9, 2019

Uh oh!

abhinavdahiya commented Jul 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Jul 15, 2019

Uh oh!

abhinavdahiya commented Jul 15, 2019

Uh oh!

umohnani8 commented Jul 16, 2019

Uh oh!

umohnani8 commented Jul 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickdillon commented Jul 16, 2019

Uh oh!

openshift-ci-robot commented Jul 16, 2019

Uh oh!

umohnani8 commented Jul 16, 2019

Uh oh!

openshift-bot commented Jul 16, 2019

Uh oh!

patrickdillon commented Jul 16, 2019

Uh oh!

patrickdillon commented Jul 16, 2019

abhinavdahiya commented Jun 27, 2019 •

edited

Loading

setup the bootstrap node with mirrors `/etc/containers/registries.conf`