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 Helm functionality #799

Merged
merged 10 commits into from
May 2, 2022
Merged

Add Helm functionality #799

merged 10 commits into from
May 2, 2022

Conversation

mamercad
Copy link
Contributor

Might help with #657; you can see how this works in my fork if you like:

❯ ./bin/helm repo add awx-operator https://mamercad.github.io/awx-operator/
"awx-operator" has been added to your repositories

❯ ./bin/helm repo update
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "awx-operator" chart repository
Update Complete. ⎈Happy Helming!⎈

❯ ./bin/helm search repo awx
NAME                            CHART VERSION   APP VERSION     DESCRIPTION                      
awx-operator/awx-operator       0.17.1          0.17.1          A Helm chart for the AWX Operator

❯ ./bin/helm install my-awx-operator awx-operator/awx-operator
NAME: my-awx-operator
LAST DEPLOYED: Thu Feb 17 22:09:05 2022
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
Helm Chart 0.17.1

❯ ./bin/helm list
NAME            NAMESPACE       REVISION        UPDATED                                 STATUS          CHART                   APP VERSION
my-awx-operator default         1               2022-02-17 22:09:05.825593 -0500 EST    deployed        awx-operator-0.17.1     0.17.1     

❯ kubectl -n awx get deploy
NAME                              READY   UP-TO-DATE   AVAILABLE   AGE
awx-operator-controller-manager   0/1     1            0           2m19s

The deployment isn't up-to-date because there's no awx-operator:0.17.1 pushed up to quay right now.

@mamercad
Copy link
Contributor Author

I forgot, this is how the usage looks:

❯ VERSION=0.17.1 make helm-chart
== KUSTOMIZE (image and namespace) ==
cd config/manager && /home/mark/src/github.com/mamercad/awx-operator/bin/kustomize edit set image controller=quay.io/ansible/awx-operator:0.17.1
cd config/default && /home/mark/src/github.com/mamercad/awx-operator/bin/kustomize edit set namespace awx
== HELM ==
cd charts && \
        /home/mark/src/github.com/mamercad/awx-operator/bin/helm create awx-operator --starter /home/mark/src/github.com/mamercad/awx-operator/.helm/starter ;\
        sed -i -e 's/version: 0.1.0/version: 0.17.1/' awx-operator/Chart.yaml ;\
        sed -i -e 's/appVersion: 0.1.0/appVersion: "0.17.1"/' awx-operator/Chart.yaml ;\
        sed -i -e 's/description: A Helm chart for Kubernetes/description: A Helm chart for the AWX Operator/' awx-operator/Chart.yaml
Creating awx-operator
apiVersion: v2
appVersion: "0.17.1"
description: A Helm chart for the AWX Operator
name: awx-operator
type: application
version: 0.17.1
== KUSTOMIZE (annotation) ==
cd config/manager && /home/mark/src/github.com/mamercad/awx-operator/bin/kustomize edit set annotation helm.sh/chart:awx-operator-0.17.1
cd config/default && /home/mark/src/github.com/mamercad/awx-operator/bin/kustomize edit set annotation helm.sh/chart:awx-operator-0.17.1
== SLICE ==
/home/mark/src/github.com/mamercad/awx-operator/bin/kustomize build config/default | \
        /home/mark/src/github.com/mamercad/awx-operator/bin/kubectl-slice --input-file=- \
                --output-dir=charts/awx-operator/templates \
                --sort-by-kind
Wrote charts/awx-operator/templates/namespace-awx.yaml -- 156 bytes.
Wrote charts/awx-operator/templates/serviceaccount-awx-operator-controller-manager.yaml -- 158 bytes.
Wrote charts/awx-operator/templates/configmap-awx-operator-awx-manager-config.yaml -- 472 bytes.
Wrote charts/awx-operator/templates/customresourcedefinition-awxbackups.awx.ansible.com.yaml -- 2685 bytes.
Wrote charts/awx-operator/templates/customresourcedefinition-awxrestores.awx.ansible.com.yaml -- 2740 bytes.
Wrote charts/awx-operator/templates/customresourcedefinition-awxs.awx.ansible.com.yaml -- 18122 bytes.
Wrote charts/awx-operator/templates/clusterrole-awx-operator-metrics-reader.yaml -- 216 bytes.
Wrote charts/awx-operator/templates/clusterrole-awx-operator-proxy-role.yaml -- 348 bytes.
Wrote charts/awx-operator/templates/clusterrolebinding-awx-operator-proxy-rolebinding.yaml -- 359 bytes.
Wrote charts/awx-operator/templates/role-awx-operator-awx-manager-role.yaml -- 1495 bytes.
Wrote charts/awx-operator/templates/role-awx-operator-leader-election-role.yaml -- 524 bytes.
Wrote charts/awx-operator/templates/rolebinding-awx-operator-awx-manager-rolebinding.yaml -- 374 bytes.
Wrote charts/awx-operator/templates/rolebinding-awx-operator-leader-election-rolebinding.yaml -- 382 bytes.
Wrote charts/awx-operator/templates/service-awx-operator-controller-manager-metrics-service.yaml -- 351 bytes.
Wrote charts/awx-operator/templates/deployment-awx-operator-controller-manager.yaml -- 1823 bytes.

Makefile Outdated Show resolved Hide resolved
Copy link

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

We'd really use this chart! Thanks for kickstarting this

@mamercad
Copy link
Contributor Author

We'd really use this chart! Thanks for kickstarting this

No worries; Helm's my preferred way right now (so, I need it, heh). Also, you'll need to push a gh-pages branch and enable Pages functionality. It's a start anyhow, we can think about doing more Helm templating down the road.

@JAORMX
Copy link

JAORMX commented Feb 22, 2022

Yeah, we use Helm all over, so I need this as well. Hopefully the community accepts this.

@mamercad
Copy link
Contributor Author

Let me update the README.md with releasing information...

@mamercad
Copy link
Contributor Author

I'll add "install using Helm" to the README.md next.

Makefile Outdated Show resolved Hide resolved
@shanemcd
Copy link
Member

Hello. Thank you for taking the time to write and submit this PR. Right now I have some concerns that would need to be addressed before we would be able to consider merging this code.

We've put in a great deal of effort to remove any hardcoded versions or rendered files in the repo, enabling us to do releases at any time by running a simple workflow. We have a relatively small team and do not have the resources to constantly update files when we do a release.

As you can see in the Makefile, we pull the version from git:

VERSION ?= $(shell git describe --tags)

And all of the resources are generated dynamically by kustomize:

@$(KUSTOMIZE) build config/default | kubectl apply -f -

Similarly to what I said in #657, maybe we could generate these files at release time.

@shanemcd shanemcd mentioned this pull request Feb 23, 2022
@mamercad
Copy link
Contributor Author

Hello. Thank you for taking the time to write and submit this PR. Right now I have some concerns that would need to be addressed before we would be able to consider merging this code.

We've put in a great deal of effort to remove any hardcoded versions or rendered files in the repo, enabling us to do releases at any time by running a simple workflow. We have a relatively small team and do not have the resources to constantly update files when we do a release.

As you can see in the Makefile, we pull the version from git:

VERSION ?= $(shell git describe --tags)

And all of the resources are generated dynamically by kustomize:

@$(KUSTOMIZE) build config/default | kubectl apply -f -

Similarly to what I said in #657, maybe we could generate these files at release time.

Sure, I can work on that.

@mamercad mamercad marked this pull request as draft February 23, 2022 16:27
@mamercad mamercad marked this pull request as ready for review February 24, 2022 00:48
@mamercad
Copy link
Contributor Author

@shanemcd I think this is closer to what you're looking for, please have another look when you have some time.

@mamercad mamercad changed the title Adding Helm functionality; creating 0.17.1 Adding Helm functionality (create and publish) Feb 24, 2022
@mamercad
Copy link
Contributor Author

Using my fork; after pushing 0.17.3, this workflow , followed by this workflow ran. The release is created and can be viewed here. Lastly, after updating my Helm repo, I can find the chart:

❯ helm search repo awx-operator
NAME                            CHART VERSION   APP VERSION     DESCRIPTION                      
awx-operator/awx-operator       0.17.3          0.17.3          A Helm chart for the AWX Operator

@pat-s
Copy link

pat-s commented Mar 4, 2022

Thanks for this!

I've tried to install it but saw the following

Error: INSTALLATION FAILED: rendered manifests contain a resource that already exists. Unable to continue with install: Namespace "awx" in namespace "" exists and cannot be imported into the current release: invalid ownership metadata; label validation error: missing key "app.kubernetes.io/managed-by": must be set to "Helm"; annotation validation error: missing key "meta.helm.sh/release-name": must be set to "awx"; annotation validation error: missing key "meta.helm.sh/release-namespace": must be set to "default"

Web search indicates that this error is quite generic and hence I am not sure if it really relates to the helm chart. Wanted to post it anyway...

