Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable registry by default for ovirt #583

Merged
merged 1 commit into from Jul 31, 2020

Conversation

rgolangh
Copy link
Contributor

As part of the upcommig CSI Driver support a registry should be enabled by
default.
The CSI driver support is part of Cluster Storage Operator support in
external providers. See
openshift/enhancements#352

Signed-off-by: Roy Golan rgolan@redhat.com

As part of the upcommig CSI Driver support a registry should be enabled by
default.
The CSI driver support is part of Cluster Storage Operator support in
external providers. See
openshift/enhancements#352

Signed-off-by: Roy Golan <rgolan@redhat.com>
@rgolangh
Copy link
Contributor Author

rgolangh commented Jul 23, 2020

This depends on oVirt csi operator support in CSO - I'll link the relevant PR. Merging this prematurely will fail ovirt e2e.
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2020
@rgolangh
Copy link
Contributor Author

@bennyz @Gal-Zaidman @gekorob please have a look

@Gal-Zaidman
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2020
@bparees
Copy link
Contributor

bparees commented Jul 23, 2020

/assign @dmage

@@ -159,6 +158,11 @@ func GetPlatformStorage(listers *regopclient.Listers) (imageregistryv1.ImageRegi
Claim: defaults.PVCImageRegistryName,
}
replicas = 1
case configapiv1.OvirtPlatformType:
cfg.PVC = &imageregistryv1.ImageRegistryConfigStoragePVC{
Claim: defaults.PVCImageRegistryName,
Copy link
Member

Choose a reason for hiding this comment

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

When the claim name is present, the operator expects PVC to be provided.

cfg.PVC = &imageregistryv1.ImageRegistryConfigStoragePVC{
Claim: defaults.PVCImageRegistryName,
}
replicas = 1
Copy link
Member

Choose a reason for hiding this comment

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

Does oVirt storage support RWX?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No - that would be a shared disk from RHV side. Do we have other options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought replica 1 would work with RWO?

Copy link
Member

Choose a reason for hiding this comment

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

We want to have at least 2 replicas when it's possible. But in this case the operator can have only 1 replica, so it's ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. We would track this and use this case as a main driver for supporting RWX

@dmage
Copy link
Member

dmage commented Jul 23, 2020

Can you add e2e-ovirt (always_run: false, optional: true) to this repo so that we can check this change?

Gal-Zaidman pushed a commit to Gal-Zaidman/release that referenced this pull request Jul 23, 2020
This PR adds cluster-image-registry-operator e2e job
for ovirt.
requested in openshift/cluster-image-registry-operator#583

Signed-off-by: Gal-Zaidman <gzaidman@redhat.com>
@Gal-Zaidman
Copy link
Contributor

Can you add e2e-ovirt (always_run: false, optional: true) to this repo so that we can check this change?

created openshift/release#10437

@rgolangh
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot 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 30, 2020
@rgolangh
Copy link
Contributor Author

ovirt e2e won't pass because of the cluster storage operator work related to ovirt openshift/cluster-storage-operator#65
Alternatively a workaround for that e2e job may fill the gap openshift/release#10233

@dmage
Copy link
Member

dmage commented Jul 30, 2020

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmage, Gal-Zaidman, rgolangh

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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2020
@openshift-bot
Copy link
Contributor

/retest

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

6 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 7abe6ba into openshift:master Jul 31, 2020
Gal-Zaidman pushed a commit to Gal-Zaidman/release that referenced this pull request Aug 2, 2020
Before we had the CSI driver for ovirt [1] we needed to patch the image registry to use emptyDir storage.
This patch removes the call for update_image_registry function on 4.6 and above.

[1]
openshift/cluster-image-registry-operator#583
openshift/cluster-storage-operator#65

Signed-off-by: Gal-Zaidman <gzaidman@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants