This repository has been archived by the owner on Aug 12, 2024. It is now read-only.

add items to dynamic watch in helm #517

Merged

akihikokuroda merged 1 commit into operator-framework:main from akihikokuroda:reconcile

Oct 12, 2022

Member

akihikokuroda commented Aug 22, 2022

Closes #514

Signed-off-by: akihikokuroda akuroda@us.ibm.com

akihikokuroda requested a review from a team as a code owner

August 22, 2022 18:02

openshift-ci bot added the do-not-merge/work-in-progress label

akihikokuroda force-pushed the reconcile branch from d54c821 to 0f83a3a Compare

August 22, 2022 22:43

akihikokuroda changed the title ~~[WIP] add items to dynamicwtach in helm~~ add items to dynamicwtach in helm

openshift-ci bot removed the do-not-merge/work-in-progress label

akihikokuroda force-pushed the reconcile branch from 0f83a3a to 2f8e7df Compare

August 23, 2022 00:12

joelanford reviewed

View reviewed changes

Member

joelanford left a comment

This seems to take a different approach than the plain provisioner. Should this with how the plain provisioner works? That would hopefully make it easier to find re-use opportunities in the future, and it would ensure the UX is generally the same for both.

Member Author

akihikokuroda commented Aug 23, 2022

@joelanford The UX should be the same. It is doing the same thing as the plain provisioner but it can not do the same way. It is adding to the watch using the release.manifest after the install because the yaml files in the charts can not be decoded without the rendering. It is adding the labels using the postrenderer because it must be done against the rendered objects.

Member

joelanford commented Aug 23, 2022

Oh right. In Plain, we know the full content of the "templates" ahead of time. In Helm, we have to run the chart through the helm libraries to know what the resulting manifest is.

One other thing that comes to mind. We use a post-renderer under the hood in the helm-operator-plugins repo to inject owner references. If we override the post-renderer to set these labels, we might be overriding the ownerref post renderer.

Member Author

akihikokuroda commented Aug 23, 2022

If we override the post-renderer to set these labels, we might be overriding the ownerref post renderer.

Yes, I found that. I added the ownerref injection in the post-render function.

akihikokuroda force-pushed the reconcile branch from 2f8e7df to 0ee9c0b Compare

August 23, 2022 16:04

Member

joelanford commented Aug 23, 2022

Not blocking for this PR, but I imagine that unknowingly setting a post-renderer like you did could be a footgun for others. I wonder if there's a way in helm-operator-plugins to support adding more caller-defined post-renderers to the chain.

akihikokuroda force-pushed the reconcile branch from 0ee9c0b to 049c44a Compare

August 23, 2022 18:45

Member Author

akihikokuroda commented Aug 23, 2022

I made changes to call the preset PostRenderer after our PostRenderer if it is set.

joelanford mentioned this pull request

add flexibility around use of post-renderer operator-framework/helm-operator-plugins#195

Merged

Member

joelanford commented Aug 24, 2022 •

edited

Loading

@akihikokuroda I've got a PR up in helm-operator-plugins that I think should get rid of the awkward hacking you had to do to use both the built-in and extra post-renderer.

operator-framework/helm-operator-plugins#195

akihikokuroda force-pushed the reconcile branch 2 times, most recently from 02920f9 to 2a9a878 Compare

August 25, 2022 21:58

timflannagan reviewed

View reviewed changes

Member

timflannagan left a comment

These changes seem reasonable. If we're hacking around the helm-operator-plugins implementation here, does it make sense to create a ticket that tracks cleaning up the implementation once Joe's changes land and a new release is cut?

internal/provisioner/helm/controllers/bundledeployment_controller.go Outdated

+              			break
+              		}
+              		if err != nil {
+              			return fmt.Errorf("read %q: %v", obj, err)

Member

timflannagan Aug 26, 2022

Is printing the obj here potentially problem for logs? Or does this just print the metadata information we care about?

Member Author

akihikokuroda Aug 26, 2022

The err has enough information. I take out the obj.

internal/provisioner/helm/controllers/bundledeployment_controller.go Outdated

@@ @@ -286,7 +323,7 @@ const ( @@
               	stateError        releaseState = "Error"
               )
-              func (r *BundleDeploymentReconciler) getReleaseState(cl helmclient.ActionInterface, obj metav1.Object, chrt *chart.Chart, values chartutil.Values) (*release.Release, releaseState, error) {
+              func (r *BundleDeploymentReconciler) getReleaseState(cl helmclient.ActionInterface, obj metav1.Object, chrt *chart.Chart, values chartutil.Values, post *postrenderer) (*release.Release, releaseState, error) {

Member

timflannagan Aug 26, 2022

We may want/need a tech debt ticket here given the number of parameters for this method.

Member Author

akihikokuroda Aug 26, 2022

OK. I'll add a TODO comment.

internal/provisioner/helm/controllers/bundledeployment_controller.go Outdated

               	if err != nil {
               		return currentRelease, stateError, err
               	}
               	if desiredRelease.Manifest != currentRelease.Manifest ||
+              		!reflect.DeepEqual(desiredRelease.Chart.Metadata, currentRelease.Chart.Metadata) ||

Member

timflannagan Aug 26, 2022

Any idea on why this comparison is needed?

Member Author

akihikokuroda Aug 26, 2022

This is checking for the changes in Chart.yaml file. It's contents are here.

internal/provisioner/helm/controllers/bundledeployment_controller.go Outdated

               		}),
               		updater.EnsureInstalledName(bundle.GetName()),
               	)
+              	if err := r.addDynamicWatch(cl, bd, bundle); err != nil {

Member

timflannagan Aug 26, 2022

This feels like something the plain provisioner could also hook into?

Member Author

akihikokuroda Aug 26, 2022

This is checking for the changes in Chart.yaml file. The plain provisioner is generating the contents of the Chart.yam file on the fly so it's always same.

internal/provisioner/helm/controllers/bundledeployment_controller.go Outdated

-              	rel, state, err := r.getReleaseState(cl, bd, chrt, values)
+              	post := &postrenderer{
+              		labels: map[string]string{
+              			util.CoreOwnerKindKey: "BundleDeployment",

Member

timflannagan Aug 26, 2022

nit: I think we have a const variable somewhere for the BD GVK.

Member Author

akihikokuroda Aug 26, 2022

It doesn't seem defined.

Member Author

akihikokuroda commented Aug 26, 2022

@timflannagan I created the issue for refactoring. #534

Member

timflannagan commented Aug 26, 2022

Awesome. Follow-up question: do we think it would be valuable to also add a comment linking to that issue for the postrenderer hacks?

Member Author

akihikokuroda commented Aug 26, 2022 •

edited

Loading

That's a good idea. I'll do it.

Refactor issue: #534

akihikokuroda force-pushed the reconcile branch 2 times, most recently from 6169128 to 6be4583 Compare

August 31, 2022 13:29

openshift-merge-robot added the needs-rebase label

akihikokuroda force-pushed the reconcile branch from 6be4583 to 3e706cf Compare

September 23, 2022 23:28

openshift-merge-robot removed the needs-rebase label

akihikokuroda force-pushed the reconcile branch from 3e706cf to 3a18d86 Compare

September 24, 2022 01:04

akihikokuroda force-pushed the reconcile branch from 3a18d86 to 5e91b3e Compare

October 7, 2022 01:30

Member Author

akihikokuroda commented Oct 7, 2022

Refactor has done. Please review.

timflannagan reviewed

View reviewed changes

internal/provisioner/bundledeployment/bundledeployment.go Outdated

Comment on lines 505 to 507

+              		For(&rukpakv1alpha1.BundleDeployment{}, builder.WithPredicates(
+              			util.BundleDeploymentProvisionerFilter(plain.ProvisionerID)),
+              		).

Member

timflannagan Oct 10, 2022

I haven't had a chance to look over the new directory structure since #447 landed: we're specifying the plain.ProvisionerID here, and filtering BundleDeployments that don't match that unique provisioner class ID from the cache. Is that the right read here? If so, I think we'll need to solve this in an agnostic way s.t. our core provisioners can leverage this functionality without inter-depending on the plain provisioner libraries.

Member Author

akihikokuroda Oct 10, 2022

Oh, it's my mistake. This function is not used any more since #447 changes. I'll take this out.

timflannagan reviewed

View reviewed changes

internal/provisioner/bundledeployment/bundledeployment.go Outdated

               	desiredRelease, err := cl.Upgrade(obj.GetName(), p.releaseNamespace, chrt, values, func(upgrade *action.Upgrade) error {
               		upgrade.DryRun = true
               		return nil
-              	})
+              	},
+              		// To be refactored issue 534

Member

timflannagan Oct 10, 2022

nit: can we copy/paste the 534 issue link so it's easier to track when reading through the codebase locally?

Member Author

akihikokuroda Oct 10, 2022

Sure. It must be better.

timflannagan reviewed

View reviewed changes

internal/provisioner/bundledeployment/bundledeployment.go Outdated

-              	rel, state, err := p.getReleaseState(cl, bd, chrt, values)
+              	post := &postrenderer{
+              		labels: map[string]string{
+              			util.CoreOwnerKindKey: "BundleDeployment",

Member

timflannagan Oct 10, 2022

nit: we have a constant variable for the BundleDeployment Kind: https://github.com/operator-framework/rukpak/blob/main/api/v1alpha1/bundledeployment_types.go#L26.

Member Author

akihikokuroda Oct 10, 2022

Thanks! I'll update.

Member

timflannagan commented Oct 10, 2022

Sorry for the slow turnaround on this. I've been pretty heads down with other projects/work recently. Can we also re-title this PR to fix the typo?

akihikokuroda changed the title ~~add items to dynamicwtach in helm~~ add items to dynamic watch in helm


          add resources to watch list

9137b59

Signed-off-by: akihikokuroda <akuroda@us.ibm.com>

akihikokuroda force-pushed the reconcile branch from 5e91b3e to 9137b59 Compare

October 10, 2022 20:00

Member

timflannagan commented Oct 12, 2022 •

edited

Loading

Pulling this down now, and quickly testing it out locally.

timflannagan approved these changes

View reviewed changes

Member

timflannagan left a comment

I tested these changes out locally, and it looks like we're good to go:

apiVersion: core.rukpak.io/v1alpha1
kind: BundleDeployment
metadata:
  name: hello-world-helm
  namespace: demo1
spec:
  provisionerClassName: core-rukpak-io-helm
  template:
    metadata:
      labels:
        app: hello-world-helm
    spec:
      provisionerClassName: core-rukpak-io-helm
      source:
        http:
          url: https://github.com/helm/examples/releases/download/hello-world-0.1.0/hello-world-0.1.0.tgz
        type: http

And then deleted the deployment that got created from that Bundle resource a couple of times, and waited for it to be quickly recreated.

I haven't had a chance to catch up the new repository structure, but this looks like a reasonable short term hack while we wait for a release to be cut.

tylerslaton approved these changes

View reviewed changes

akihikokuroda merged commit e430327 into operator-framework:main

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet