Skip to content
Merged
Show file tree
Hide file tree
Changes from 21 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 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
43 changes: 40 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) (l *OperatorVersionList, err error) {
l = &OperatorVersionList{}
if err := c.List(context.TODO(), l, client.InNamespace(ns)); err != nil {
return nil, err
}
return l, 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,27 @@ 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
}

func ToSortableOperatorList(ovList []OperatorVersion) kudo.SortableOperatorList {
l := kudo.SortableOperatorList{}
for _, ov := range ovList {
ov := ov
l = append(l, &ov)
}
return l
}
2 changes: 1 addition & 1 deletion pkg/controller/instance/resolver_incluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ type InClusterResolver struct {
}

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

ov, err := kudoapi.GetOperatorVersionByName(ovn, r.ns, r.c)
if err != nil {
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
1 change: 1 addition & 0 deletions pkg/kudoctl/cmd/verify/verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ func (VersionVerifier) Verify(pf *packages.Files) verifier.Result {
return res
}
verifySemVer(pf.Operator.OperatorVersion, "operatorVersion", &res, true)
verifySemVer(pf.Operator.AppVersion, "appVersion", &res, false)
verifySemVer(pf.Operator.KubernetesVersion, "kubernetesVersion", &res, true)
verifySemVer(pf.Operator.KUDOVersion, "kudoVersion", &res, false)
return res
Expand Down
7 changes: 7 additions & 0 deletions pkg/kudoctl/kube/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"k8s.io/client-go/tools/clientcmd"
"sigs.k8s.io/controller-runtime/pkg/client"

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

Expand All @@ -19,6 +20,7 @@ type Client struct {
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 +72,16 @@ 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)
}

return &Client{
KubeClient: kubeClient,
ExtClient: extClient,
DynamicClient: dynamicClient,
CtrlClient: ctrlClient,
KudoClient: kudoClient,
}, nil
}
119 changes: 119 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,119 @@
//nolint:golint,stylecheck
package _01_migrate_ov_names

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/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
ctx context.Context

migratedOVs map[string]*kudoapi.OperatorVersion
}

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

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

func (m *MigrateOvNames) Migrate() error {
if !m.dryRun {
clog.V(0).Printf("Migrate OperatorVersion names")
}
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(m.ctx, ov, v1.CreateOptions{})
if err != nil {
return fmt.Errorf("failed to create copy of operator version: %v", err)
}
}

// Store migrated operator versions for instance migration
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(m.ctx, 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 {
clog.V(1).Printf("Adjust OperatorVersion 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.


// 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(m.ctx, i, v1.UpdateOptions{})
if err != nil {
return fmt.Errorf("failed to update Instance %q with new operator version: %v", i.Name, err)
}
}
}
return nil
}
12 changes: 8 additions & 4 deletions pkg/kudoctl/kudoinit/migration/migration.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
package migration

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

// Migrater is the base interface for migrations.
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
// This function should only return an error if it is sure that the migration can not be executed
CanMigrate() error

// Migrate executes the migration. The call must be idempotent and ignore already migrated resources
// It can be called multiple times on the same cluster and encounter migrated and unmigrated resources.
Migrate() error
}
56 changes: 56 additions & 0 deletions pkg/kudoctl/kudoinit/migration/migration_helper.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// This package contains helper functions that can be reused by all migrations
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"
)

// ForEachNamespace calls the given function for all namespaces in the cluster
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 :)

l, 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 l.Items {
if err := f(ns.Name); err != nil {
return fmt.Errorf("failed to run function for namespace %q: %v", ns.Name, err)
}
}
return nil
}

// ForEachOperatorVersion calls the given function for all operatorversions in the given namespace
func ForEachOperatorVersion(client *kube.Client, ns string, f func(ov *kudoapi.OperatorVersion) error) error {
l, 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 l.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
}

// ForEachInstance calls the given function for all instances in the given namespace
func ForEachInstance(client *kube.Client, ns string, f func(ov *kudoapi.Instance) error) error {
l, 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 _, i := range l.Items {
i := i
if err := f(&i); err != nil {
return fmt.Errorf("failed to run function for Instance \"%s/%s\": %v", i.Namespace, i.Name, err)
}
}
return nil
}
19 changes: 11 additions & 8 deletions pkg/kudoctl/kudoinit/setup/setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/kudobuilder/kudo/pkg/kudoctl/kudoinit/crd"
"github.com/kudobuilder/kudo/pkg/kudoctl/kudoinit/manager"
"github.com/kudobuilder/kudo/pkg/kudoctl/kudoinit/migration"
_01_migrate_ov_names "github.com/kudobuilder/kudo/pkg/kudoctl/kudoinit/migration/01_migrate_ov_names"
"github.com/kudobuilder/kudo/pkg/kudoctl/kudoinit/prereq"
"github.com/kudobuilder/kudo/pkg/kudoctl/verifier"
)
Expand Down Expand Up @@ -68,11 +69,10 @@ func (i *Installer) VerifyInstallation(client *kube.Client, result *verifier.Res
return nil
}

func requiredMigrations() []migration.Migrater {

func requiredMigrations(client *kube.Client, dryRun bool) []migration.Migrater {
// Determine which migrations to run
return []migration.Migrater{
// Implement actual migrations
_01_migrate_ov_names.New(client, dryRun),
}
}

Expand All @@ -88,13 +88,16 @@ func (i *Installer) PreUpgradeVerify(client *kube.Client, result *verifier.Resul
return nil
}

// Step 2 - Verify that any migration is possible
migrations := requiredMigrations()
// Step 2 - Verify that all migrations can run (with dryRun)
migrations := requiredMigrations(client, true)
clog.V(1).Printf("Verify that %d required migrations can be applied", len(migrations))
for _, m := range migrations {
if err := m.CanMigrate(client); err != nil {
if err := m.CanMigrate(); err != nil {
result.AddErrors(fmt.Errorf("migration %s failed install check: %v", m, err).Error())
}
if err := m.Migrate(); err != nil {
result.AddErrors(fmt.Errorf("migration %s failed dry-run: %v", m, err).Error())
}
}

// TODO: Verify existing operators and instances?
Expand All @@ -117,10 +120,10 @@ func (i *Installer) Upgrade(client *kube.Client) error {
}

// Step 5 - Execute Migrations
migrations := requiredMigrations()
migrations := requiredMigrations(client, false)
clog.Printf("Run %d migrations", len(migrations))
for _, m := range migrations {
if err := m.Migrate(client); err != nil {
if err := m.Migrate(); err != nil {
return fmt.Errorf("migration %s failed to execute: %v", m, err)
}
}
Expand Down
Loading