Fixed issue with compat/client with applying empty structures to api server #454

Danil-Grigorev · 2020-03-17T23:30:32Z

Re-implementing functionality of #443

Added downconvert for cronjobs

…server - Added downconvert to cronjobs

Danil-Grigorev · 2020-03-19T18:08:11Z

@jortel This should be a smaller scoped PR, only addressing the issue with resource name being empty. Please review.

{"level":"info","ts":1584511208.2217383,"logger":"migration|j8589","msg":"[RUN]","migration":"mssql-app-mig-1584510245","stage":false,"phase":"QuiesceApplications"}
{"level":"error","ts":1584511209.0168467,"logger":"migration|j8589","msg":"","migration":"mssql-app-mig-1584510245","error":"resource name may not be empty","stacktrace":"github.com/konveyor/mig-controller/vendor/github.com/go-logr/zapr.(*zapLogger).Error\n\t/go/src/github.com/konveyor/mig-controller/vendor/github.com/go-logr/zapr/zapr.go:128\ngithub.com/konveyor/mig-controller/pkg/logging.Logger.Error\n\t/go/src/github.com/konveyor/mig-controller/pkg/logging/logger.go:75\ngithub.com/konveyor/mig-controller/pkg/logging.Logger.Trace\n\t/go/src/github.com/konveyor/mig-controller/pkg/logging/logger.go:81\ngithub.com/konveyor/mig-controller/pkg/controller/migmigration.(*Task).scaleDownDeployments\n\t/go/src/github.com/konveyor/mig-controller/pkg/controller/migmigration/quiesce.go:110\ngithub.com/konveyor/mig-controller/pkg/controller/migmigration.(*Task).quiesceApplications\n\t/go/src/github.com/konveyor/mig-controller/pkg/controller/migmigration/quiesce.go:31\ngithub.com/konveyor/mig-controller/pkg/controller/migmigration.(*Task).Run\n\t/go/src/github.com/konveyor/mig-controller/pkg/controller/migmigration/task.go:283\ngithub.com/konveyor/mig-controller/pkg/controller/migmigration.(*ReconcileMigMigration).migrate\n\t/go/src/github.com/konveyor/mig-controller/pkg/controller/migmigration/migrate.go:78\ngithub.com/konveyor/mig-controller/pkg/controller/migmigration.(*ReconcileMigMigration).Reconcile\n\t/go/src/github.com/konveyor/mig-controller/pkg/controller/migmigration/migmigration_controller.go:195\ngithub.com/konveyor/mig-controller/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/go/src/github.com/konveyor/mig-controller/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:215\ngithub.com/konveyor/mig-controller/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func1\n\t/go/src/github.com/konveyor/mig-controller/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:158\ngithub.com/konveyor/mig-controller/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil.func1\n\t/go/src/github.com/konveyor/mig-controller/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:133\ngithub.com/konveyor/mig-controller/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil\n\t/go/src/github.com/konveyor/mig-controller/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:134\ngithub.com/konveyor/mig-controller/vendor/k8s.io/apimachinery/pkg/util/wait.Until\n\t/go/src/github.com/konveyor/mig-controller/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:88"}

jortel · 2020-03-19T18:13:14Z

pkg/compat/client.go

@@ -109,11 +122,9 @@ func (c Client) Get(ctx context.Context, key k8sclient.ObjectKey, in runtime.Obj
 	if err != nil {
 		return err
 	}
-	if in != obj {


The purpose for this check is so that we don't up-convert the result unless it was down-converted.

I think it is already being done in the scheme.Convert

Drilling down in the code, it seems it's still doing some work we could/should avoid. Thoughts?
The check provides some symmetry in that we conditionally convert as needed both up and down.

jortel · 2020-03-19T18:14:44Z

pkg/compat/client.go

@@ -151,6 +164,10 @@ func (c Client) Create(ctx context.Context, in runtime.Object) error {
 // The resource will be converted to a compatible version as needed.
 func (c Client) Delete(ctx context.Context, in runtime.Object, opt ...k8sclient.DeleteOptionFunc) error {
 	obj := c.downConvert(in)
+	err := scheme.Scheme.Convert(in, obj, ctx)


Seems like doing the down-conversion in the downConvert() method would make more sense.

jortel · 2020-03-19T18:15:52Z

pkg/compat/client.go

 	return c.Client.Update(ctx, obj)
 }
-
-func (c Client) Status() k8sclient.StatusWriter {


I thought this was needed to implement the Client interface. No?

It is inheriting that from the client inside the structure.

jortel · 2020-03-19T18:18:33Z

pkg/controller/migmigration/quiesce.go

 				continue
 			}
-			unstructured.SetNestedField(r.Object, true, fields...)
+			r.Spec.Suspend = pointer.BoolPtr(true)


👍 Using pointer.BoolPtr() is so much cleaner than using a bool variable.

jortel · 2020-03-20T14:33:15Z

pkg/compat/client.go

 	if c.Minor < 16 {
+		switch obj.(type) {


Cast to Type and switch statement is a nice improvement.

jortel · 2020-03-20T17:37:17Z

@Danil-Grigorev These changes look really good but no longer include a few changes I recall seeing in earlier versions of the PR. Mainly files related to generating the conversions. Earlier you added the conversion-gen to our Makefile but no longer see that diff. Also, IIRC you added a subpackage to our compat package with generated conversions that I no longer see in the diff.
Are the additions to /vendor still needed?

Danil-Grigorev · 2020-03-23T14:30:33Z

@Danil-Grigorev These changes look really good but no longer include a few changes I recall seeing in earlier versions of the PR. Mainly files related to generating the conversions. Earlier you added the conversion-gen to our Makefile but no longer see that diff. Also, IIRC you added a subpackage to our compat package with generated conversions that I no longer see in the diff.

Look into PR #460, those two PRs are addressing different issues.

Are the additions to /vendor still needed?

Yes, they are needed for BoolPtr methods.

@jortel Tested this in a full setup, everything works.

jortel

LGTM

…server (#454) * Fixed issue with compat/client with applying empty structures to api server - Added downconvert to cronjobs * Added up and down conversion functions (cherry picked from commit 424d13a)

Danil-Grigorev added the PoC label Mar 17, 2020

Danil-Grigorev requested review from sseago and jortel March 17, 2020 23:30

Danil-Grigorev removed the PoC label Mar 18, 2020

Danil-Grigorev force-pushed the downscale-conversion-fix branch from 79bee55 to 490e791 Compare March 18, 2020 14:12

Danil-Grigorev changed the title ~~[WIP] Added generated conversion function for appsv1~~ Added generated conversion function for appsv1 Mar 18, 2020

Danil-Grigorev marked this pull request as ready for review March 18, 2020 14:20

Danil-Grigorev added the PoC label Mar 19, 2020

Fixed issue with compat/client with applying empty structures to api …

25d6fdd

…server - Added downconvert to cronjobs

Danil-Grigorev force-pushed the downscale-conversion-fix branch from 490e791 to 25d6fdd Compare March 19, 2020 17:56

Danil-Grigorev changed the title ~~Added generated conversion function for appsv1~~ Fixed issue with compat/client with applying empty structures to api server Mar 19, 2020

Danil-Grigorev added kind/bug Categorizes issue or PR as related to a bug. and removed PoC labels Mar 19, 2020

jortel reviewed Mar 19, 2020

View reviewed changes

Added up and down conversion functions

0cdf840

Danil-Grigorev force-pushed the downscale-conversion-fix branch from 4015159 to 0cdf840 Compare March 20, 2020 00:10

Danil-Grigorev requested a review from jortel March 20, 2020 00:10

Danil-Grigorev added the release-1.2.0 label Mar 20, 2020

jortel reviewed Mar 20, 2020

View reviewed changes

pkg/compat/client.go

if c.Minor < 16 {

switch obj.(type) {

Copy link

Contributor

jortel Mar 20, 2020 •

edited

Choose a reason for hiding this comment

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

Cast to Type and switch statement is a nice improvement.

jortel approved these changes Mar 23, 2020

View reviewed changes

Danil-Grigorev merged commit 424d13a into migtools:master Mar 23, 2020

Danil-Grigorev deleted the downscale-conversion-fix branch March 23, 2020 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed issue with compat/client with applying empty structures to api server #454

Fixed issue with compat/client with applying empty structures to api server #454

Danil-Grigorev commented Mar 17, 2020 •

edited

Danil-Grigorev commented Mar 19, 2020 •

edited

jortel Mar 19, 2020

Danil-Grigorev Mar 19, 2020

jortel Mar 19, 2020 •

edited

Danil-Grigorev Mar 20, 2020

jortel Mar 19, 2020 •

edited

Danil-Grigorev Mar 19, 2020

jortel Mar 19, 2020

Danil-Grigorev Mar 19, 2020

jortel Mar 19, 2020

jortel Mar 20, 2020 •

edited

jortel commented Mar 20, 2020 •

edited

Danil-Grigorev commented Mar 23, 2020 •

edited

jortel left a comment

Fixed issue with compat/client with applying empty structures to api server #454

Fixed issue with compat/client with applying empty structures to api server #454

Conversation

Danil-Grigorev commented Mar 17, 2020 • edited

Danil-Grigorev commented Mar 19, 2020 • edited

jortel Mar 19, 2020

Choose a reason for hiding this comment

Danil-Grigorev Mar 19, 2020

Choose a reason for hiding this comment

jortel Mar 19, 2020 • edited

Choose a reason for hiding this comment

Danil-Grigorev Mar 20, 2020

Choose a reason for hiding this comment

jortel Mar 19, 2020 • edited

Choose a reason for hiding this comment

Danil-Grigorev Mar 19, 2020

Choose a reason for hiding this comment

jortel Mar 19, 2020

Choose a reason for hiding this comment

Danil-Grigorev Mar 19, 2020

Choose a reason for hiding this comment

jortel Mar 19, 2020

Choose a reason for hiding this comment

jortel Mar 20, 2020 • edited

Choose a reason for hiding this comment

jortel commented Mar 20, 2020 • edited

Danil-Grigorev commented Mar 23, 2020 • edited

jortel left a comment

Choose a reason for hiding this comment

Danil-Grigorev commented Mar 17, 2020 •

edited

Danil-Grigorev commented Mar 19, 2020 •

edited

jortel Mar 19, 2020 •

edited

jortel Mar 19, 2020 •

edited

jortel Mar 20, 2020 •

edited

jortel commented Mar 20, 2020 •

edited

Danil-Grigorev commented Mar 23, 2020 •

edited