@mamercad
Copy link
Contributor Author

mamercad commented Mar 5, 2022

Thanks for this!

I've tried to install it but saw the following

Error: INSTALLATION FAILED: rendered manifests contain a resource that already exists. Unable to continue with install: Namespace "awx" in namespace "" exists and cannot be imported into the current release: invalid ownership metadata; label validation error: missing key "app.kubernetes.io/managed-by": must be set to "Helm"; annotation validation error: missing key "meta.helm.sh/release-name": must be set to "awx"; annotation validation error: missing key "meta.helm.sh/release-namespace": must be set to "default"

Web search indicates that this error is quite generic and hence I am not sure if it really relates to the helm chart. Wanted to post it anyway...

Seems like you might have either AWX or the AWX Operator already deployed? I just tried this again, here's a gist.

@shanemcd
Copy link
Member

shanemcd commented Mar 5, 2022

This is looking pretty great @mamercad. Thank you for all your work here. Apologies for being slow to respond, we've been been busy lately. I want to test this out, but I'm on vacation most of next week so it might be the week of the 14th before I can give this a proper review.

@mamercad
Copy link
Contributor Author

mamercad commented Mar 5, 2022

This is looking pretty great @mamercad. Thank you for all your work here. Apologies for being slow to respond, we've been been busy lately. I want to test this out, but I'm on vacation most of next week so it might be the week of the 14th before I can give this a proper review.

No worries at all; thanks for taking a look -- I'm in no rush on this whatsoever, I can always use my fork if needed -- have a nice vacation!

Copy link
Member

@shanemcd shanemcd left a comment

Choose a reason for hiding this comment

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

Amazing work on this. I left some minor comments just after cloning and reading the code. I haven't yet tried it out on my fork. Will do that sometime next week. Thanks again, this is a great contribution.

.github/workflows/helm-release.yaml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@mamercad mamercad requested a review from shanemcd March 18, 2022 19:23
@mamercad
Copy link
Contributor Author

Amazing work on this. I left some minor comments just after cloning and reading the code. I haven't yet tried it out on my fork. Will do that sometime next week. Thanks again, this is a great contribution.

No worries; appreciate the review!

@shanemcd
Copy link
Member

@mamercad I just sat down to finally test this and noticed I just merged a PR that created a merge conflict. Would you mind resolving that? Or I can handle it and push to your branch. Let me know what you prefer.

@mamercad
Copy link
Contributor Author

@mamercad I just sat down to finally test this and noticed I just merged a PR that created a merge conflict. Would you mind resolving that? Or I can handle it and push to your branch. Let me know what you prefer.

I can rebase, give me a bit.

@shanemcd
Copy link
Member

I'm also seeing this:

$ make kubectl-slice

gzip: stdin: not in gzip format
tar: Child returned status 1
tar: Error is not recoverable: exiting now
make: *** [Makefile:205: kubectl-slice] Error 2

It looks like this is because on my machine ARCH is amd64 which does not map to a filename under https://github.com/patrickdappollonio/kubectl-slice/releases/tag/v1.1.0

@mamercad
Copy link
Contributor Author

Hmmm. I can, but what should that branch contain? Are there any docs I can read about the approach you're taking here? I probably should have asked that question sooner 🙂

Yep, it's using chart releaser, which operates on a branch. It's been a while since I've looked at this as well at this point, but, I think it's better to use a separate branch for GitHub Pages (versus the default branch of the project), and that by default, is a branch named gh-pages. I don't think that cr can create the branch if it doesn't exist.

@shanemcd
Copy link
Member

Looking into it now, not sure I'll figure it out before I get pulled off to something else.

After reading a bit it looks like it's expecting a clean slate with a "charts" directory. I'm going to try that first.

@shanemcd
Copy link
Member

Hope you dont mind me pushing to this branch. Found a small little problem in your Makefile changes. We probably didn't see it in GHA since their runners already have yq installed.

@mamercad
Copy link
Contributor Author

mamercad commented Apr 29, 2022

Hope you dont mind me pushing to this branch. Found a small little problem in your Makefile changes. We probably didn't see it in GHA since their runners already have yq installed.

Not at all :) -- we'll (re)figure out how this works, together, hehe.

@nickjmv
Copy link

nickjmv commented May 2, 2022

We are really looking forward to an official helm chart to use. Can I conclude that you guys are close to a helm chart installation method for the operator?

@Hokwang
Copy link

Hokwang commented May 2, 2022

@mamercad
I am the person who is looking forward to using helm chart of awx-operator and moreover I think it is ok to remove kustomize. helm chart can be replacement.

