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

Add oVirt and manila to CSO #65

Merged
merged 3 commits into from Jul 31, 2020

Conversation

jsafrane
Copy link
Contributor

  • oVirt is simple
  • Manila is an optional CSI driver.
    • If underlying cloud has Manila, everything is the same as for the other (mandatory) drivers
    • If there is no Manila, Manila operator reports condition Disabled: true. CSO then consumes it and reports Available: true (because there is nothing wrong in the cluster). User can get details about Manila in Storage CR.

@openshift/storage

/hold
requires openshift/csi-driver-manila-operator#44 merged first

@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 29, 2020
@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 29, 2020
@jsafrane
Copy link
Contributor Author

/test e2e-ovirt

@jsafrane
Copy link
Contributor Author

/test e2e-ovirt

labels:
name: ovirt-csi-driver-operator
spec:
serviceAccountName: ovirt-csi-driver-operator

Choose a reason for hiding this comment

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

07_deployment.yaml was just updated, please pull it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. And added CredentialRequest sync for oVirt to CSO.

@jsafrane jsafrane force-pushed the add-ovirt-manila branch 2 times, most recently from a5b8516 to f06cd58 Compare July 30, 2020 11:43
@jsafrane
Copy link
Contributor Author

/test e2e-ovirt

@jsafrane
Copy link
Contributor Author

/test e2e-ovirt

1 similar comment
@jsafrane
Copy link
Contributor Author

/test e2e-ovirt

operator: Exists
args:
- start
- "--node=$(KUBE_NODE_NAME)"
Copy link
Member

Choose a reason for hiding this comment

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

What about adding ${LOG_LEVEL}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@jsafrane
Copy link
Contributor Author

/test e2e-ovirt

2 similar comments
@rgolangh
Copy link

/test e2e-ovirt

@jsafrane
Copy link
Contributor Author

/test e2e-ovirt

@openshift-ci-robot
Copy link
Contributor

@jsafrane: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-ovirt 7f1f9b9 link /test e2e-ovirt

Full PR test history. Your PR dashboard.

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.

degradedCnd.Type = c.crConditionName(operatorapi.OperatorStatusTypeDegraded)
if degradedCnd.Status == operatorapi.ConditionUnknown {
degradedCnd.Status = operatorapi.ConditionFalse
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we have an operator that's disabled&optional but is also progressing or degraded?

Doesn't sound like we want to handle that, but just checking...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disabled & degraded can happen:

  1. Manila operator checks there is no Manila in OpenStack and sets Disabled: true
  2. On regular re-sync, Manila fails to talk to OpenStack and reports Degraded: true. This keeps Disabled: true (at least I think so)

With the current code, CSO will report Degraded: true.

Disabled & Progressing is IMO impossible, but CSO should be robust enough to handle that.

DeploymentAsset: "csidriveroperators/ovirt/07_deployment.yaml",
ImageReplacer: strings.NewReplacer(pairs...),
ExtraControllers: []factory.Controller{
newOVirtCredentialsRequest(clients, recorder),
Copy link
Member

Choose a reason for hiding this comment

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

Why are we creating the credentials request in CSO for oVirt?

oVirt operator creates it as well: https://github.com/openshift/ovirt-csi-driver-operator/blob/master/pkg/operator/starter.go#L86

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oVirt operator needs to check name of storage domain from oVirt to create the default storage class.

Copy link
Member

Choose a reason for hiding this comment

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

Ack.

@rgolangh does that mean the oVirt operator doesn't need to create the credentialsrequestcontroller [1]?

[1] https://github.com/openshift/ovirt-csi-driver-operator/blob/master/pkg/operator/starter.go#L86

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we could fix oVirt operator to reuse secret provided by CSO's CredentialRequest.

@@ -240,6 +221,61 @@ func (c *CSIDriverOperatorCRController) applyClusterCSIDriver(requiredOriginal *
return actual, true, err
}

func (c *CSIDriverOperatorCRController) syncConditions(conditions []operatorapi.OperatorCondition, updatefn v1helpers.UpdateStatusFunc) error {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it would be more readable to split this into 2 functions: one to handle optional operators and the other to handle non-optional ones? Or have a big if-else clause (if c.optional)?

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 tried and it looks even worse - two functions that look like copies, with only small differences.

@jsafrane
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 31, 2020
Copy link
Member

@bertinatto bertinatto left a comment

Choose a reason for hiding this comment

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

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, jsafrane

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 [bertinatto,jsafrane]

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

@jsafrane
Copy link
Contributor Author

jsafrane commented Jul 31, 2020

The last ovirt tests almost succeeded. Only this one failed:

[sig-instrumentation] Prometheus when installed on the cluster shouldn't report any alerts in firing state apart from Watchdog and AlertmanagerReceiversNotConfigured [Early] [Suite:openshift/conformance/parallel] . And the alert do not look storage related (memory overcommit on nodes and KubePodCrashLooping in ovirt-infra)

@openshift-merge-robot openshift-merge-robot merged commit 46465ca 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>
Gal-Zaidman pushed a commit to Gal-Zaidman/cluster-image-registry-operator that referenced this pull request Aug 3, 2020
When the csi driver for ovirt was merged into the storage operator[1] there is no "standard" StorageClass in the cluster, but there is a ovirt-csi-sc storage class which is the default storage class.
This cause the installer to fail due to image registry not being ready, since it was panding due to the pending pvc.
Instead the default storageclass should be used, see thread on forum-storage[2], not specifing the storage class[3] catches the default storage class.

[1] openshift/cluster-storage-operator#65
[2] https://coreos.slack.com/archives/CBQHQFU0N/p1596452181198600
[3] https://kubernetes.io/docs/concepts/storage/persistent-volumes/#class-1

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

5 participants