Bug 1745192: configure builds to use mirrors in disconnected enivronments

[gabemontero]

@openshift/openshift-team-developer-experience @bparees @nalind @mtrmac

/assign @adambkaplan
/assign @nalind
/assign @mtrmac

initially marked as WIP because I'm a little unclear on how to fully migrate the build controller's existing representation of registries.conf to use the V2 registries.conf format, but I have enough in place I believe to solicit feedback/guidance

Ultimately I'll re-enable / add more unit tests once that migration/mapping is sorted out

[openshift-ci-robot]

@gabemontero: GitHub didn't allow me to assign the following users: nalind.

Note that only openshift members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

@openshift/openshift-team-developer-experience @bparees @nalind @mtrmac

/assign @adambkaplan
/assign @nalind
/assign @mtrmac

initially marked as WIP because I'm a little unclear on how to fully migrate the build controller's existing representation of registries.conf to use the V2 registries.conf format, but I have enough in place I believe to solicit feedback/guidance

Ultimately I'll re-enable / add more unit tests once that migration/mapping is sorted out

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.

[openshift-ci-robot]

@gabemontero: This pull request references Bugzilla bug 1745192, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1745192: WIP: configure builds to use mirrors in disconnected enivronments

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.

[gabemontero]

Hmm ... doesn't like my combination of WIP and Bug ....

/hold

[mtrmac]

Thanks for working on this.

I’m afraid correctly handling ICSP, and generating registries.conf, is much more work, and something we should keep consistent throughout the cluster.

Basically some version of https://github.com/openshift/machine-config-operator/blob/cfb78728a6e985fc40a3ef318ba1de359de17a6d/pkg/controller/container-runtime-config/helpers.go#L321 is necessary (providing the docker.io search path[1] configuration either by hard-coding the input, or by changing the API of that function a bit)

So, we should extract that into a separate Go subpackage, and call that both from the machine-config-controller and in here.

Where is the best place for that subpackage to be maintained? In MCO? In here? In some other repository? (I suppose even containers/image would be an option, though importing openshift/api into an otherwise OpenShift-unaware library feels a bit unclean.)

---

[1] Leaving completely aside whether the node and builder configuration should differ in that aspect. That should be eventually resolved, but that’s not the purpose of this PR.

[gabemontero]

@mtrmac it seems to me that the https://github.com/openshift/machine-config-operator/blob/cfb78728a6e985fc40a3ef318ba1de359de17a6d/pkg/controller/container-runtime-config/helpers.go#L321 code should be moved to https://github.com/openshift/library-go to facilitate the openshift conventions around sharing utilities like this. In general we don't want the openshift ocm-o depending on the mco. Certainly it could be added to library-go first, and then the mco could be subsequently changed to leverage it.

Anyway, that is my take on answering your question around where that code should live to be properly shared as you suggested.

[gabemontero]

OK, while certainly not complete from a coding or unit-test perspective, and some specific questions noted with TODO in the implementation, I've updated the PR to hopefully progress the evaluation and discovery of next steps.

In particular:

• I'm pulling in containers/image, and leverage the systemregistries type directly
• I've copied a variety of MCO logic directly into build controller (don't want to vendor in the MCO, at least not yet ... especially if we can move some logic to library-go, especially once the TODO's are resolved) per @mtrmac 's initial pointer

[mtrmac]

