TIR - remove dynamic pod data and surface helm status #1135
Conversation
If helm is used for deployment, the components deployed by helm are no longer listed as toplevel elements in the TIR. Instead the helm release is represented by its status so that users will see tables under: Deployment Resource: <helm-release-name>-deploymentMean Deployment Resource: <helm-release-name>-deploymentStatus The DeploymentStatus has rows surfacing the releaseName, releaseRevision, namespace, deployStatus, deployDescription, lastDeployed and resources. Here an example from the resources: "Deployment: backend-helm-monorepo-chart-component-a, backend- helm-monorepo-chart-component-b, Service: backend-helm-monorepo- chart" Non-Helm deployments are surfaced as before. Further change notes: - To avoid CPS issues in Jenkins all types were using their own file and the Immutable annotation was avoided. - Type HelmStatusData captures helm status with high fidelity whereas HelmStatusSimpleData captures shallower information in particular omitting the details of resources except their names and kinds. This approach simplifies the json Helm status json parsing code while keeping the consuming code relatively lean. In the deployment artifact the simplified helmStatus is kept under key 'helmStatus' in the deploymentMean information. Which is also written to `ods-deployments.json` by DeploymentDescriptor. - PodData fields capturing dynamic data where removed, except for the podName which has a prefix useful for correlating dynamic data with pods. At the moment this is still needed for both tailor and helm deployments. The code which finds this named pods did require retry logic even for helm installs using --atomic.
henrjk
force-pushed
the
feature/tir-rm-dynamic-data-for-master
branch
from
750db34 to
449dd9e
Compare
| @@ -94,10 +95,16 @@ class HelmDeploymentStrategy extends AbstractDeploymentStrategy { | |||
|
|
|||
| logger.info "Rolling out ${context.componentId} with HELM, selector: ${options.selector}" | |||
| helmUpgrade(context.targetProject) | |||
| HelmStatusSimpleData helmStatus = openShift.helmStatus(context.targetProject, options.helmReleaseName) | |||
| def helmStatusMap = helmStatus.toMap() | |||
There was a problem hiding this comment.
This map seems to be used only for logging purposes. I would enclose the instruction that performs the log in a condition on logger.getDebugOn() and generate the map inside, so that it isn't generated unless necessary.
There was a problem hiding this comment.
Thank you - will adjust the code!
| context.targetProject, DEPLOYMENT_KINDS, options.selector | ||
| ) | ||
| logger.info("${this.class.name} -- HELM STATUS") | ||
| logger.info( |
There was a problem hiding this comment.
Logging the contents of the helm status map is detailed debug info. Please change this to logger.debug and enclose it in a condition on logger.getDebugOn() so that no processing is done, if debug mode is inactive.
| JsonOutput.prettyPrint( | ||
| JsonOutput.toJson(helmStatusMap))) | ||
|
|
||
| // not sure if we need both HELM STATUS and DEPLOYMENT RESOURCES" |
There was a problem hiding this comment.
Please, don't leave this kind of comments. Could you make a decision and remove it?
| JsonOutput.prettyPrint( | ||
| JsonOutput.toJson(helmStatusMap))) | ||
|
|
||
| // not sure if we need both HELM STATUS and DEPLOYMENT RESOURCES" | ||
| logger.info("${this.class.name} -- DEPLOYMENT RESOURCES") | ||
| logger.info( | ||
| JsonOutput.prettyPrint( |
There was a problem hiding this comment.
This also feels like debug level. Let's discuss this.
There was a problem hiding this comment.
The logging of DEPLOYMENT RESOURCES comes from master. Should it be changed?
| @@ -124,7 +132,7 @@ class HelmDeploymentStrategy extends AbstractDeploymentStrategy { | |||
| options.helmValues['componentId'] = context.componentId | |||
|
|
|||
| // we persist the original ones set from outside - here we just add ours | |||
| Map mergedHelmValues = [:] | |||
| Map mergedHelmValues = [:] as Map<String, String> | |||
There was a problem hiding this comment.
Thanks for the suggestion - I will make that change and also look out for similar cases.
| /** | ||
| * Helm releases become top level elements, tailor deployments are left alone. | ||
| */ | ||
| @SuppressWarnings('CyclomaticComplexity') |
There was a problem hiding this comment.
It is not a good practice to suppress quality warnings. Let's talk and see how we could simplify this code.
There was a problem hiding this comment.
The codeNarc report says its cyclomatic complexity of 21 where the threshold is 20. Now in https://codenarc.org/codenarc-rules-size.html the rules are stated. Adding another statement if (logger.debugMode) to not do json encoding when not logging will raise complexity by one.
Let's discuss whether there are good ideas to make this more clear.
| def object = new JsonSlurperClassic().parseText(helmStdout) | ||
| def helmStatusData = HelmStatusData.fromJsonObject(object) | ||
| helmStatusData | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
In groovy you can just catch(e), but it's better to catch concrete exceptions, if possible.
There was a problem hiding this comment.
Surrounding code in OpenShiftService has simply catch (ex) {. Not sure this is better, but at least it would be one style.
| @@ -429,7 +458,12 @@ class OpenShiftService { | |||
| ) | |||
| } | |||
|
|
|||
| // Returns data about the pods (replicas) of the deployment. | |||
| boolean isDeploymentKind(String kind) { | |||
There was a problem hiding this comment.
This feels like a private method
There was a problem hiding this comment.
It used when generating the TIR data in assembleDeployments called by org.ods.orchestration.usecase.LeVADocumentUseCase#createTIR
My rationale was that the createTIR method should not need to know what deployment kinds we support. Let's discuss more.
| // version is an integer but we map it to a string. | ||
| String version | ||
|
|
||
| @SuppressWarnings(['IfStatementBraces']) |
There was a problem hiding this comment.
Better to comply with the rules instead of suppressing warnings
There was a problem hiding this comment.
To my eyes the code looks cleaner in this particular case.
| info.resources = resourcesMap | ||
| status.info = new HelmStatusInfoData(info) | ||
| new HelmStatusData(status) | ||
| } catch (Exception e) { |
|
This PR has been replaced by PR #1143. |
This change is meant to be released with the change in opendevstack/ods-document-generation-templates#145 although there is no technical coupling at the moment.
If helm is used for deployment, the components deployed by helm are no longer listed as toplevel elements in the TIR. Instead the helm release is represented by its status so that users will see tables under:
Deployment Resource: -deploymentMean
Deployment Resource: -deploymentStatus
The DeploymentStatus has rows surfacing the releaseName, releaseRevision, namespace, deployStatus, deployDescription, lastDeployed and resources.
Here an example of the resources:
"Deployment: backend-helm-monorepo-chart-component-a, backend- helm-monorepo-chart-component-b, Service: backend-helm-monorepo-chart"
Non-Helm deployments are surfaced as before.
Further change notes:
In the deployment artifact the simplified helmStatus is kept under key 'helmStatus' in the deploymentMean information. Which is also written to
ods-deployments.jsonby DeploymentDescriptor.