controller: create storage configmap on reconcile #227

danielerez · 2019-08-20T07:04:25Z

Added a new ConfigMap for defining default values for
storage according to the following logic:

For a BareMetal infrastructure:
- volumeMode: Block
- accessMode: ReadWriteMany
For all other deployments:
- volumeMode: File
- accessMode: ReadWriteMany

danielerez · 2019-08-20T07:07:46Z

@rthallisey @mareklibra @aglitke @awels @jelkosz @rollandf - please review

mareklibra

This patch is sort of hardcoding the defaults, no matter a ConfigMap is used.

Is automatic detection of StorageClass capabilities still in plan?
The ConfigMap should contain sc-specific defaults if these generic (Filesystem/RWM) can not be used (like for the local storage).

There is no way to hardcode the defaults right for a generic cluster, as recently implemented here.
If detection of SC capabilities will not land shortly, there should be a way how the administrator can change values in this ConfigMap. Via HCO configuration or manually while not-breaking the reconciliation logic.

docs/configmaps.md

mareklibra · 2019-08-20T07:43:23Z

pkg/controller/hyperconverged/hyperconverged_controller.go

+			Namespace: namespace,
+		},
+		Data: map[string]string{
+			"accessMode": "ReadWriteMany",


With this set, no VM created by UI (Wizard) can be started when local SC is used.

ok, so I'll add also local-sc.accessMode and local-sc.volumeMode then.

docs/configmaps.md

pkg/controller/hyperconverged/hyperconverged_controller.go

danielerez · 2019-08-20T18:57:03Z

This patch is sort of hardcoding the defaults, no matter a ConfigMap is used.

Is automatic detection of StorageClass capabilities still in plan?
The ConfigMap should contain sc-specific defaults if these generic (Filesystem/RWM) can not be used (like for the local storage).

There is no way to hardcode the defaults right for a generic cluster, as recently implemented here.
If detection of SC capabilities will not land shortly, there should be a way how the administrator can change values in this ConfigMap. Via HCO configuration or manually while not-breaking the reconciliation logic.

In 'ensureKubeVirtStorageConfig' func we create the ConfigMap only if doesn't exist, so admin should be able to modify it.

djzager · 2019-08-21T13:09:00Z

I would rather the user set a RHHI or Baremetal flag instead of us auto detecting. You can add the flag here.

I'm curious @rthallisey why we are against auto detecting. Originally I would have thought we would want to auto detect where possible.

pkg/apis/hco/v1alpha1/hyperconverged_types.go

rthallisey · 2019-08-21T20:09:25Z

I'm curious @rthallisey why we are against auto detecting. Originally I would have thought we would want to auto detect where possible.

My thinking is that I want to avoid having code that checks for OpenShift specific things in tree wherever possible. Having a flag for the user to set isn't too bad of a trade-off IMO.

djzager · 2019-08-22T13:52:35Z

My thinking is that I want to avoid having code that checks for OpenShift specific things in tree wherever possible. Having a flag for the user to set isn't too bad of a trade-off IMO.

Interesting, if I may offer a different perspective. I would suggest that "we", as the maintainers of the HCO, know best how to install all of the different KubeVirt components and the HyperConverged resource is our API. Every field in our HyperConverged's spec, we accept all of the supported values and support them -- consider a memcached-operator that accepts a version (but only 1, 2, 3, 4) then a user would expect to be able to put any of 1-4 in the spec.version (there could be issues where downgrades are not allowed but you get the idea).

In our current scenario, we have no way of supporting BareMetalPlatform on anything but a bare metal platform...so we would need to have some validation on that field before doing work anyway. We could simply detect the appropriate APIs before doing work without asking the user for anything...that would prevent the user from mucking around with fields and values that we never intend to support (like BareMetalPlatform: true on a non-baremetal cluster).

danielerez · 2019-08-22T14:08:49Z

My thinking is that I want to avoid having code that checks for OpenShift specific things in tree wherever possible. Having a flag for the user to set isn't too bad of a trade-off IMO.

Interesting, if I may offer a different perspective. I would suggest that "we", as the maintainers of the HCO, know best how to install all of the different KubeVirt components and the HyperConverged resource is our API. Every field in our HyperConverged's spec, we accept all of the supported values and support them -- consider a memcached-operator that accepts a version (but only 1, 2, 3, 4) then a user would expect to be able to put any of 1-4 in the spec.version (there could be issues where downgrades are not allowed but you get the idea).

In our current scenario, we have no way of supporting BareMetalPlatform on anything but a bare metal platform...so we would need to have some validation on that field before doing work anyway. We could simply detect the appropriate APIs before doing work without asking the user for anything...that would prevent the user from mucking around with fields and values that we never intend to support (like BareMetalPlatform: true on a non-baremetal cluster).

@rthallisey So what's your take on that then? Should we keep the flag or revert to the previous solution (detect automatically by fetching the infrastructure's platform).

