Spike: Investigate what is needed for registry operator and helm chart consistency #1193
Comments
openshift-ci
bot
added
the
area/registry
|
blocked by #1028 changes to the deployment setup. |
|
#1211 is blocking this issue from starting and will be no longer be a blocker once the pre-release is cut. |
michael-valdron
changed the title
|
Updated acceptance criteria to include additional PR acceptance criteria for the operator, setting for next refinement call. |
|
Further refinement is needed between this issue and #1260. |
|
Deciding to convert this into a spike story, an investigation to what needs to be changed between the helm chart and the registry operator. Investigation will cover all the area currently documented as the acceptance criteria. |
michael-valdron
changed the title
|
#1028 is closed and so this issue is unblocked now. |
Conversion is completed. |
michael-valdron
added
the
severity/blocker
maysunfaisal
changed the title
Deployment SpecificationPod Level securityContext The devfile registry operator sets the UIDs and non root flag at the pod level if storage is enabled: https://github.com/devfile/registry-operator/blob/f55ab8e7c8061934547c7d55bd67d81d509a8752/pkg/registry/deployment.go#L294 Index ServerstartupProbe Timeouts on the helm chart should be updated to the timeouts on the registry operator: initialDelaySeconds: 30
periodSeconds: 10
timeoutSeconds: 20resources Helm chart uses The registry operator should do this as well. Registry ViewerlivenessProbe Timeouts on the helm chart should be updated to the timeouts on the registry operator: initialDelaySeconds: 15
periodSeconds: 10
timeoutSeconds: 20readinessProbe Timeouts on the helm chart should be updated to the timeouts on the registry operator: initialDelaySeconds: 15
periodSeconds: 10
timeoutSeconds: 20volumeMounts The registry operator no longer uses a mounted resources Helm chart uses The registry operator should do this as well. OCI Registryresources Helm chart uses The registry operator should do this as well. |
Ingress SpecificationHelm chart uses full hostname with the ingress domain to define the host of the ingress: https://github.com/devfile/registry-support/blob/f66264f22310c3c93bf0c77db5f4cd4b560aba5e/deploy/chart/devfile-registry/templates/ingress.yaml#L29 Registry operator uses CR field This would result in a difference in host address between the registry operator and the helm chart when deploying to a k8s cluster. |
Persistent Volume Claim (PVC) SpecificationRegistry operator sets a Helm chart does not set a Namespace should be set on both. |
DevfileRegistry CRD Fields & Helm Chart Value ParametersDevfile Registry Container Specs - Missing MemoryLimit The Devfile Registry Container Specs - Image Fields Registry operator Helm chart has two fields, Consistency should be added between these two, either handle the whole image string or handle the two parts in two fields. Missing ingress class name The registry operator is missing a field to set the ingress class name https://github.com/devfile/registry-operator/blob/f55ab8e7c8061934547c7d55bd67d81d509a8752/api/v1alpha1/devfileregistry_types.go#L117, which is what the helm chart has: https://github.com/devfile/registry-support/blob/f66264f22310c3c93bf0c77db5f4cd4b560aba5e/deploy/chart/devfile-registry/values.yaml#L18 |
CSV & Chart Definition DifferencesOutdated Maintainers List The list of maintainers under the Missing Fields The helm chart Chart.yaml is missing the following fields to sync with the registry operator CSV:
Incorrect Home Field
This is included as one of the entries under |
Hostname SetupsHostname FormatAs mentioned in #1193 (comment), the registry operator hostname setup only uses the CR name value for the hostname part, for openshift deployments and how the helm chart sets up the hostname _template.tpl#L16, the CR name is coupled with the namespace name to create the hostname. The registry operator source should be changed to something of the following: GetDevfileRegistryIngress func GetDevfileRegistryIngress(cr *registryv1alpha1.DevfileRegistry) string {
return GetHostname(cr) + "." + cr.Spec.K8s.IngressDomain
}GetHostname func GetHostname(cr *registryv1alpha1.DevfileRegistry) string {
return fmt.Sprintf("%s-%s", cr.Name, cr.Namespace)
}Hostname OverrideUsing the helm chart, one can use For example, the following will set the ingress address for the devfile registry deployment to helm install devfile-registry deploy/chart/devfile-registry ... --set hostnameOverride='dr.192.168.x.x.nip.io' ...Currently, the registry operator does not have this feature. |
RequirementsPrerequisites for the helm chart should be revised to better sync with the requirements for the registry operator along with a commonly shared CLUSTER_SUPPORT.md document. |
App Name SetupsFull App NameThe registry operator currently only has an app name that is used across the operator specifications: https://github.com/devfile/registry-operator/blob/f55ab8e7c8061934547c7d55bd67d81d509a8752/pkg/registry/naming.go The helm chart uses both an app name and a fully qualified app name which a combo of the app name and release name by default: https://github.com/devfile/registry-support/blob/f66264f22310c3c93bf0c77db5f4cd4b560aba5e/deploy/chart/devfile-registry/templates/_template.tpl#L20-L39 The registry operator should implement a fully qualified app name as well and use this field in the same places as the helm chart does. App Name OverridesUsing the helm chart, one can use For example, the following will set the app name for the devfile registry deployment to helm install devfile-registry deploy/chart/devfile-registry ... --set nameOverride='dr' --set fullnameOverride='devfile-registry' ...Currently, the registry operator does not have this feature. Name TruncatingWhen setting the app names, the helm chart truncates strings used for the names to be 63 characters in length:
This is because some Kubernetes name fields are limited to this (by the DNS naming spec). Currently, the registry operator does not truncate the app name to suite this limitation. Deployment SpecificationThe helm chart will use app name for the The full app name is also used for the following:
Configmap SpecificationThe helm chart will use app name for the Service SpecificationThe helm chart will use app name for the Persistent Volume Claim (PVC) SpecificationThe helm chart will use app name for the Ingress SpecificationThe helm chart will use app name for the The full app name is also used for the following: Route SpecificationThe helm chart will use app name for the The full app name is also used for the following: |
|
Full findings document is accessible here: https://gist.github.com/michael-valdron/5d8ec297a93e7c8cd1691c4842332529 This document is under review. |
|
Seeing the wide range of changes needed to be made, it might be best to break these down into different issues and convert #1274 into an epic. |
|
Review has concluded, findings looks good for next steps, opting to convert #1274 into an epic due to the scope of the changes and setting milestones to suite bigger feature changes needed.
|
Which area/kind this issue is related to?
/area registry
Issue Description
The setup for the devfile registry helm chart and devfile registry operator are currently inconsistent. We need to ensure that the registry helm chart and registry operator setups are in sync before we can provide official releases
#410.First, we need to investigate what we need to change to ensure consistency of these setups.
#1028 is a bug which only exists in the registry operator and needs to be fixed in order to close this issue.
Related to #1260.Outcome
To obtain details needed for implementing the changes needed to make the registry operator and registry helm chart consistent #1274.
Acceptance Criteria
DevfileRegistryCR and helm chart parametersChart.yamlThe text was updated successfully, but these errors were encountered: