Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
d45938c
Added migration, adjusted incluster_resolver
ANeumann82 Sep 10, 2020
8d231ff
Extract operator version sorting into kudo package
ANeumann82 Sep 10, 2020
90399cb
Started to add tests
ANeumann82 Sep 10, 2020
83e386c
Fixed migration, adjusted upgrade test to confirm migration
ANeumann82 Sep 15, 2020
d2a04f5
Merge branch 'main' into an/rename-operatorversions
ANeumann82 Sep 15, 2020
0bd2cc9
Adjusted migrations
ANeumann82 Sep 15, 2020
7683c13
Use adjusted operator version tests
ANeumann82 Sep 16, 2020
88701b4
Fixed integration test
ANeumann82 Sep 16, 2020
4733914
Fix e2e-tests
ANeumann82 Sep 16, 2020
be11d6d
Small cleanup
ANeumann82 Sep 16, 2020
0fe273c
Merge branch 'main' into an/rename-operatorversions
ANeumann82 Sep 16, 2020
d54c556
Cleanup migrations
ANeumann82 Sep 16, 2020
d2aab43
Merge branch 'main' into an/rename-operatorversions
ANeumann82 Sep 21, 2020
b1f5b4f
Adjustments for merged main
ANeumann82 Sep 21, 2020
c5f7030
Small cleanup
ANeumann82 Sep 21, 2020
ec415a5
Use SemVer v3
ANeumann82 Sep 22, 2020
108c854
Updated documentation
ANeumann82 Sep 23, 2020
553a1fa
Verify that OV `appVersion` is also a semver (if present)
zen-dog Sep 23, 2020
971404a
Usre more go-like var names
ANeumann82 Sep 25, 2020
11dd956
Add SemVer verify for appVersion back in.
ANeumann82 Sep 25, 2020
13ffcd7
Small changes from review
ANeumann82 Oct 2, 2020
48304db
Code review requests
ANeumann82 Oct 8, 2020
9c8b72b
Adjusted migration
ANeumann82 Oct 8, 2020
45366f1
Removed migration
ANeumann82 Oct 8, 2020
fb3f2a8
Merge branch 'main' into an/rename-operatorversions
ANeumann82 Oct 8, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ go 1.14

require (
github.com/Masterminds/goutils v1.1.0 // indirect
github.com/Masterminds/semver v1.5.0 // indirect
github.com/Masterminds/semver v1.5.0
github.com/Masterminds/semver/v3 v3.1.0
github.com/Masterminds/sprig v2.22.0+incompatible
github.com/go-bindata/go-bindata/v3 v3.1.3
Expand Down
2 changes: 1 addition & 1 deletion hack/run-operator-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ docker build . \
-t "kudobuilder/controller:$KUDO_VERSION"

rm -rf operators
git clone https://github.com/kudobuilder/operators
git clone --branch "an/rename-operatorversions" https://github.com/kudobuilder/operators
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This will need to be removed before merging - It requires changes on the operator tests

mkdir operators/bin/
cp ./bin/kubectl-kudo operators/bin/
sed "s/%version%/$KUDO_VERSION/" operators/kudo-test.yaml.tmpl > operators/kudo-test.yaml
Expand Down
34 changes: 31 additions & 3 deletions pkg/apis/kudo/v1beta1/operatorversion_types_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,37 @@ import (

"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/kudobuilder/kudo/pkg/util/kudo"
)

func OperatorInstanceName(operatorName string) string {
return fmt.Sprintf("%s-instance", operatorName)
}

func OperatorVersionName(operatorName, version string) string {
return fmt.Sprintf("%s-%s", operatorName, version)
func OperatorVersionName(operatorName, ovVersion, appVersion string) string {
if appVersion == "" {
return fmt.Sprintf("%s-%s", operatorName, ovVersion)
}
return fmt.Sprintf("%s-%s-%s", operatorName, appVersion, ovVersion)
}

func (ov *OperatorVersion) FullyQualifiedName() string {
return fmt.Sprintf("%s-%s", ov.Name, ov.Spec.AppVersion)
return OperatorVersionName(ov.Spec.Operator.Name, ov.Spec.Version, ov.Spec.AppVersion)
}

func (ov *OperatorVersion) EqualOperatorVersion(other *OperatorVersion) bool {
return ov.FullyQualifiedName() == other.FullyQualifiedName()
}

func ListOperatorVersions(ns string, c client.Reader) (ovList *OperatorVersionList, err error) {
ovList = &OperatorVersionList{}
if err := c.List(context.TODO(), ovList, client.InNamespace(ns)); err != nil {
return nil, err
}
return ovList, nil
}

func GetOperatorVersionByName(name, ns string, c client.Reader) (ov *OperatorVersion, err error) {
ov = &OperatorVersion{}
err = c.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: ns}, ov)
Expand All @@ -32,3 +45,18 @@ func GetOperatorVersionByName(name, ns string, c client.Reader) (ov *OperatorVer
}
return ov, nil
}

// Sortable Operator implements functionality to correctly sort OVs by name, appVersion and operatorVersion
var _ kudo.SortableOperator = &OperatorVersion{}

func (ov *OperatorVersion) OperatorName() string {
return ov.Spec.Operator.Name
}

func (ov *OperatorVersion) OperatorVersion() string {
return ov.Spec.Version
}

func (ov *OperatorVersion) AppVersion() string {
return ov.Spec.AppVersion
}
31 changes: 27 additions & 4 deletions pkg/controller/instance/resolver_incluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

kudoapi "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
"github.com/kudobuilder/kudo/pkg/util/kudo"
)

// InClusterResolver resolves packages that are already installed in the cluster. Note, that unlike other resolvers, the
Expand All @@ -18,11 +19,33 @@ type InClusterResolver struct {
}

func (r InClusterResolver) Resolve(name string, appVersion string, operatorVersion string) (*packages.Package, error) {
ovn := kudoapi.OperatorVersionName(name, operatorVersion)

ov, err := kudoapi.GetOperatorVersionByName(ovn, r.ns, r.c)
ovList, err := kudoapi.ListOperatorVersions(r.ns, r.c)
if err != nil {
return nil, fmt.Errorf("failed to resolve operator version %s/%s:%s", r.ns, ovn, appVersion)
return nil, fmt.Errorf("failed to list operator versions in namespace %q: %v", r.ns, err)
}

if len(ovList.Items) == 0 {
return nil, fmt.Errorf("failed to find any operator version in namespace %s", r.ns)
}

// Put all items into a new list to be sortable
newOvList := kudo.SortableOperatorList{}
for _, ovFromList := range ovList.Items {
ovFromList := ovFromList
newOvList = append(newOvList, &ovFromList)
}

// Only consider OVs for the given name
newOvList = newOvList.FilterByName(name)

// Sort items
newOvList.Sort()

// Find first matching OV
ov := newOvList.FindFirstMatch(name, operatorVersion, appVersion).(*kudoapi.OperatorVersion) // nolint:errcheck

if ov == nil {
return nil, fmt.Errorf("failed to resolve operator version in namespace %q for name %q, version %q, appVersion %q", r.ns, name, operatorVersion, appVersion)
}

o, err := kudoapi.GetOperator(name, r.ns, r.c)
Expand Down
3 changes: 2 additions & 1 deletion pkg/engine/task/task_kudo_operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ func (kt KudoOperatorTask) Run(ctx Context) (bool, error) {
namespace := ctx.Meta.InstanceNamespace
operatorName := kt.OperatorName
operatorVersion := kt.OperatorVersion
operatorVersionName := kudoapi.OperatorVersionName(operatorName, operatorVersion)
appVersion := kt.AppVersion
operatorVersionName := kudoapi.OperatorVersionName(operatorName, operatorVersion, appVersion)
instanceName := dependencyInstanceName(ctx.Meta.InstanceName, kt.InstanceName, operatorName)

// 1. - Expand parameter file if exists -
Expand Down
17 changes: 17 additions & 0 deletions pkg/kudoctl/kube/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,27 @@ import (
"fmt"

apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/kudobuilder/kudo/pkg/apis"
"github.com/kudobuilder/kudo/pkg/client/clientset/versioned"
"github.com/kudobuilder/kudo/pkg/kudoctl/clog"
)

// Client provides access different K8S clients
type Client struct {
Scheme *runtime.Scheme
KubeClient kubernetes.Interface
ExtClient apiextensionsclient.Interface
DynamicClient dynamic.Interface
CtrlClient client.Client
KudoClient versioned.Interface
}

// GetConfig returns a Kubernetes client config for a given kubeconfig.
Expand Down Expand Up @@ -70,11 +76,22 @@ func GetKubeClientForConfig(config *rest.Config) (*Client, error) {
if err != nil {
return nil, fmt.Errorf("could not create Controller Runtime client: %s", err)
}
kudoClient, err := versioned.NewForConfig(config)
if err != nil {
return nil, fmt.Errorf("could not create KUDO client: %v", err)
}

fullScheme := scheme.Scheme
if err := apis.AddToScheme(fullScheme); err != nil {
return nil, fmt.Errorf("failed to create runtime.Scheme: %v", err)
}

return &Client{
KubeClient: kubeClient,
ExtClient: extClient,
DynamicClient: dynamicClient,
CtrlClient: ctrlClient,
KudoClient: kudoClient,
Scheme: fullScheme,
}, nil
}
124 changes: 124 additions & 0 deletions pkg/kudoctl/kudoinit/migration/01_migrate_ov_names/migrate_ov_names.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
//nolint:golint,stylecheck
package _01_migrate_ov_names

import (
"context"
"fmt"

"github.com/thoas/go-funk"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

kudoapi "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
"github.com/kudobuilder/kudo/pkg/kudoctl/clog"
"github.com/kudobuilder/kudo/pkg/kudoctl/kube"
"github.com/kudobuilder/kudo/pkg/kudoctl/kudoinit/migration"
)

var (
_ migration.Migrater = &MigrateOvNames{}
)

type MigrateOvNames struct {
client *kube.Client
dryRun bool

migratedOVs map[string]*kudoapi.OperatorVersion
}

func New(client *kube.Client, dryRun bool) *MigrateOvNames {
return &MigrateOvNames{
client: client,
dryRun: dryRun,
migratedOVs: map[string]*kudoapi.OperatorVersion{},
}
}

func (m *MigrateOvNames) CanMigrate() error {
// No migrate check required for this migration
return nil
}

func (m *MigrateOvNames) Migrate() error {
return migration.ForEachNamespace(m.client, m.migrateInNamespace)
}

func (m *MigrateOvNames) migrateInNamespace(ns string) error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so this will be run when you upgrade to 0.17 but will this be run again when migrating from 0.17 to 0.18? I don't think we want to do that

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It would, yes - And it doesn't matter, the migrations are (or should be) idempotent. You can re-run them again and again, they shouldn't modify anything after the first run.

clog.V(1).Printf("Run OperatorVersion name migration on namespace %q", ns)
if err := migration.ForEachOperatorVersion(m.client, ns, m.migrateOperatorVersion); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is not really atomic because before you migrate instance, it has no OV linked or the link is rather broken :/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so what if this fails in the middle? we'll leave the cluster in broken state with no easy way to fix this :/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe the re-run of upgrade would actually fix itself? 🤔 but will it even work if manager is down and everything is "in between"?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah - the migration could have been fixed to be completely reentrant, the code that you reviewed wasn't though. I've removed the migration now, but we should keep that in mind for future migrations.

return fmt.Errorf("failed to migrate OperatorVersions in namespace %q: %v", ns, err)
}
if err := migration.ForEachInstance(m.client, ns, m.migrateInstance); err != nil {
return fmt.Errorf("failed to migrate Instance ownership in namespace %q: %v", ns, err)
}

return nil
}

func (m *MigrateOvNames) migrateOperatorVersion(ov *kudoapi.OperatorVersion) error {
expectedName := ov.FullyQualifiedName()
if ov.Name == expectedName {
// Nothing to migrate
return nil
}

oldName := ov.Name
clog.V(0).Printf("Migrate OperatorVersion from %q to %q", ov.Name, ov.FullyQualifiedName())

ov.Name = expectedName

// Reset Read-Only fields
ov.Status = kudoapi.OperatorVersionStatus{}
ov.ResourceVersion = ""
ov.Generation = 0
ov.UID = ""
ov.CreationTimestamp = v1.Time{}

if !m.dryRun {
var err error
clog.V(4).Printf("Create copy of OperatorVersion with name %q", ov.Name)
ov, err = m.client.KudoClient.KudoV1beta1().OperatorVersions(ov.Namespace).Create(context.TODO(), ov, v1.CreateOptions{})
if err != nil {
return err
}
}

// Store migrated operator versions for instance owner references
m.migratedOVs[oldName] = ov

if !m.dryRun {
clog.V(4).Printf("Delete old OperatorVersion with name %q", oldName)
if err := m.client.KudoClient.KudoV1beta1().OperatorVersions(ov.Namespace).Delete(context.TODO(), oldName, v1.DeleteOptions{}); err != nil {
return fmt.Errorf("failed to delete old OperatorVersion %q: %v", oldName, err)
}
}
return nil
}

