Update for openshift/api changes to ImageContentSourcePolicy #984

mtrmac · 2019-07-17T23:49:21Z

~~DO NOT MERGE as is: Includes an UNMERGED PR to openshift/api.~~

- What I did

openshift/api#384 modifies ImageContentSourcePolicy to be explicit about sources == pull specs, and mirrors, instead of defining sets of repositories that can be mirrors of each other.

This updates the vendored API and the relevant code.

- How to verify it

Updated unit tests, I suppose manual testing that it all works.

- Description for the changelog
Updated for ImageContentSourcePolicy distinguishing mirrors from primary repositories

bcrochet · 2019-07-18T10:25:38Z

Also needed for #795

Adds pods to master and worker nodes as appropriate Updates haproxy container to use openshift/router-haproxy image instead of docker.io/library/haproxy Also adds liveness tests for the coredns,mdns-publisher, haproxy and keepalived static pods, changes worker node /etc/resolv.conf to point to node's IP instead of 127.0.0.1 and fix the bug generating haproxy cfg file Depends-On: openshift#943 Depends-On: openshift#984

mtrmac · 2019-07-18T18:51:15Z

/retest

sgreene570

some small comments.

pkg/controller/container-runtime-config/helpers_test.go

pkg/controller/container-runtime-config/helpers.go

sgreene570 · 2019-07-18T18:49:57Z

pkg/controller/container-runtime-config/helpers.go

Is it harmful to leave source in the list? Would it be possible to prevent source from being added to the list rather than removing it later?

Right now not removing source from sortedRepos would result in an unnecessary extra attempt to contact that registry (if the image were not available); it’s not catastrophic but it is unnecessary. Plus, removing source from the list results in registries.conf more similar to how a human would write it, especially for the trivial case of a single RepositoryDigestMirrors.

Yes, not adding the source to the list instead of removing it here would be problematic: Consider

