Install the manifest based on the version specified in knativeserving spec #102

houshengbo · 2020-05-26T20:07:59Z

Fixes #56
We can use the following CR to install knative serving 0.15.0.

apiVersion: operator.knative.dev/v1alpha1
kind: KnativeServing
metadata:
  name: knative-serving
  namespace: knative-serving
spec:
  version: 0.15.0

Proposed Changes

This PR bundles two versions of Knative Serving 0.14.0 and 0.15.0.
User can specify the version with knativeserving CRD spec.version.
Knative serving can upgrade from 0.14.0 to 0.15.0 and downgrade from 0.15.0 to 0.14.0.
If no version is specified, the default version is taken. The default version is retrieved by reading the names of the directories under kodata/knative-serving.

Release Note

knative-prow-robot · 2020-05-26T20:08:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: houshengbo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [houshengbo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow-robot

@houshengbo: 0 warnings.

In response to this:

Fixes #

Proposed Changes

Release Note

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

markusthoemmes · 2020-05-27T12:51:26Z

pkg/reconciler/knativeserving/knativeserving.go

 	if err != nil {
 		ks.Status.MarkInstallFailed(err.Error())
 		return err
 	}

+	// Find the common resources between the old and the current serving manifests
+	manifestApply := servingManifest.Filter(mf.In(oldManifest))


This probably shouldn't filter at all or we'll never create new resources.

Right, don't filter this at all.

jcrossley3

I think we can make the logic in here more clear.

jcrossley3 · 2020-05-27T19:49:06Z

pkg/reconciler/knativeserving/knativeserving.go

+	} else if len(servingManifest.Resources()) == 0 {
+		return fmt.Errorf("unable to find the manifest for the Knative Serving version %s", version.ServingVersion)
+	}


You perform this check every time you call retrieveManifest. Therefore, move this check into retrieveManifest and you can eliminate all the "else if..." in the callers.

jcrossley3 · 2020-05-27T19:54:14Z

pkg/reconciler/knativeserving/knativeserving.go

@@ -87,34 +99,76 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, original *servingv1alpha1
 // converge the two.
 func (r *Reconciler) ReconcileKind(ctx context.Context, ks *servingv1alpha1.KnativeServing) pkgreconciler.Event {
 	logger := logging.FromContext(ctx)
+	// Read the old version of the Knative Serving and the version of Knative Serving to be installed
+	oldVerion, newVersion := r.retrieveVersions(ks)


This is needlessly confusing. Instead of retrieveVersions how about two functions:

func (r *Reconciler) getTarget(ctx context.Context, instance *servingv1alpha1.KnativeServing) (mf.Manifest, error) {...} func (r *Reconciler) getCurrent(ctx context.Context, instance *servingv1alpha1.KnativeServing) (mf.Manifest, error) {...}

Encapsulate the logic of determining the proper version within each, including applying the appropriate transforms. The first function may, in fact, use status.version if spec.version is nil.

Yes, it is much more convenient to have two functions, wrapping the logic to verify the versions.

jcrossley3 · 2020-05-27T19:55:57Z

pkg/reconciler/knativeserving/knativeserving.go

+	} else if len(servingManifest.Resources()) == 0 {
+		return fmt.Errorf("unable to find the manifest for the Knative Serving version %s", newVersion)
+	}


jcrossley3 · 2020-05-27T19:56:11Z

pkg/reconciler/knativeserving/knativeserving.go

+	} else if len(oldManifest.Resources()) == 0 {
+		return fmt.Errorf("unable to find the previous manifest for the Knative Serving version %s", oldVerion)
+	}


jcrossley3 · 2020-05-27T19:57:11Z

pkg/reconciler/knativeserving/knativeserving.go

+	deleteStages := []func(context.Context, *mf.Manifest, *servingv1alpha1.KnativeServing) error{
 		r.deleteObsoleteResources,
 	}


This isn't necessary. We don't even need deleteObsoleteResources anymore.

jcrossley3 · 2020-05-27T20:01:13Z

pkg/reconciler/knativeserving/knativeserving.go

+	for _, stage := range deleteStages {
+		if err := stage(ctx, &manifestDelete, ks); err != nil {
 			return err
 		}
 	}


This can just be the following...

if err := oldManifest.Filter(mf.None(mf.In(servingManifest))).Delete(); err != nil { return err }

jcrossley3 · 2020-05-27T20:01:39Z

pkg/reconciler/knativeserving/knativeserving.go

 	if err != nil {
 		ks.Status.MarkInstallFailed(err.Error())
 		return err
 	}

+	// Find the common resources between the old and the current serving manifests
+	manifestApply := servingManifest.Filter(mf.In(oldManifest))
+	manifestDelete := servingManifest.Filter(mf.None(mf.In(oldManifest)))


This is backwards. Remove it.

jcrossley3 · 2020-05-27T20:03:04Z

pkg/reconciler/knativeserving/knativeserving.go

 	logger.Infow("Reconcile stages complete", "status", ks.Status)
 	return nil
 }

 // Transform the resources
-func (r *Reconciler) transform(ctx context.Context, instance *servingv1alpha1.KnativeServing) (mf.Manifest, error) {
+func (r *Reconciler) transform(ctx context.Context, instance *servingv1alpha1.KnativeServing, servingManifest mf.Manifest) (mf.Manifest, error) {


It's weird to pass a manifest only to transform it. Let's have transform return a []Transformer and let the caller deal with it.

pkg/reconciler/knativeserving/knativeserving.go

jcrossley3 · 2020-05-27T20:07:33Z

pkg/reconciler/knativeserving/knativeserving.go

+	return manifest, nil
+}
+
+func (r *Reconciler) retrieveVersions(instance *servingv1alpha1.KnativeServing) (string, string) {


It's confusing to force callers to deal with the possible empty versions strings here. I'd recommend a more "intentional" approach: a function to return the manifest i need, a function to return the manifest i have. Make sense?

This function is removed.

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

jcrossley3 · 2020-05-28T15:44:19Z

pkg/reconciler/knativeserving/knativeserving.go

@@ -74,7 +77,7 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, original *servingv1alpha1
 		}
 	}

-	manifest, err := r.transform(ctx, original)
+	manifest, err := r.getCurrentManifest(ctx, original)
 	if err != nil {
 		return fmt.Errorf("failed to transform manifest: %w", err)


I think I'd just return err here, as "transform" isn't exactly accurate, as long as you ensure proper errors are returned from getCurrentManifest

jcrossley3 · 2020-05-28T15:46:09Z

pkg/reconciler/knativeserving/knativeserving.go

 			return err
 		}
 	}
+
+	// Remove the resources, that does not exist in the new Serving manifest


Suggested change

// Remove the resources, that does not exist in the new Serving manifest

// Remove the resources that do not exist in the new Serving manifest

jcrossley3 · 2020-05-28T15:48:52Z

pkg/reconciler/knativeserving/knativeserving.go

 }

 // Apply the manifest resources
 func (r *Reconciler) install(ctx context.Context, manifest *mf.Manifest, instance *servingv1alpha1.KnativeServing) error {
 	logger := logging.FromContext(ctx)
 	logger.Debug("Installing manifest")
-	return common.Install(manifest, version.ServingVersion, &instance.Status)
+	return common.Install(manifest, instance.Spec.GetVersion(), &instance.Status)


Can instance.Spec.GetVersion() return "" here? Should the operator set it if the user doesn't? Will common.Install handle "" predictably?

Right, I need to makes sure there is a valid version.

jcrossley3 · 2020-05-28T15:49:20Z

pkg/reconciler/knativeserving/knativeserving.go

+	manifest, found := r.manifests[version]
+	if !found {
+		koDataDir := os.Getenv("KO_DATA_PATH")
+		manifesrDir := fmt.Sprintf("knative-serving/%s", version)


Suggested change

manifesrDir := fmt.Sprintf("knative-serving/%s", version)

manifestDir := fmt.Sprintf("knative-serving/%s", version)

jcrossley3 · 2020-05-28T15:50:57Z

pkg/reconciler/knativeserving/knativeserving.go

+	// Transform the manifest
+	transformers, err := r.transform(ctx, instance)
+	if err != nil {
+		return mf.Manifest{}, err
+	}
+
+	manifestTransformed, err := manifest.Transform(transformers...)
+	if err != nil {
+		return mf.Manifest{}, err
+	}


Not committed yet?

jcrossley3 · 2020-05-28T15:52:59Z

pkg/reconciler/knativeserving/knativeserving.go

+	logger := logging.FromContext(ctx)
+	var err error
+	manifest, found := r.manifests[version]
+	if !found {


This logic should really be in common since eventing will need it, too. At which point, you can delete version/version.go

jcrossley3 · 2020-05-28T15:53:59Z

version/version_test.go

 	"path/filepath"
 	"runtime"
 	"testing"

 	mf "github.com/manifestival/manifestival"
+	"knative.dev/operator/pkg/reconciler/common"
 )

 func TestManifestVersionServingSame(t *testing.T) {


We should get rid of this file entirely.

…is set

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

pkg/reconciler/knativeserving/knativeserving.go

jcrossley3 · 2020-05-28T22:45:17Z

/hold

knative-prow-robot · 2020-05-29T01:48:00Z

@houshengbo: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-operator-go-coverage	`fdf4380`	link	`/test pull-knative-operator-go-coverage`
pull-knative-operator-integration-tests	`fdf4380`	link	`/test pull-knative-operator-integration-tests`
pull-knative-operator-upgrade-tests	`fdf4380`	link	`/test pull-knative-operator-upgrade-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

houshengbo · 2020-06-01T13:49:23Z

This PR points to the direction of how spec.version is implemented.
I will start to break it into separate PRs.
Also, some foundational PRs will be opened to refactor the test cases and test scripts.
@jcrossley3 @markusthoemmes @evankanderson

knative-prow-robot · 2020-06-02T02:44:10Z

@houshengbo: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot requested review from jcrossley3 and markusthoemmes May 26, 2020 20:08

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 26, 2020

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 26, 2020

knative-prow-robot reviewed May 26, 2020

View reviewed changes

mattmoor reviewed May 26, 2020

View reviewed changes

houshengbo force-pushed the load-manifest-version branch from 9002e46 to d544f7c Compare May 26, 2020 21:40

mattmoor reviewed May 26, 2020

View reviewed changes

houshengbo changed the title ~~Load manifest version~~ Install the manifest based on the version specified in knativeserving spec May 26, 2020

houshengbo changed the title ~~Install the manifest based on the version specified in knativeserving spec~~ WIP: Install the manifest based on the version specified in knativeserving spec May 26, 2020

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 26, 2020

houshengbo force-pushed the load-manifest-version branch from c2c06f8 to fc90c53 Compare May 26, 2020 22:00

mattmoor reviewed May 26, 2020

View reviewed changes

houshengbo force-pushed the load-manifest-version branch 2 times, most recently from 8fd24dd to 8dac9b7 Compare May 27, 2020 02:55

mattmoor reviewed May 27, 2020

View reviewed changes

houshengbo force-pushed the load-manifest-version branch from 8dac9b7 to f912ec1 Compare May 27, 2020 03:05

mattmoor reviewed May 27, 2020

View reviewed changes

markusthoemmes reviewed May 27, 2020

View reviewed changes

jcrossley3 suggested changes May 27, 2020

View reviewed changes

Add the functions to read releases from the kodata directory

b3d2828

houshengbo force-pushed the load-manifest-version branch from f912ec1 to 54e0d5f Compare May 27, 2020 21:55

mattmoor reviewed May 27, 2020

View reviewed changes

Vincent Hou added 2 commits May 27, 2020 18:02

Add the mfClient into the reconciler

0a4f714

Put different knative serving manifests into different directories

312bbbe

Save the non-transformed manifest

6b920e5

mattmoor reviewed May 28, 2020

View reviewed changes

houshengbo force-pushed the load-manifest-version branch 2 times, most recently from 0543b87 to f86b0fd Compare May 28, 2020 13:46

Remove ServingVersion

f86b0fd

houshengbo mentioned this pull request May 28, 2020

WIP: Get the latest patch release based on a provided SemVer #103

Closed

jcrossley3 suggested changes May 28, 2020

View reviewed changes

Vincent Hou added 3 commits May 28, 2020 16:01

Read the current manifest as the target manifest, if no spec.version …

80aa20b

…is set

Remove version directory

f1c01ef

Fix the PR to resolve all comments

2ac14e1

mattmoor reviewed May 28, 2020

View reviewed changes

houshengbo force-pushed the load-manifest-version branch from 790f21a to 7746d6b Compare May 28, 2020 22:28

mattmoor reviewed May 28, 2020

View reviewed changes

pkg/reconciler/knativeserving/knativeserving.go Show resolved Hide resolved

houshengbo force-pushed the load-manifest-version branch 2 times, most recently from ccda62b to 23054ab Compare May 28, 2020 22:43

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 28, 2020

Fix version issue for Knative Eventing after removing version directory

fdf4380

houshengbo force-pushed the load-manifest-version branch from 23054ab to fdf4380 Compare May 29, 2020 01:15

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2020

houshengbo closed this Jul 6, 2020

	// Remove the resources, that does not exist in the new Serving manifest
	// Remove the resources that do not exist in the new Serving manifest

	manifesrDir := fmt.Sprintf("knative-serving/%s", version)
	manifestDir := fmt.Sprintf("knative-serving/%s", version)

Install the manifest based on the version specified in knativeserving spec #102

Install the manifest based on the version specified in knativeserving spec #102

Conversation

houshengbo commented May 26, 2020 • edited Loading

Proposed Changes

knative-prow-robot commented May 26, 2020

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcrossley3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcrossley3 May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

jcrossley3 commented May 28, 2020

knative-prow-robot commented May 29, 2020

houshengbo commented Jun 1, 2020

knative-prow-robot commented Jun 2, 2020

houshengbo commented May 26, 2020 •

edited

Loading

jcrossley3 May 27, 2020 •

edited

Loading