feat: introduces conditional control plane installation #10

bartoszmajsak · 2023-08-31T18:13:17Z

Overview

When using KfDef such as:

apiVersion: kfdef.apps.kubeflow.org/v1
kind: KfDef
metadata:
  name: odh-mesh
spec:
  plugins:
    - kind: KfOssmPlugin
      spec:
        mesh:
          name: minimal
          namespace: istio-system
          mode: Managed
          certificate:
            generate: true
        auth:
          name: authorino
          namespace: auth-provider
          authorino:
            label: authorino/topic=odh
  applications:
    # your components
  repos:
    - name: manifests
      uri: https://github.com/maistra/odh-manifests/tarball/v0.0.6
  version: v0.0.6

the operator will install a minimal version of the control plane.

Changes

new feature of conditional installation of control plane based on Mesh.Mode value
reworked plugin integration in the KfApp call chain (see docs around Apply() and Delete())
fixed issue of leaving KfApp in an odd state when errors on Apply occurs (raised as separated PR too fix(reconcile): corrects KfDef status in case of any error opendatahub-io/opendatahub-operator#501)
reworks SMCP readiness check - it now checks not status, but a list of components in defined states
simplifies CRD existence check call

and reuse it in smcp-related funcs

instead of nil by mistake

in certain cases this information can only be determined at runtime, for example based on user-defined values of the plugin spec.

coupled with wait condition

It relies on owner reference so can be cleaned up as other resources

Additionally moves OssmResourceTracker creation to .Apply func

This way we can cleanup the resources in case deploying service mesh manifests because creating those in Generate phase leaves them hanging due to failing Delete hook

Additionally propagates the error through reconcile to keep trying, as it was ignored.

pkg/kfapp/ossm/feature/builder.go

cam-garrison

Requesting changes (or explanations) for two things, otherwise LGTM!

pkg/kfapp/ossm/feature/verification.go

pkg/kfapp/ossm/templates/control-plane/control-plane-managed.tmpl

pkg/kfapp/ossm/feature/verification.go

cam-garrison · 2023-08-31T20:05:56Z

Note from testing. If the mesh.mode field is not set, eg:

mesh:
          name: basic
          namespace: istio-system
          certificate:
            generate: true

The operator enters a CrashLoopBackOff state, with the error

2023-08-31T20:04:22.269Z INFO KfOssmPlugin Initializing {"plugin": "KfOssmPlugin"}
panic: reflect.Set: value of type string is not assignable to type ossmplugin.InstallationMode
goroutine 813 [running]:
reflect.Value.assignTo({0x1e65ba0?, 0xc0038441c0?, 0x7f992d423f18?}, {0x22e0a70, 0xb}, 0x1e5fee0, 0x0)
/usr/lib/golang/src/reflect/value.go:3064 +0x2ac
reflect.Value.Set({0x1e5fee0?, 0xc005a70180?, 0x22d975a?}, {0x1e65ba0?, 0xc0038441c0?, 0x1e5fee0?})
/usr/lib/golang/src/reflect/value.go:2090 +0xeb
github.com/opendatahub-io/opendatahub-operator/pkg/kfconfig/ossmplugin.setDefaults({0x1f9bea0?, 0xc005a70160?})
/workspace/pkg/kfconfig/ossmplugin/types.go:89 +0x2fe
github.com/opendatahub-io/opendatahub-operator/pkg/kfconfig/ossmplugin.setDefaults({0x202bea0?, 0xc005a70160?})
/workspace/pkg/kfconfig/ossmplugin/types.go:93 +0x33b
github.com/opendatahub-io/opendatahub-operator/pkg/kfconfig/ossmplugin.(*OssmPluginSpec).SetDefaults(...)
..................

To get out of it, have to delete KfDef and remove finalizer manually.

for example `type InstallationMode string` was previously failing when converting the default value defined in a custom tag. Now there's a check performed and if value can be convereted to a given type it will. In all other cases it will return an error.

bartoszmajsak · 2023-09-01T10:06:08Z

The operator enters a CrashLoopBackOff state, with the error

Thanks for spotting it! It's now fixed in ca7e9d8

in YAML: mode -> installationMode

values:

pre-installed
minimal

prometheus is gone

installation Proposed installation modes are: - minimal - pre-installed

cam-garrison

tested some more, lgtm

* chore: extracts smcp GVR to a var and reuse it in smcp-related funcs * fix: returns err on oauth creation failure instead of nil by mistake * feat: adds ability to enable feature based on predicates in certain cases this information can only be determined at runtime, for example based on user-defined values of the plugin spec. * feat: introduces control plane installation coupled with wait condition * fix: makes feature enabled by default * chore: reworks smcp creation It relies on owner reference so can be cleaned up as other resources * fix: applies cleanups only if feature is enabled Additionally moves OssmResourceTracker creation to .Apply func * feat: reworks plugin integration to use Apply and Delete funcs of KfApp This way we can cleanup the resources in case deploying service mesh manifests because creating those in Generate phase leaves them hanging due to failing Delete hook * fix(reconcile): corrects KfDef status in case of any error Additionally propagates the error through reconcile to keep trying, as it was ignored. * fix: reworks SMCP component readiness check * chore: improves KfApp func docs * chore: reworks logging in verifiers * chore: simplifies CRD existence check * chore: adds tests for feature enablement * fix: sets interval for SMCP polling to 5s * fix: handles types which are derived for built-in ones for example `type InstallationMode string` was previously failing when converting the default value defined in a custom tag. Now there's a check performed and if value can be convereted to a given type it will. In all other cases it will return an error. * fix: sets default control plane installation mode rely on existing installation Proposed installation modes are: - minimal - pre-installed

bartoszmajsak added 14 commits August 29, 2023 13:26

chore: extracts smcp GVR to a var

de59d3f

and reuse it in smcp-related funcs

fix: returns err on oauth creation failure

5f8629f

instead of nil by mistake

feat: adds ability to enable feature based on predicates

3a7f660

in certain cases this information can only be determined at runtime, for example based on user-defined values of the plugin spec.

feat: introduces control plane installation

88c5bfe

coupled with wait condition

fix: makes feature enabled by default

a5b2b5e

chore: reworks smcp creation

a340f02

It relies on owner reference so can be cleaned up as other resources

fix: applies cleanups only if feature is enabled

3fc060d

Additionally moves OssmResourceTracker creation to .Apply func

feat: reworks plugin integration to use Apply and Delete funcs of KfApp

ecd321f

This way we can cleanup the resources in case deploying service mesh manifests because creating those in Generate phase leaves them hanging due to failing Delete hook

fix(reconcile): corrects KfDef status in case of any error

e8320a9

Additionally propagates the error through reconcile to keep trying, as it was ignored.

fix: reworks SMCP component readiness check

ae3066a

chore: improves KfApp func docs

e328637

chore: reworks logging in verifiers

d3551af

chore: simplifies CRD existence check

cb39700

chore: adds tests for feature enablement

30430bc

bartoszmajsak requested a review from cam-garrison August 31, 2023 18:13

bartoszmajsak commented Aug 31, 2023

View reviewed changes

pkg/kfapp/ossm/feature/builder.go Show resolved Hide resolved

cam-garrison requested changes Aug 31, 2023

View reviewed changes

pkg/kfapp/ossm/feature/verification.go Outdated Show resolved Hide resolved

pkg/kfapp/ossm/templates/control-plane/control-plane-managed.tmpl Outdated Show resolved Hide resolved

bartoszmajsak commented Aug 31, 2023

View reviewed changes

pkg/kfapp/ossm/feature/verification.go Outdated Show resolved Hide resolved

fix: sets interval for SMCP polling to 5s

f25c848

bartoszmajsak requested a review from cam-garrison August 31, 2023 20:01

bartoszmajsak force-pushed the installation_feature branch from fb9768d to 561d0e6 Compare September 1, 2023 08:54

bartoszmajsak force-pushed the installation_feature branch from 561d0e6 to be7923b Compare September 1, 2023 10:00

fix: sets default control plane installation mode rely on existing

ca7e9d8

installation Proposed installation modes are: - minimal - pre-installed

cam-garrison approved these changes Sep 1, 2023

View reviewed changes

bartoszmajsak merged commit 38a59a5 into maistra:kf_ossm_plugin Sep 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduces conditional control plane installation #10

feat: introduces conditional control plane installation #10

bartoszmajsak commented Aug 31, 2023

cam-garrison left a comment

cam-garrison commented Aug 31, 2023

bartoszmajsak commented Sep 1, 2023 •

edited

Loading

cam-garrison left a comment

feat: introduces conditional control plane installation #10

feat: introduces conditional control plane installation #10

Conversation

bartoszmajsak commented Aug 31, 2023

Overview

Changes

cam-garrison left a comment

Choose a reason for hiding this comment

cam-garrison commented Aug 31, 2023

bartoszmajsak commented Sep 1, 2023 • edited Loading

cam-garrison left a comment

Choose a reason for hiding this comment

bartoszmajsak commented Sep 1, 2023 •

edited

Loading