[]RepositoryDigestMirrors{ {Source: "S", Mirrors: {"A"}} // Expected order of attempts: AS {Source: "S", Mirrors: {"BSC"}} // Expected order of attempts: BSC(S) , the second S being redundant/pointless/not applicable }

We need to add the AS edge to topoGraph so that the merged order in sortedRepos is one of ABSC/BASC; without it, it could end up e.g. BSCA (or maybe BSCA(S)), unnecessarily inconsistent with the input entries.

(Sure, this is all corner cases. Everything about including Source as a member of Mirrors is a corner case. Originally I had an assert “Source is last” in here, then I considered the “Source is included in Mirrors” counterexample, and it turned out simpler/clearer/more general to just implement that fully than to detect the case and fail.)

OK I understand. Thank you for elaborating.

sgreene570 · 2019-07-18T18:54:54Z

pkg/controller/container-runtime-config/helpers_test.go

This comment is identical to the previous test comment. Are these 2 tests testing the same thing? Trying to follow along.

They test two different things:

Source is included in Mirrors

Two RepositoryDigestMirrors items contain items in Mirrors in conflicting order.

ICSP does not document how such cases are resolved, it’s not part of the API guarantee.

OTOH, MCC does have an interest of keeping the output stable over time, so that the controller does not issue unnecessary updates to the configuration. So, the implementation sorts the items to behave consistently over time, and these tests hard-code that implementation detail.

The warnings are added to decrease the chance that someone will read the code and the tests, and conclude that the current order is an API promise. Sure, it’s pretty unlikely that anyone will read that, if you think the warnings are too confusing to be worth that, I’d be happy to remove them.

I think the warnings are a good idea, since like you said there is room for confusion. Just wanted to make sure that the warning did need to be present in two places.

runcom · 2019-07-18T23:29:05Z

/approve
/lgtm

mrunalp · 2019-07-18T23:29:44Z

/test e2e-aws

bcrochet · 2019-07-19T11:03:25Z

/retest

runcom · 2019-07-19T14:38:58Z

needs rebase

openshift-bot · 2019-07-19T18:27:34Z

/retest

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

LorbusChris · 2019-07-19T18:51:34Z

@mtrmac could you try updating just the api dep?
dep ensure -update github.com/openshift/api

mtrmac · 2019-07-19T19:09:33Z

Sure, limited the vendor update.

ImageContentSourcePolicy now explicitly distinguishes between sources (pull specs) and mirrors; update the vendored API and the relevant code. Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac · 2019-07-19T19:34:14Z

/retest

LorbusChris · 2019-07-19T19:40:34Z

we're unfortunately still hitting resource limits on AWS :(
/lgtm
nonetheless, let's hope this gets in quickly

openshift-ci-robot · 2019-07-19T19:40:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LorbusChris, mtrmac, runcom

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~~ [LorbusChris,runcom]

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

Adds pods to master and worker nodes as appropriate Updates haproxy container to use openshift/router-haproxy image instead of docker.io/library/haproxy Also adds liveness tests for the coredns,mdns-publisher, haproxy and keepalived static pods, changes worker node /etc/resolv.conf to point to node's IP instead of 127.0.0.1 and fix the bug generating haproxy cfg file Depends-On: openshift#943 Depends-On: openshift#984

Adds pods to master and worker nodes as appropriate Updates haproxy container to use openshift/router-haproxy image instead of docker.io/library/haproxy Also adds liveness tests for the coredns,mdns-publisher, haproxy and keepalived static pods, changes worker node /etc/resolv.conf to point to node's IP instead of 127.0.0.1 and fix the bug generating haproxy cfg file Depends-On: openshift#943 Depends-On: openshift#984 Conflicts: templates/common/baremetal/files/baremetal-coredns-corefile.yaml templates/common/baremetal/files/baremetal-coredns.yaml templates/common/baremetal/files/baremetal-mdns-publisher.yaml templates/master/00-master/baremetal/files/baremetal-haproxy-haproxy.yaml templates/master/00-master/baremetal/files/baremetal-haproxy.yaml templates/master/00-master/baremetal/files/baremetal-keepalived-keepalived.yaml templates/master/00-master/baremetal/files/baremetal-mdns-config.yaml templates/master/00-master/baremetal/files/dhcp-dhclient-conf.yaml templates/worker/00-worker/baremetal/files/baremetal-mdns-config.yaml templates/worker/00-worker/baremetal/files/dhcp-dhclient-conf.yaml

Adds pods to master and worker nodes as appropriate Updates haproxy container to use openshift/router-haproxy image instead of docker.io/library/haproxy Also adds liveness tests for the coredns,mdns-publisher, haproxy and keepalived static pods, changes worker node /etc/resolv.conf to point to node's IP instead of 127.0.0.1 and fix the bug generating haproxy cfg file Due to the fact that both use the same image, there was a bit of confusion here. We want keepalived to track OCP Router 1936 and we want API LB Haproxy pod to have health checked at 50936, which is where we configure haproxy to expose health at. Depends-On: openshift#943 Depends-On: openshift#984

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 17, 2019

openshift-ci-robot requested review from ericavonb and runcom July 17, 2019 23:49

mtrmac force-pushed the icsp-explicit-source branch from fa69609 to 1894758 Compare July 18, 2019 16:43

mtrmac changed the title ~~WIP: Update for openshift/api changes to ImageContentSourcePolicy~~ Update for openshift/api changes to ImageContentSourcePolicy Jul 18, 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 Jul 18, 2019

mtrmac force-pushed the icsp-explicit-source branch from 1894758 to 454c1e1 Compare July 18, 2019 16:52

sgreene570 reviewed Jul 18, 2019

View reviewed changes

mtrmac force-pushed the icsp-explicit-source branch from 454c1e1 to 3272230 Compare July 18, 2019 19:16

openshift-ci-robot assigned runcom Jul 18, 2019

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 18, 2019

mtrmac force-pushed the icsp-explicit-source branch from 4d034b3 to 15a34cf Compare July 19, 2019 19:09

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 19, 2019

Update for openshift/api changes to ImageContentSourcePolicy

15a34cf

ImageContentSourcePolicy now explicitly distinguishes between sources (pull specs) and mirrors; update the vendored API and the relevant code. Signed-off-by: Miloslav Trmač <[email protected]>

openshift-ci-robot assigned LorbusChris Jul 19, 2019

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

openshift-merge-robot merged commit e202842 into openshift:master Jul 19, 2019

mtrmac deleted the icsp-explicit-source branch July 19, 2019 21:57

Update for openshift/api changes to ImageContentSourcePolicy #984

Update for openshift/api changes to ImageContentSourcePolicy #984

Uh oh!

Conversation

mtrmac commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcrochet commented Jul 18, 2019

Uh oh!

mtrmac commented Jul 18, 2019

Uh oh!

sgreene570 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sgreene570 Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

mtrmac Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

sgreene570 Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

sgreene570 Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

mtrmac Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

sgreene570 Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

runcom commented Jul 18, 2019

Uh oh!

mrunalp commented Jul 18, 2019

Uh oh!

bcrochet commented Jul 19, 2019

Uh oh!

runcom commented Jul 19, 2019

Uh oh!

openshift-bot commented Jul 19, 2019

Uh oh!

LorbusChris commented Jul 19, 2019

Uh oh!

mtrmac commented Jul 19, 2019

Uh oh!

mtrmac commented Jul 19, 2019

Uh oh!

LorbusChris commented Jul 19, 2019

Uh oh!

openshift-ci-robot commented Jul 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

mtrmac commented Jul 17, 2019 •

edited

Loading