Remove `yamlgen`. Refactor deployment and yaml generation to use Helm templating #350

jpmcb · 2022-12-01T17:39:47Z

Issue number:

Closes #126

Description of changes:

Refactors deployment and yaml templating to use Helm. Still a work in progress. Using this draft PR as a staging area for early feedback

Testing done:

Able to:

make images
helm install brupop-crd deploy/charts/bottlerocket-shadow
helm install brupop deploy/charts/bottlerocket-update-operator

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

jpmcb · 2022-12-15T16:42:12Z

Force pushed to update certificate resources that changed with #340 - Ran through successful updates using:

$ helm install brupop-crd --create-namespace deploy/charts/bottlerocket-shadow
$ helm install brupop deploy/charts/bottlerocket-update-operator

jpmcb · 2022-12-20T16:36:24Z

Force pushed:

fixed build errors
resolved merge conflict (with Cargo.lock update)
Updated READMEs with documentation on how to install brupop via helm
added make manifest target that can be used to generate manifest at root of repository
Removed .env file (in favor of helm values yaml file)
Added logic to create / delete brupop namespace in integration tests

Marking this as ready for review.

There are no big things left to do here and I'd love to start getting some feedback from the community. I'm sure there are little things left here since this is a bigger refactor, but we can continue to tackle those in the coming weeks!

We'll also want to think about doing a one off change to the new release workflow to ensure that works for releases. Aka, we can change that to trigger on each push to develop and ensure it's working properly. That workflow is designed to push a helm repo to a github page hosted in this repository. We'll also need to ensure we have the right access right associated with that so it can be created on release automatically.

stmcginnis

Looks good to me. There are a lot of changes here, but with the noted testing done I feel comfortable with the change.

Looks like there is a merge conflict to be resolved now. Since an update is needed anyway, a couple small things pointed out.

deploy/charts/bottlerocket-shadow/templates/NOTES.txt

README.md

deploy/charts/bottlerocket-update-operator/templates/NOTES.txt

README.md

cbgbt · 2023-01-09T17:56:46Z

deploy/build.rs

+const DEPLOY_DIR: &str = env!("CARGO_MANIFEST_DIR");
+
+fn main() {
+    dotenv::dotenv().ok();


Since we're still using dotenv as a development tool, should we leave the .env file around with some comments for what it can do?

Hmmmm that's a good point - I think this means we'll have to yes. Let me circle back and look at that.

The new workflow should become:

Create a values.yaml file (maybe we can create a top level one for development purposes and git ignore it)

Populate it with necessary values. For example:

namespace: "my-custom-ns"

Pass the values yaml file to be used during install either via:

helm install -f values.yaml ... (to install to a cluster)

helm template -f values.yaml ... > file.yaml (to template into the yaml)

I think there's some better dev UX to tease out here, especially with how the integration tests work into this flow: currently, the integration tests will just read from bottlerocket-update-operator.yaml and apply that to the cluster but there's opportunity here to make this more seamless.

deploy/charts/bottlerocket-shadow/README.md

deploy/charts/bottlerocket-shadow/templates/custom-resource-definition.yaml

deploy/charts/bottlerocket-update-operator/README.md

cbgbt · 2023-01-09T18:13:39Z

This is awesome, nice work @jpmcb! 🚀

cbgbt

Whoops, forgot to hit send on these.

integ/src/main.rs

deploy/charts/bottlerocket-shadow/templates/custom-resource-definition.yaml

deploy/charts/bottlerocket-update-operator/README.md

jpmcb · 2023-01-12T18:08:37Z

Force pushed to correct for Cargo.lock merge conflict.

Going to do another round of iteration here based on comments and a few TODO lines left.

jpmcb · 2023-01-13T18:02:07Z

Force pushed to add the following:

Snapshot testing of our generated yaml CRDs. This way, if something changes under the hood in our rust CRD definitions, we can have a signal to accept/reject those changes. This mechanism can also be used as an indicator to update the CRD templates in the helm chart.
- cargo test to run snapshot tests via insta. And cargo insta review to accept new snapshots.
- make check-crd-golden-diff to compare the rust generated CRD yaml to the helm chart template.
Remove const declaration of namespace. With the helm chart, removes the requirement that users deploy into the bottlerocket-aws namespace we had hard coded. Now, we infer the current in cluster namespace from configs.
Update docs based on feedback

Integration tests still looking 👍🏼

❯ k get bottlerocketshadows -A
NAMESPACE                 NAME                                                STATE   VERSION   TARGET STATE   TARGET VERSION   CRASH COUNT
brupop-bottlerocket-aws   brs-ip-192-168-142-119.us-west-2.compute.internal   Idle    1.11.1    Idle                            0
brupop-bottlerocket-aws   brs-ip-192-168-144-207.us-west-2.compute.internal   Idle    1.11.1    Idle                            0
brupop-bottlerocket-aws   brs-ip-192-168-156-62.us-west-2.compute.internal    Idle    1.11.1    Idle                            0

jpmcb · 2023-01-18T01:29:07Z

Force pushed to template a few remaining references to brupop-bottlerocket-aws - getting close!!

.github/workflows/deploy-helm-charts.yaml

cbgbt

Looks good! Just a few minor changes that I'd like to see and a few nits that I could take or leave.

README.md

agent/src/agentclient.rs

apiserver/src/client/webclient.rs

controller/src/main.rs

deploy/charts/bottlerocket-shadow/templates/custom-resource-definition.yaml

deploy/charts/bottlerocket-update-operator/README.md

deploy/src/test.rs

jpmcb · 2023-02-14T20:21:52Z

Force pushed to address merge conflicts - addressing comments next!

jpmcb · 2023-02-14T23:56:38Z

Force pushed to address above comments

jpmcb · 2023-02-17T21:27:58Z

Force push to address Sean's comments around infering the namespace throughout. Now, when the various client configs are created, we pass down the namespace into subsequent constructors that require namespaced resources. It didn't make sense to have to create a new client config with each query to namespaced resources. That would be a frequent, costly operation. Instead, we can now access the namespace data member built from new.

cbgbt

Super clean! Nice work John! 🚀 🎉

webern

About 1/3 coverage from me. A few questions of ignorance on my part. Looks good.

Makefile

webern · 2023-02-23T17:20:47Z

integ/src/main.rs

@@ -217,11 +217,14 @@ async fn run() -> Result<()> {
            )
            .await
            .context(error::LoadKubeConfigSnafu)?;
+
+            // infer the default namespace from the loaded kubeconfig
+            let namespace = config.default_namespace.to_string();


If the user can configure the namespace of their choice, how is it that you get that from the kubeconfig?

Good question: the config.default_namespace property is the namespace defined by the loaded client config. This is typically the namespace of where the execution is currently happening.

So, in the case of the controller, it reads the in-cluster config and infers that it is in the "bottlerocket-aws" (or whatever the namespace was set during deploy).

This case in the integration tests is abit different in that we aren't inferring a in-cluster client config, we load one explicitly from eksctl. But the principle is the same - it infers the default namespace to deploy some stuff to (and in this case, it is actually "default")

It's abit confusing because the name "default" is overloaded. Most of the time, from the controllers perspective, we are simply attempting to infer what namespace the current context is executing from.

models/src/constants.rs

deploy/tests/snapshots/generated_crds__generated_crds.snap

jpmcb · 2023-02-24T01:27:01Z

Force pushed to rename the insta Rust test file in order to be abit more clear what's happening. The name of this file should now be insta_test__generated_crds.snap which is the <test-filename>__<test-name>.snap

FYI - this will also need a hefty rebase and abit of additonal legwork to get in once the cron PR merges

nalshamaajc · 2023-04-18T17:36:59Z

I hope this helm chart gets ready soon! 🙏 😄

lsobowalejc · 2023-04-18T17:39:04Z

Yes! this is definitely a needed feature. Can't wait!

maxres-ch · 2023-05-23T22:22:43Z

Hi there. Just wanted to check if there's any insight on when this might be merged?

jpmcb · 2023-06-01T18:39:26Z

Force pushed with large rebase (which included integrating the work for the scheduler cron) and some docs updates. The biggest thing you'll notice is different is the templates now include the scheduler_cron_expression template value.

diff --git a/bottlerocket-update-operator.yaml b/bottlerocket-update-operator.yaml
index cf982ec..a96c65a 100644
--- a/bottlerocket-update-operator.yaml
+++ b/bottlerocket-update-operator.yaml
@@ -711,10 +711,8 @@ spec:
                   fieldPath: spec.nodeName
             - name: MAX_CONCURRENT_UPDATE
               value: "1"
-            - name: UPDATE_WINDOW_START
-              value: "0:0:0"
-            - name: UPDATE_WINDOW_STOP
-              value: "0:0:0"
+            - name: SCHEDULER_CRON_EXPRESSION
+              value: "* * * * * * *"
           image: public.ecr.aws/bottlerocket/bottlerocket-update-operator:v1.0.0
           name: brupop
       priorityClassName: brupop-controller-high-priority

Testing

Deployed new nodegroup to a k8s cluster
Installed shadow CRD successfully:

$ helm install brupop-crd deploy/charts/bottlerocket-shadow
...

$ kubectl get crds
NAME                                          CREATED AT
bottlerocketshadows.brupop.bottlerocket.aws   2023-06-01T18:24:49Z
...

Installed the controller stack:

$ kubectl create namespace brupop-bottlerocket-aws
$ helm install brupop deploy/charts/bottlerocket-update-operator
...

Nodes update as expected.

This should be ready to go in now

gthao313 · 2023-06-01T18:47:52Z

Follow the instruction above, it works perfectly on my side! nice work!!!

.github/workflows/deploy-helm-charts.yaml

stmcginnis

Overall looks good, and I think it would be great to get this in and available to get more usages/feedback.

deploy/charts/bottlerocket-update-operator/README.md

stmcginnis

Thanks! Looks like the checks might just need a re-run to get by a transient error.

- helm install brupop-crd --create-namespace deploy/charts/bottlerocket-shadow - helm install brupop deploy/charts/bottlerocket-shadow - Able to deploy brupop to different namespaces - Removes yamlgen subprogram - Snapshot testing of rust generated CRD yaml via cargo insta Signed-off-by: John McBride <[email protected]>

jpmcb · 2023-06-07T14:47:44Z

Force pushed to rebase on develop which now has fixes for problems caused by #461

stmcginnis

Let's gooooo!! 🥳

jpmcb force-pushed the helm-refactor branch from 28a68c0 to d948d73 Compare December 15, 2022 16:39

jpmcb force-pushed the helm-refactor branch 3 times, most recently from 6425170 to 64622a9 Compare December 20, 2022 16:30

jpmcb marked this pull request as ready for review December 20, 2022 16:36

jpmcb requested review from gthao313 and cbgbt December 20, 2022 16:38

jpmcb changed the title ~~WIP: Refactor deployment and yaml generation to use Helm~~ Refactor deployment and yaml generation to use Helm Dec 21, 2022

stmcginnis reviewed Dec 23, 2022

View reviewed changes

deploy/charts/bottlerocket-shadow/templates/NOTES.txt Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

jpmcb force-pushed the helm-refactor branch from 64622a9 to 8e7d749 Compare December 23, 2022 19:07

cbgbt reviewed Jan 9, 2023

View reviewed changes

integ/src/main.rs Outdated Show resolved Hide resolved

deploy/charts/bottlerocket-shadow/templates/custom-resource-definition.yaml Show resolved Hide resolved

jpmcb commented Jan 12, 2023

View reviewed changes

deploy/charts/bottlerocket-shadow/templates/custom-resource-definition.yaml Outdated Show resolved Hide resolved

deploy/charts/bottlerocket-update-operator/README.md Show resolved Hide resolved

jpmcb force-pushed the helm-refactor branch from 8e7d749 to bc16ed4 Compare January 12, 2023 18:07

jpmcb force-pushed the helm-refactor branch from bc16ed4 to fad4fea Compare January 13, 2023 18:01

jpmcb force-pushed the helm-refactor branch from fad4fea to f7c1272 Compare January 18, 2023 01:26

jpmcb mentioned this pull request Jan 18, 2023

improve time window to become cron expression based scheduler #351

Merged

2 tasks

jpmcb changed the title ~~Refactor deployment and yaml generation to use Helm~~ Remove yamlgen. Refactor deployment and yaml generation to use Helm templating Jan 18, 2023

gthao313 mentioned this pull request Jan 18, 2023

Refactor integration test to install brupop via Helm #386

Open

jpmcb commented Feb 1, 2023

View reviewed changes

.github/workflows/deploy-helm-charts.yaml Show resolved Hide resolved

cbgbt reviewed Feb 2, 2023

View reviewed changes

jpmcb force-pushed the helm-refactor branch from f7c1272 to b6e00ce Compare February 14, 2023 20:21

jpmcb force-pushed the helm-refactor branch from b6e00ce to aa381db Compare February 14, 2023 21:36

jpmcb force-pushed the helm-refactor branch from aa381db to be82886 Compare February 17, 2023 21:24

cbgbt approved these changes Feb 22, 2023

View reviewed changes

webern approved these changes Feb 23, 2023

View reviewed changes

jpmcb force-pushed the helm-refactor branch from be82886 to 8d3de57 Compare February 24, 2023 01:24

jpmcb force-pushed the helm-refactor branch 2 times, most recently from 9b98b50 to 7399de4 Compare June 1, 2023 18:38

gthao313 approved these changes Jun 1, 2023

View reviewed changes

stmcginnis reviewed Jun 5, 2023

View reviewed changes

.github/workflows/deploy-helm-charts.yaml Show resolved Hide resolved

stmcginnis approved these changes Jun 5, 2023

View reviewed changes

deploy/charts/bottlerocket-update-operator/README.md Outdated Show resolved Hide resolved

jpmcb force-pushed the helm-refactor branch from 7399de4 to fdc4c8c Compare June 6, 2023 14:15

stmcginnis approved these changes Jun 6, 2023

View reviewed changes

jpmcb force-pushed the helm-refactor branch from fdc4c8c to 9242f7e Compare June 7, 2023 14:46

stmcginnis approved these changes Jun 7, 2023

View reviewed changes

jpmcb merged commit 0c7a21e into bottlerocket-os:develop Jun 12, 2023

jpmcb deleted the helm-refactor branch June 12, 2023 20:33

gthao313 mentioned this pull request Jul 10, 2023

v1.2.0 Tracking Issue #479

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `yamlgen`. Refactor deployment and yaml generation to use Helm templating #350

Remove `yamlgen`. Refactor deployment and yaml generation to use Helm templating #350

jpmcb commented Dec 1, 2022

jpmcb commented Dec 15, 2022

jpmcb commented Dec 20, 2022

stmcginnis left a comment

cbgbt Jan 9, 2023

jpmcb Jan 12, 2023

jpmcb Feb 14, 2023

cbgbt commented Jan 9, 2023

cbgbt left a comment

jpmcb commented Jan 12, 2023

jpmcb commented Jan 13, 2023

jpmcb commented Jan 18, 2023

cbgbt left a comment

jpmcb commented Feb 14, 2023

jpmcb commented Feb 14, 2023

jpmcb commented Feb 17, 2023 •

edited

Loading

cbgbt left a comment

webern left a comment

webern Feb 23, 2023

jpmcb Feb 24, 2023

jpmcb commented Feb 24, 2023

nalshamaajc commented Apr 18, 2023

lsobowalejc commented Apr 18, 2023

maxres-ch commented May 23, 2023

jpmcb commented Jun 1, 2023

gthao313 commented Jun 1, 2023 •

edited

Loading

stmcginnis left a comment

stmcginnis left a comment

jpmcb commented Jun 7, 2023

stmcginnis left a comment

Remove yamlgen. Refactor deployment and yaml generation to use Helm templating #350

Remove yamlgen. Refactor deployment and yaml generation to use Helm templating #350

Conversation

jpmcb commented Dec 1, 2022

jpmcb commented Dec 15, 2022

jpmcb commented Dec 20, 2022

stmcginnis left a comment

Choose a reason for hiding this comment

cbgbt Jan 9, 2023

Choose a reason for hiding this comment

jpmcb Jan 12, 2023

Choose a reason for hiding this comment

jpmcb Feb 14, 2023

Choose a reason for hiding this comment

cbgbt commented Jan 9, 2023

cbgbt left a comment

Choose a reason for hiding this comment

jpmcb commented Jan 12, 2023

jpmcb commented Jan 13, 2023

jpmcb commented Jan 18, 2023

cbgbt left a comment

Choose a reason for hiding this comment

jpmcb commented Feb 14, 2023

jpmcb commented Feb 14, 2023

jpmcb commented Feb 17, 2023 • edited Loading

cbgbt left a comment

Choose a reason for hiding this comment

webern left a comment

Choose a reason for hiding this comment

webern Feb 23, 2023

Choose a reason for hiding this comment

jpmcb Feb 24, 2023

Choose a reason for hiding this comment

jpmcb commented Feb 24, 2023

nalshamaajc commented Apr 18, 2023

lsobowalejc commented Apr 18, 2023

maxres-ch commented May 23, 2023

jpmcb commented Jun 1, 2023

Testing

gthao313 commented Jun 1, 2023 • edited Loading

stmcginnis left a comment

Choose a reason for hiding this comment

stmcginnis left a comment

Choose a reason for hiding this comment

jpmcb commented Jun 7, 2023

stmcginnis left a comment

Choose a reason for hiding this comment

Remove `yamlgen`. Refactor deployment and yaml generation to use Helm templating #350

Remove `yamlgen`. Refactor deployment and yaml generation to use Helm templating #350

jpmcb commented Feb 17, 2023 •

edited

Loading

gthao313 commented Jun 1, 2023 •

edited

Loading