This should work, but I’d very strongly prefer to not maintain the code in two places. (E.g. recent #19 .)

If library-go is the right repo, I’ll prepare a PR for that one.

(I haven’t verified that the copy&pasted code is unmodified.)

[gabemontero]

ok I've pushed a new round of updates stemming from @mtrmac 's last set of comments, as well as adding

• the search registry unit test back in
• adding the unit test around using sysregistriesv2.GetRegistries that @mtrmac suggested previously

Assuming we are good with those updates, aside from any new ones that arise independently, we are down to settling on a landing spot for the modified helpers currently residing in MCO, and adjusting the copied code here accordingly.

[mtrmac]

ACK.

Let me take a stab at the library-go PR later today…

[gabemontero]

two minor clean up items from @mtrmac pushed

@mtrmac - on library-go, I revisited the README to make sure I understood what was involved and now remember the "high bar" warnings about what is required for inclusion.

If possible you might want to run the idea by the folks in the OWNERS file and see how amenable they would be before you go too far down this path.

If they push back, we'll then need to revisit either

• building a new repo with what we want (I did see any other library like repo under github.com/openshift
• just making the needed changes in the MCO, and we bite the bullet here to vendor that in
• or we live with the code copy until post 4.2, when we have more time for one of the first two bullets

[mtrmac]

@mtrmac - on library-go, I revisited the README to make sure I understood what was involved and now remember the "high bar" warnings about what is required for inclusion.

If possible you might want to run the idea by the folks in the OWNERS file and see how amenable they would be before you go too far down this path.

In the meantime, I have created openshift/machine-config-operator#1087 . Please check that the proposed API works well.

[gabemontero]

/retest

[gabemontero]

the verify-deps failure is some complaints around by glide/vendor bump for container/images

starting off tabula rasa / refreshing the bump ... I did it from a copy of my branch and then picked into the version of my branch I pushed ... maybe that had a hiccup

[gabemontero]

ok make verify-deps is succeeding for me locally ... pushing commits

[gabemontero]

/retest

[gabemontero]

clean test runs

[mtrmac]

I’d still much prefer the code to exist in one place, but I’m not sure whether we can make that happen in time.

In the meantime, could you apply openshift/machine-config-operator#1082 to this copy, please?

[gabemontero]

at this point @mtrmac I'd rather see openshift/machine-config-operator#1087 merge and I refactor towards that ... if the library-go attempt gets traction, we can pivot.

before we push on that though, let me revisit the "do we need this in 4.2" thread with the various parties I mentioned in your MCO PR and based on the results, go from there

[gabemontero]

I've sent out an email to the interested parties to get a decision on the risk of merging openshift/machine-config-operator#1087 for 4.2 and then basing this PR off of that

[adambkaplan]

@gabemontero openshift/machine-config-operator#1087 is now LGTM - we'll likely want to copy the code in with the same registries subpackage.

[mtrmac]

Not copy, vendor in and call, please.

[gabemontero]

/hold cancel

[gabemontero]

OK MCO is vendored in, calling @mtrmac 's new API, and rebased on recent changes from @adambkaplan

@adambkaplan bump / PTAL

[gabemontero]

/retest

[gabemontero]

ok update pushed for @mtrmac 's #19 (comment)

Rationales for not making adjustments for the other 2 posted and he and I have reached consensus to leave as is.

@adambkaplan - PTAL

[gabemontero]

and we have passing test @adambkaplan ... bump

[adambkaplan]

@gabemontero nits on log levels. otherwise looks good.

For 4.3 I'd like to see us create a library-go-esque repo to help us generate the essential container runtime config files, since these are used across MCO, builds, and image stream import:

1. registries.conf
2. policy.json

[gabemontero]

On Wed, Sep 4, 2019 at 10:55 AM Adam Kaplan ***@***.***> wrote: ***@***.**** requested changes on this pull request. @gabemontero <https://github.com/gabemontero> nits on log levels. otherwise looks good. For 4.3 I'd like to see us create a library-go-esque repo to help us generate the essential container runtime config files, since these are used across MCO, builds, and image stream import:

yeah Miloslav and I have talked about that for 4.3 As his library-go PR did not get much traction

…

1. registries.conf 2. policy.json ------------------------------ In pkg/build/controller/build/build_controller.go <#19 (comment)> : > return "", nil } klog.V(4).Info("overrode registry settings for builds") - klog.V(5).Infof("generated registries.conf for build pods: \n%s", string(configTOML)) - return string(configTOML), nil + klog.V(0).Infof("generated registries.conf for build pods: \n%s", string(newData.Bytes())) I would not expect the full registries.conf file to be dumped into normal logs. Please revert to V(5) ------------------------------ In pkg/build/controller/build/build_controller.go <#19 (comment)> : > if err != nil { + klog.V(4).Infof("MCO library had problem building registries config: %s", err.Error()) Use a higher log level for an error - V(0)? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19?email_source=notifications&email_token=AA3NU5C6CGWZFOZN3645FDLQH7D5BA5CNFSM4IPGFO42YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCDUIU5Y#pullrequestreview-283675255>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3NU5FXEMVBE5NDAKAMBGTQH7D5BANCNFSM4IPGFO4Q> .

[gabemontero]

updates pushed @adambkaplan ...PTAL

[adambkaplan]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, gabemontero

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:

• OWNERS [adambkaplan]

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