redhat-openshift / acm-submariner-tester

1 stars 0 forks source link

Replacing MachineConfig with ImageContentSourcePolicy to configure OCP registry mirror #22

Open manosnoam opened 2 years ago

manosnoam commented 2 years ago

Hi @SteveMattar Looking at the MachineConfig we're creating on every OCP node, in order to configure mirroring from internal registry to external images: https://github.com/redhat-openshift/acm-submariner-tester/blob/8bd34177193cfac20670c2ff623c598ab5a28f7f/setup_subm.sh#L3199-L3215

Which also includes the config source mirror data: https://github.com/redhat-openshift/acm-submariner-tester/blob/8bd34177193cfac20670c2ff623c598ab5a28f7f/setup_subm.sh#L3100-L3180

Now I'm curious to know if it can be replaced with a simpler method as @MaxBab suggested - to apply an image-content-source-policy.yamlon the cluster, which does not require the long and risky nodes restarts process:

apiVersion: operator.openshift.io/v1alpha1
kind: ImageContentSourcePolicy
metadata:
  name: brew-registry
spec:
  repositoryDigestMirrors:
  - mirrors:
    - "$BREW_REGISTRY"
    source: registry.redhat.io
  - mirrors:
    - "$BREW_REGISTRY"
    source: registry.stage.redhat.io
  - mirrors:
    - "$BREW_REGISTRY"
source: registry-proxy.engineering.redhat.com

A side note question - Will it work also if using 'SubCtl' deploy & join commands (without ACM and Submariner Addon) ?

MaxBab commented 2 years ago

Yes, it should work. The ImageContentSourcePolicy is an OCP based configuration.

It is not tied to ACM or any kind of Addon.

manosnoam commented 2 years ago

Thanks @MaxBab.

Is there any reason/advantages to keep using MachineConfig over ImageContentSourcePolicy ? I don't want to break backward compatibility (we used it before testing Submariner with ACM). @SteveMattar do you think it can influence other use-cases maybe ?

MaxBab commented 2 years ago

No, I don't see any advantage in using MachineConfig over the ImageContentSourcePolicy.

As you can see by the following link [1], images configuration is a global ocp option and does not related to acm/submariner specific usecases.

But let's heat Steve thoughts as well.

[1] - https://docs.openshift.com/container-platform/4.9/openshift_images/image-configuration.html#images-configuration-registry-mirror_image-configuration

SteveMattar commented 2 years ago

@manosnoam LGTM @MaxBab ImageContentSourcePolicy is a custom resource that uses MachineConfig behind the scene. The only advantage in using MachineConfig is when you need support for floating tags using the option mirror-by-digest-only = false . Now that Submariner supports digests we don't need MachineConfig anymore.

manosnoam commented 2 years ago

Thanks for you input @SteveMattar If ImageContentSourcePolicy is still creating MachineConfig, does it also restart the nodes ? i.e. Would I still need to wait for nodes to be ready:

https://github.com/redhat-openshift/acm-submariner-tester/blob/8bd34177193cfac20670c2ff623c598ab5a28f7f/setup_subm.sh#L3063-L3067

SteveMattar commented 2 years ago

@manosnoam Yes, it does

manosnoam commented 2 years ago

Hi @SteveMattar, @MaxBab pointed out why we still need to use the MachineConfig workaround: https://access.redhat.com/solutions/4817401

As of OCP 4, using image tags for ImageContentSourcePolicy resource is not supported. The supported and recommended approach is to use image SHA digest instead of tags for working with Operators. There is a RFE to make mirror-by-digest-only option configurable through ImageContentSourcePolicy: https://issues.redhat.com/browse/RFE-1608

So only when the RFE is implemented, we will be able to use the ImageContentSourcePolicy resource to configure this.

I will hold on with this patch for now: https://github.com/redhat-openshift/acm-submariner-tester/pull/29

SteveMattar commented 2 years ago

Hi @manosnoam this PR https://github.com/submariner-io/submariner-operator/pull/1702 supports Related images with digests I merged it U/S and @dfarrell07 applied the patch in D/S as well. You don't need to use floating tags anymore. The bundle will pull the relevant images directly from brew as long as you have the ICSP defined.

MaxBab commented 2 years ago

@SteveMattar Could you elaborate a bit, please? How would I specify the required image by digest? If I need to pull the gateway:0.110, how would I find the proper digest of that image?

SteveMattar commented 2 years ago

@MaxBab no need to pull it by yourself anymore It will be done automatically when you create the bundle subscription

MaxBab commented 2 years ago

@SteveMattar I set the startingCSV: 0.11.2 parameter as part of the usbscription config. We have a new zstream version.

I set the brew.registry.redhat.io mirroring with ICSP and CatalogSource to point to the iib image. The submariner-operator and submariner-addon pulled successfully. But other submariner components ended with the following error:

Back-off pulling image "registry.redhat.io/rhacm2-tech-preview/submariner-gateway-rhel8:v0.11.2"

SteveMattar commented 2 years ago

@MaxBab can you verify with Daniel if he applied the patch in d/s?

MaxBab commented 2 years ago

@SteveMattar Sure. Will check it. Thanks!

MaxBab commented 2 years ago

@SteveMattar

Does the definition of the quay.io in the images value within the config/manager/patches/related-images.deployment.config.yaml file will still allow us to fetch the images from brew?

SteveMattar commented 2 years ago

In d/s the url is registry.redhat.io Same as your ICSP source

MaxBab commented 2 years ago

@SteveMattar I'm looking at your presentations that you prepared regarding the upstream -> downstream flow. It looks like it should take the upstream code base on a daily basis, request a change to the gerrit, push it to a distgit and so on.

With that, is there any action required by the developer to ensure the patch is applied to the downstream or is it done automatically? Should I spoke to Andrew to ask those questions?

Thanks a lot for your help.

SteveMattar commented 2 years ago

@MaxBab it is automatically done The only change is the config file that I mentioned in the description The d/s should have a different content

MaxBab commented 2 years ago

@SteveMattar Got it.

But where the change for the downstream should be applied by the developer? I asked Stephen if your patch has been applied downstream and he told me that he has no idea what I'm talking about.

SteveMattar commented 2 years ago

Please ask Nir or Daniel

MaxBab commented 2 years ago

Sure. Thanks.

SteveMattar commented 2 years ago

The patch is part of CPaaS