func (m *MigrateOvNames) migrateInstance(i *kudoapi.Instance) error {
newOwner, ovWasMigrated := m.migratedOVs[i.Spec.OperatorVersion.Name]
if ovWasMigrated {
oldName := i.Spec.OperatorVersion.Name
clog.V(1).Printf("Adjust OperatorVersion and owner reference of Instance %q", i.Name)

// Set OperatorVersion
i.Spec.OperatorVersion.Name = newOwner.Name
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm, I'm not sure operators are prepared to deal with such change at runtime. If {{ .OperatorName }} was used by the deploy plan, the value might have trickled down deep into resources, get exposed in UIs, URLs, etc 🤔
Does this change, when applied, cause the update or deploy plan to be triggered?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's a good point. I didn't even think about the "OperatorName" variable -.-

I have removed the migration for now - We'll still have to test an upgrade with more complex operators that use dependencies.


// Replace OwnerReference
//nolint:errcheck
i.OwnerReferences = funk.Filter(i.OwnerReferences, func(o v1.OwnerReference) bool { return o.Name != oldName }).([]v1.OwnerReference)
if err := controllerutil.SetOwnerReference(newOwner, i, m.client.Scheme); err != nil {
return fmt.Errorf("failed to set resource ownership for the new instance: %v", err)
}

// Save update
if !m.dryRun {
clog.V(4).Printf("Update instance %q with new owner reference", i.Name)
_, err := m.client.KudoClient.KudoV1beta1().Instances(i.Namespace).Update(context.TODO(), i, v1.UpdateOptions{})
if err != nil {
return fmt.Errorf("failed to update Instance %q with new owner reference: %v", i.Name, err)
}
}
}
return nil
}
9 changes: 5 additions & 4 deletions pkg/kudoctl/kudoinit/migration/migration.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package migration

import "github.com/kudobuilder/kudo/pkg/kudoctl/kube"

type Migrater interface {
CanMigrate(client *kube.Client) error
Migrate(client *kube.Client) error
// CanMigrate checks if there are any conditions that would prevent this migration to run
CanMigrate() error

// Migrate executes the
Migrate() error
}
52 changes: 52 additions & 0 deletions pkg/kudoctl/kudoinit/migration/migration_helper.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package migration

import (
"context"
"fmt"

v1 "k8s.io/apimachinery/pkg/apis/meta/v1"

kudoapi "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
"github.com/kudobuilder/kudo/pkg/kudoctl/kube"
)

func ForEachNamespace(client *kube.Client, f func(ns string) error) error {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: These functions would make sense in pkg/kudoctl/util/kudo as well. But with a kube.Client and kudo.Client, it looks like our usage of the Kubernetes client API will need some refactoring first.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeaaaah.... The usage of the different clients is something that really should be refactored and cleaned up. Same with the loggers.

We should have established rules which client(s) and which logger(s) is used where, (i.e. CLI code, Manager code and shared code), but that's a discussion for a different time :)

nsList, err := client.KubeClient.CoreV1().Namespaces().List(context.TODO(), v1.ListOptions{})
if err != nil {
return fmt.Errorf("failed to fetch namespaces: %v", err)
}
for _, ns := range nsList.Items {
if err := f(ns.Name); err != nil {
return fmt.Errorf("failed to run function for namespace %q: %v", ns.Name, err)
}
}
return nil
}

func ForEachOperatorVersion(client *kube.Client, ns string, f func(ov *kudoapi.OperatorVersion) error) error {
ovList, err := client.KudoClient.KudoV1beta1().OperatorVersions(ns).List(context.TODO(), v1.ListOptions{})
if err != nil {
return fmt.Errorf("failed to fetch OperatorVersions from namespace %q: %v", ns, err)
}
for _, ov := range ovList.Items {
ov := ov
if err := f(&ov); err != nil {
return fmt.Errorf("failed to run function for OperatorVersion \"%s/%s\": %v", ov.Namespace, ov.Name, err)
}
}
return nil
}

func ForEachInstance(client *kube.Client, ns string, f func(ov *kudoapi.Instance) error) error {
iList, err := client.KudoClient.KudoV1beta1().Instances(ns).List(context.TODO(), v1.ListOptions{})
if err != nil {
return fmt.Errorf("failed to fetch Instances from namespace %q: %v", ns, err)
}
for _, instance := range iList.Items {
instance := instance
if err := f(&instance); err != nil {
return fmt.Errorf("failed to run function for Instance \"%s/%s\": %v", instance.Namespace, instance.Name, err)
}
}
return nil
}
Loading