enhancement : Helm Charts upgraded

[Helion55]

Description

Helm charts upgraded with,

1. Chart dependencies containing Prometheus and Loki chart which can be modified by user with --set flag during installation,
2. A separate namespace for every k8s objects of this deployment,
3. Parameterized image-name and image-version values in deployment.yaml file. This will help to update those values from automated workflow pipeline.

Issue

#1064

Proposing Further activities

1. Using a workflow to update those image values on every upgradtion.
2. Writting a description and additional instruction about this helm release in NOTES.txt
3. Also adding cert-manager as dependency chart.

[openshift-ci[bot]]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign kalmanmeth for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[openshift-ci[bot]]

Hi @Helion55. Thanks for your PR.

I'm waiting for a netobserv member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 62.49%. Comparing base (81fa61a) to head (1d7ee71). Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1433      +/-   ##
==========================================
+ Coverage   62.33%   62.49%   +0.15%     
==========================================
  Files          76       76              
  Lines       11534    11534              
==========================================
+ Hits         7190     7208      +18     
+ Misses       3884     3871      -13     
+ Partials      460      455       -5     

Flag	Coverage Δ
unittests	62.49% <ø> (+0.15%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

see 6 files with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jotak]

Thanks @Helion55 ! I'll test it! One remark is, I'm not sure about hard-coding the namespace. It's just an assumption but I think users often want to organize their namespaces as they want...

[Helion55]

As most of the k8s operators uses a default pre-defined namespace for deployment and netobserv namespace is also fitting with with the name of this application my thoughts are that it will not create any confusion for users, if they want they can change the values manually also. Otherwise we can't work with the CRDs as per now.

[Helion55]

Done the further 2 steps,

1. Using a workflow to update those image values on every upgradtion.
2. Writting a description and additional instruction about this helm release in NOTES.txt

[jotak]

Hi @Helion55 I'm getting errors with the namespace "netobserv" not found. I suspect there must be issues with the ordering of installations, e.g. if the namespace isn't created yet when other resources are installed? TBH I think that would be another reason why it's better to let helm deal with namespaces rather than forcing it.

I'm trying to understand what really brings having the crd declared in ./crds as documented here. My understanding is that helm will first install CRDs before installing anything else, but I'm not sure this is really needed in our case as we don't pre-install a FlowCollector...

[jotak]

2. Writting a description and additional instruction about this helm release in NOTES.txt

Why not using the existing README.md in ./helm ?

[jotak]

Why do we need loki & prometheus tgz embedded there? I don't think we're supposed to host them, they should come from their respective repositories

[edit] hmmm after trying without, helm complains they are missing, so I guess it's needed indeed?

[jotak]

I'm also seeing that the loki default install includes promtail, which we don't need here, we should see if that's something we can remove by config

[Helion55]

Hi @Helion55 I'm getting errors with the namespace "netobserv" not found. I suspect there must be issues with the ordering of installations, e.g. if the namespace isn't created yet when other resources are installed? TBH I think that would be another reason why it's better to let helm deal with namespaces rather than forcing it.

I have also tried one more time but it is working fine and namespace is also created automatically, I think it might be a problem releated to my pull request. can you please clone this repo and then try once. I am doing this,

1. git clone https://github.com/Helion55/network-observability-operator.git
2. kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/v1.17.2/cert-manager.yaml
3. cd network-observability-operator/helm && helm install my-netobserv .

Namespace is also created and everything is up and running.

[Helion55]

2. Writting a description and additional instruction about this helm release in NOTES.txt

Why not using the existing README.md in ./helm

Yes, no issue; we can refer to that. If anything more meaningful needs to be displayed, suggest it to me, and I will add that. Currently, I am just showing the manifest to apply and the port-forward steps, just to make things handy.

[Helion55]

Why do we need loki & prometheus tgz embedded there? I don't think we're supposed to host them, they should come from their respective repositories

[edit] hmmm after trying without, helm complains they are missing, so I guess it's needed indeed?

No, it's not mandatory to embed there. We can run, helm dependency build to install them can then execute helm install command.

[Helion55]

I'm also seeing that the loki default install includes promtail, which we don't need here, we should see if that's something we can remove by config

Alright, finding something for it.

I have tried to install loki without promtail in chart depencies with,

dependencies:
  - name: loki
    version: 5.42.0
    repository: https://grafana.github.io/helm-charts
    

but this is showing error.

[openshift-merge-robot]

PR needs rebase.

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-sigs/prow repository.

[jotak]

hey, sorry I rebased and pushed before noticing this was your main branch, I don't want to mess thing up for you, so I've rolled back my rebase.

I'm going to take a stab at these changes. One thing that will ease everything is that we don't need the conversion webhook at this point, I'll remove it from the CRD, which will also remove the need for the namespace here.

[jotak]

Thanks @Helion55, your contributions are mentioned in the release 1.9.1-community. I'm closing this PR which was replaced with https://github.com/netobserv/network-observability-operator/pull/1712