mareklibra · 2019-08-23T07:10:48Z

pkg/controller/hyperconverged/hyperconverged_controller.go

+		Data: map[string]string{
+			"accessMode":          "ReadWriteMany",
+			"volumeMode":          volumeMode,
+			"local-sc.accessMode": "ReadWriteOnce",


@danielerez , this assumes that the target cluster has a storage class called local-sc. It's up to cluster's admin how (s)he calls it and what particular SC is behind that name.

We can either

autodetect

or provide a way for manual intervention

The admin can change it by modifying the ConfigMap (reconcile won't change it once already created). @rthallisey do we want to add the local class name to HyperConvergedSpec as well or is it too specific?

@mareklibra @rthallisey Added 'LocalStorageClassName' key to HyperConverged spec, so it could be configured on deployment.

rthallisey · 2019-08-26T21:54:57Z

Interesting, if I may offer a different perspective. I would suggest that "we", as the maintainers of the HCO, know best how to install all of the different KubeVirt components and the HyperConverged resource is our API. Every field in our HyperConverged's spec, we accept all of the supported values and support them -- consider a memcached-operator that accepts a version (but only 1, 2, 3, 4) then a user would expect to be able to put any of 1-4 in the spec.version (there could be issues where downgrades are not allowed but you get the idea).

In our current scenario, we have no way of supporting BareMetalPlatform on anything but a bare metal platform...so we would need to have some validation on that field before doing work anyway. We could simply detect the appropriate APIs before doing work without asking the user for anything...that would prevent the user from mucking around with fields and values that we never intend to support (like BareMetalPlatform: true on a non-baremetal cluster).

I understand the argument of "we can detect the configuration so we should", but I'm concerned about the cost of this UX. If we consume an openshift API apiconfigv1.Infrastructure{}, I think it 1) limits our adoption in the community 2) opens the door to making more assumptions about the underlying platform. We are the HCO maintainers and we understand how to deliver the HCO, but we don't provision the cluster or the infra. I think we're reaching a little for a small gain in UX. For example, I could see infra detection like this added to the OCS operator because it is an infra-operator.

I do think there is an argument to be made about adding something to our API. Adding this option requires we support and CRD changes in the future. It is a True/False bool so I don't think it will be hard, but No API is the best API.

rthallisey · 2019-08-27T14:54:17Z

ci test please

djzager · 2019-08-27T15:17:06Z

I understand the argument of "we can detect the configuration so we should", but I'm concerned about the cost of this UX. If we consume an openshift API apiconfigv1.Infrastructure{}, I think it 1) limits our adoption in the community 2) opens the door to making more assumptions about the underlying platform.

I think we should only be attempting to consume APIs that are available. That is, if some API (like apiconfigv1.Infrastructure{}) is available, sure then use it or make some other decisions based on that information but I agree with you that we absolutely should not assume it's there (or require it). Consider the hypothetical scenario where we don't add a bunch of cluster networking components to a cluster but make decisions based on what is available...this to me seems like something we would want to be able to handle without exposing a new field on the HyperConverged resource.

I am in absolute agreement with you that we should not be making assumptions about the underlying platform or requiring some special OpenShift API, but I think it is correct for us to discover the APIs we want, handling their presence or absence and I don't believe this to be unique to what is now only OpenShift APIs (in the future I see it being some special API provided by another operator that we don't want to require).

danielerez · 2019-08-28T11:06:23Z

/test hco-e2e-aws

rthallisey · 2019-08-28T17:34:26Z

/retest

rthallisey · 2019-08-28T17:36:57Z

@djzager and I talked offline. I'm advocating we don't include any openshift libraries or code. Specifically, that means nothing that ties us down to having to ask ourselves "if OpenShift, do this. Otherwise, do that". Having a flag with no validation is the safest way to do this, so we're going with a flag.

rthallisey

One last nit then this LG2M

rthallisey · 2019-08-28T20:09:08Z

pkg/apis/hco/v1alpha1/hyperconverged_types.go

@@ -17,6 +17,12 @@ type HyperConvergedSpec struct {
 	// INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
 	// Important: Run "operator-sdk generate k8s" to regenerate code after modifying this file
 	// Add custom validation using kubebuilder tags: https://book.kubebuilder.io/beyond_basics/generating_crd.html
+
+	// BareMetalPlatform indicates whether the infrastructure is baremetal.
+	BareMetalPlatform bool `json:"BareMetalPlatform,omitempty"`


We should expose this flag on the HCO's CR by default. Add to the template CSV then run /hack/build-manifests.sh

--- a/templates/olm-catalog/kubevirt-hyperconverged/VERSION/kubevirt-hyperconverged-operator.VERSION.clusterserviceversion.yaml.in +++ b/templates/olm-catalog/kubevirt-hyperconverged/VERSION/kubevirt-hyperconverged-operator.VERSION.clusterserviceversion.yaml.in @@ -10,6 +10,9 @@ metadata: "metadata": { "name": "kubevirt-hyperconverged", "namespace": "kubevirt-hyperconverged" + }, + "spec": { + "BareMetalPlatform": "false", }{{if .Converged}} }, {

rthallisey · 2019-08-28T20:11:17Z

pkg/apis/hco/v1alpha1/hyperconverged_types.go

+	BareMetalPlatform bool `json:"BareMetalPlatform,omitempty"`
+
+	// LocalStorageClassName the name of the local storage class.
+	LocalStorageClassName string `json:"LocalStorageClassName,omitempty"`


We'll leave LocalStorageClassName off the default CR. I think this string should be exposed primarily in the docs.

rthallisey · 2019-08-28T20:12:48Z

@danielerez Let's try to get this in tomorrow.

Added a new ConfigMap for defining default values for storage according to the following logic: * For a BareMetal infrastructure: - volumeMode: Block - accessMode: ReadWriteMany * For all other deployments: - volumeMode: File - accessMode: ReadWriteMany Change-Id: Iea3cc601c66970c86ea21575c38db0487a18228f Signed-off-by: Daniel Erez <derez@redhat.com>

danielerez force-pushed the storage-configmap branch from e0b82ba to ee89949 Compare August 20, 2019 07:21

mareklibra reviewed Aug 20, 2019

View reviewed changes

danielerez changed the title ~~controller: create storage class configmap on reconcile~~ controller: create storage configmap on reconcile Aug 20, 2019

rthallisey requested changes Aug 20, 2019

View reviewed changes

docs/configmaps.md Outdated Show resolved Hide resolved

pkg/controller/hyperconverged/hyperconverged_controller.go Outdated Show resolved Hide resolved

danielerez force-pushed the storage-configmap branch from ee89949 to 2a87508 Compare August 20, 2019 18:51

danielerez force-pushed the storage-configmap branch 2 times, most recently from db8e437 to 86f8412 Compare August 20, 2019 21:31

rthallisey reviewed Aug 21, 2019

View reviewed changes

pkg/apis/hco/v1alpha1/hyperconverged_types.go Outdated Show resolved Hide resolved

danielerez force-pushed the storage-configmap branch 2 times, most recently from fe23e75 to 2cd7d8c Compare August 21, 2019 16:03

mareklibra reviewed Aug 23, 2019

View reviewed changes

danielerez force-pushed the storage-configmap branch 7 times, most recently from 7194c83 to 21bd5db Compare August 27, 2019 20:55

rthallisey reviewed Aug 28, 2019

View reviewed changes

danielerez force-pushed the storage-configmap branch from 21bd5db to e0cebad Compare August 29, 2019 06:29

rthallisey approved these changes Aug 30, 2019

View reviewed changes

rthallisey merged commit 132a898 into kubevirt:master Aug 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

controller: create storage configmap on reconcile #227

controller: create storage configmap on reconcile #227

danielerez commented Aug 20, 2019 •

edited

danielerez commented Aug 20, 2019

mareklibra left a comment

mareklibra Aug 20, 2019

danielerez Aug 20, 2019

danielerez commented Aug 20, 2019

djzager commented Aug 21, 2019

rthallisey commented Aug 21, 2019 •

edited

djzager commented Aug 22, 2019 •

edited

danielerez commented Aug 22, 2019

mareklibra Aug 23, 2019

danielerez Aug 23, 2019

danielerez Aug 27, 2019

rthallisey commented Aug 26, 2019

rthallisey commented Aug 27, 2019

djzager commented Aug 27, 2019

danielerez commented Aug 28, 2019

rthallisey commented Aug 28, 2019

rthallisey commented Aug 28, 2019

rthallisey left a comment

rthallisey Aug 28, 2019

danielerez Aug 29, 2019

rthallisey Aug 28, 2019

rthallisey commented Aug 28, 2019 •

edited

controller: create storage configmap on reconcile #227

controller: create storage configmap on reconcile #227

Conversation

danielerez commented Aug 20, 2019 • edited

danielerez commented Aug 20, 2019

mareklibra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielerez commented Aug 20, 2019

djzager commented Aug 21, 2019

rthallisey commented Aug 21, 2019 • edited

djzager commented Aug 22, 2019 • edited

danielerez commented Aug 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rthallisey commented Aug 26, 2019

rthallisey commented Aug 27, 2019

djzager commented Aug 27, 2019

danielerez commented Aug 28, 2019

rthallisey commented Aug 28, 2019

rthallisey commented Aug 28, 2019

rthallisey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rthallisey commented Aug 28, 2019 • edited

danielerez commented Aug 20, 2019 •

edited

rthallisey commented Aug 21, 2019 •

edited

djzager commented Aug 22, 2019 •

edited

rthallisey commented Aug 28, 2019 •

edited