By the way, I am looking in to code, the values.yaml is empty. no one can adjust values. I think this is just wrapper,
user can run only helm install awx-operator without -f dev-values.yaml or -f prod-values.yaml.

and I think whole helm chart code should exist in codebase. In your code, chart tgz file is generated only. not easy to see diff.

@shanemcd
Copy link
Member

shanemcd commented May 2, 2022

I think it is ok to remove kustomize. helm chart can be replacement.

We can't do this because operator-sdk (and surrounding tooling like OLM) is built on top of an auto-generated kustomize-based framework. And from what I can tell it seems to be more flexible than helm.

By the way, I am looking in to code, the values.yaml is empty. no one can adjust values

This is a pretty fair complaint. I dont know much about helm, but do these values require you to use helm's templating language?

mamercad and others added 10 commits May 2, 2022 14:12
Without this I was seeing:

$ make yq
tar: yq_linux_amd64: Not found in archive
tar: Exiting with failure status due to previous errors
make: *** [Makefile:240: yq] Error 2
This was mostly me working around a limitation in chart-releaser where it does not allow for uploading a chart to an existing release.
@shanemcd
Copy link
Member

shanemcd commented May 2, 2022

@mamercad I rebased and resolved a conflict with this PR + pushed 2 new commits that:

  • adds a basic smoke test in CI
  • gets this integrated into our existing release processes

The comment from @Hokwang is valid and makes me question the overall usefulness of this, but I guess it's better than nothing and will work for the general case. We can continue to iterate on it if there are good solutions to these problems.

@shanemcd shanemcd changed the title Adding Helm functionality (create and publish) Add Helm functionality May 2, 2022
@shanemcd shanemcd merged commit bf74d5c into ansible:devel May 2, 2022
@mamercad
Copy link
Contributor Author

mamercad commented May 3, 2022

@mamercad I rebased and resolved a conflict with this PR + pushed 2 new commits that:

  • adds a basic smoke test in CI
  • gets this integrated into our existing release processes

The comment from @Hokwang is valid and makes me question the overall usefulness of this, but I guess it's better than nothing and will work for the general case. We can continue to iterate on it if there are good solutions to these problems.

Totally agree; it's just a start and a framework. If folks find it useful going forward, we can put work towards the Helm values.

@nickjmv
Copy link

nickjmv commented May 3, 2022

So right now there is no option to modify values for the helm chart? You just have to take what the chart deploys, right? Or is there a workaround for now when the upstream values.yaml file is still empty?

@mamercad
Copy link
Contributor Author

mamercad commented May 3, 2022

So right now there is no option to modify values for the helm chart? You just have to take what the chart deploys, right? Or is there a workaround for now when the upstream values.yaml file is still empty?

This was just merged; so no, there's no workaround right now. As it stands right now, it packages up the existing deployment method, i.e., kustomize build.

@mamercad mamercad deleted the helm branch May 3, 2022 10:31
@mateuszdrab
Copy link
Contributor

mateuszdrab commented May 17, 2022

@mamercad Thanks for contributing this in

Quick question, could you provide some steps to safely migrate from the kustomize deployment to helm without losing the existing instances? Would be nice if this could be added to the upgrade section of the project's readme.
I'm fearing that if I delete the kustomize deployment and associated CRDs, I will lose my instance

@mamercad
Copy link
Contributor Author

@mamercad Thanks for contributing this in

Quick question, could you provide some steps to safely migrate from the kustomize deployment to helm without losing the existing instances? Would be nice if this could be added to the upgrade section of the project's readme. I'm fearing that if I delete the kustomize deployment and associated CRDs, I will lose my instance

Unfortunately, I can't give advice on this other than "caveat emptor". If it were me, I'd definitely test this in a sandbox before production. If I can get some time, I'll try this in my homelab (but, to be honest, I've been swamped with work lately).

@mateuszdrab
Copy link
Contributor

No rush, I'll try to deploy it when I gain more 'confidence' - I think I'll just remove all resources except the crds and redeploy via helm (which should take ownership of the CRDs then)

@mamercad
Copy link
Contributor Author

No rush, I'll try to deploy it when I gain more 'confidence' - I think I'll just remove all resources except the crds and redeploy via helm (which should take ownership of the CRDs then)

Yeah, that sounds reasonable. Not sure how closely you looked -- the Helm chart is simply packaging up the result of kustomize build (after slicing up by kind).

@shanemcd shanemcd mentioned this pull request Oct 6, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants