feat: Add service import command by dsimansk · Pull Request #1065 · knative/client

dsimansk · 2020-10-15T15:14:41Z

Description

The PR is missing any kind of test coverage, TBD. I'd like to gather early feedback on the implementation approach here.

Changes

Add service import command

Reference

Fixes #654

/lint
/hold
/cc @rhuss @navidshaikh

knative-prow-robot

@dsimansk: 4 warnings.

Details

In response to this:

Description

The PR is missing any kind of test coverage, TBD. I'd like to gather early feedback on the implementation approach here.

Changes

Add service import command

Reference

Fixes #654

/lint
/hold
/cc @rhuss @navidshaikh

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 · 2020-10-15T15:14:51Z

 	return call.Result[0].(*servingv1.Configuration), mock.ErrorOrNil(call.Result[1])
 }

+// Create a new revision


Golint comments: comment on exported method ServingRecorder.CreateRevision should be of the form "CreateRevision ...". More info.

knative-prow-robot · 2020-10-15T15:14:52Z

+	return mock.ErrorOrNil(call.Result[0])
+}
+
+// Update the given revision


Golint comments: comment on exported method ServingRecorder.UpdateRevision should be of the form "UpdateRevision ...". More info.

knative-prow-robot · 2020-10-15T15:14:52Z

+	sr.r.Add("UpdateRevision", []interface{}{revision}, []interface{}{err})
+}
+
+func (c *MockKnServingClient) UpdateRevision(revision *servingv1.Revision) error {


Golint comments: exported method MockKnServingClient.UpdateRevision should have comment or be unexported. More info.

itsmurugappan · 2020-10-15T20:29:05Z

looks good. Have two questions /comments

Will this is only support only Export kind ? If I export in kubectl friendly style, will I not be able to use the import functionality
I see initial service without traffic and traffic getting updated later. IIRC traffic with missing revisions should not cause the ksvc to fail. Could save you an additional step of updating the traffic.

maximilien

/ok-to-test

Nice start. I wonder if `—dry-runs option would be nice whereby the import verifies but does not deploy. Maybe as a follow on. Anyhow, love seeing this whole export/import working out. An obvious e2e test will be something like:

kn service create testSvc —image ...
...
kn service export testSvc test.yml
...
kn service delete testSvc
...
kn service import testSvc test.yml

And end result is the same as first step :)

dsimansk · 2020-10-19T10:34:53Z

1. Will this is only support only Export kind ? If I export in kubectl friendly style, will I not be able to use the import functionality

Per our conversation with @rhuss kn shouldn't duplicate functionality as such export is already usable with kubectl. Therefore kn service import should work with its own Export format. Please @rhuss correct me if I'm wrong here.

2. I see initial service without traffic and traffic getting updated later. IIRC traffic with missing revisions should not cause the ksvc to fail. Could save you an additional step of updating the traffic.

That's intentional to avoid error states. During my local testing I've seen intermittent failures during wait phase. Those might have been caused by waiting issue that was recently fixed. Thanks for the suggestion, I'll double check if there's still an issue or not.

Nice start. I wonder if `—dry-runs option would be nice whereby the import verifies but does not deploy. Maybe as a follow on. Anyhow, love seeing this whole export/import working out. An obvious e2e test will be something like:

Thanks for the suggestion, --dry-run sounds useful especially in this context.

itsmurugappan · 2020-10-24T20:10:10Z

+			tmp.OwnerReferences = []metav1.OwnerReference{
+				{
+					APIVersion:         currentConf.APIVersion,
+					Kind:               currentConf.Kind,
+					Name:               currentConf.Name,
+					UID:                currentConf.UID,
+					Controller:         ptr.Bool(true),
+					BlockOwnerDeletion: ptr.Bool(true),


pls check whether you can use the kmeta package to set the owner reference. Below is from the serving repo
https://github.com/knative/serving/blob/5233f0c607c5a89d6a58d6faac9eed57b4877b79/pkg/reconciler/revision/resources/deploy.go#L253

Nice find, thanks! It's exactly the function needed here.

rhuss · 2020-10-27T09:49:58Z

@dsimansk please let me know when this PR is ready from your side (so no WIP anymore), I would then jump on the review, too.

dsimansk · 2020-10-30T11:25:58Z

@rhuss ready now. 😺

/unhold

dsimansk · 2020-10-30T11:42:39Z

@rhuss @navidshaikh @itsmurugappan

There's one scenario I'm not really sure how to handle.

Let's have simple hello ksvc without additional revisions
Exec: kn service export hello -o yaml --mode export

Cmd output

➜  client git:(service-import) kn service export hello -o yaml --mode export                                  
apiVersion: serving.knative.dev/v1
kind: Service
metadata:
  creationTimestamp: null
  name: hello
spec:
  template:
    metadata:
      annotations:
        client.knative.dev/user-image: docker.io/dsimansk/helloworld
      creationTimestamp: null
      name: hello-djrrc-1
    spec:
      containerConcurrency: 0
      containers:
      - env:
        - name: TARGET
          value: v1
        image: docker.io/dsimansk/helloworld
        name: user-container
        readinessProbe:
          successThreshold: 1
          tcpSocket:
            port: 0
        resources: {}
      enableServiceLinks: false
      timeoutSeconds: 300
status: {}

For this case --mode flag is ignored and plain ksvc output is produces
As a result there's no usable Export type object for the import to use.
- If import would support plain ksvc yaml than there's a duplicite functionality with service create --filename, service apply and kubectl.
Exec: kn service export hello -o yaml --mode export --with-revisions
- Adding --with-revisions will produce requested Export type

Cmd output

➜  client git:(service-import) kn service export hello -o yaml --mode export --with-revisions
apiVersion: client.knative.dev/v1alpha1
kind: Export
metadata:
  creationTimestamp: null
spec:
  revisions: null
  service:
    apiVersion: serving.knative.dev/v1
    kind: Service
    metadata:
      creationTimestamp: null
      name: hello
    spec:
      template:
        metadata:
          annotations:
            client.knative.dev/user-image: docker.io/dsimansk/helloworld
          creationTimestamp: null
          name: hello-djrrc-1
        spec:
          containerConcurrency: 0
          containers:
          - env:
            - name: TARGET
              value: v1
            image: docker.io/dsimansk/helloworld
            name: user-container
            readinessProbe:
              successThreshold: 1
              tcpSocket:
                port: 0
            resources: {}
          enableServiceLinks: false
          timeoutSeconds: 300
    status: {}

It feels like inconsistent behaviour of --mode flag, that's actually depending on --with-revisions to produce expected result.

dsimansk · 2020-10-30T12:59:58Z

        🦆 kn service import /tmp/kn-file993807706/foo-with-revisions --namespace kne2etests16
        ┃ 
        🔥 Error: configurations.serving.knative.dev "foo" not found
        🔥 Run 'kn --help' for usage
        🔥

https://github.com/knative/client/pull/1065/files#diff-89bebfee01d13203ce4186956ccc919b30072d4ca985d3c5b054336ce0c735f0R105-R109

It seems that controller was still handling the subsequent creation of Configuration for the Service.
Would you mind something like GetConfigurationWithRetry() or watch?

itsmurugappan · 2020-10-31T20:03:46Z

It feels like inconsistent behaviour of --mode flag, that's actually depending on --with-revisions to produce expected result.

IMO this is a bug on kn service export, whenever the mode is export , kn should give a yaml in the Export kind instead of KSVC kind.

@rhuss @navidshaikh please pitch in.

dsimansk · 2020-11-02T12:24:03Z

I've opened a new issue to track export bug. #1087

dsimansk · 2020-11-03T11:37:40Z

+	// Retrieve current Configuration to be use in OwnerReference
+	retries := 0
+	var currentConf *servingv1.Configuration
+	for {
+		currentConf, err = client.GetConfiguration(serviceName)
+		if err != nil {
+			if apierrors.IsNotFound(err) && retries < 5 {
+				retries++
+				time.Sleep(time.Second)
+				continue
+			} else {
+				return err
+			}
+		}
+		break
+	}


I've added a simple retry-loop to retrieve Configuration to verify it it's causing e2e test to fail. The retry can be moved to client like UpdateServiceWithRetry. Let me know if there are any objections to actual retry or it could be done differently.

The code should resolve the following error.

There's also retry.OnError from client-go utils that's usable here, if there's a need for more sophisticated approach.

I am with fine this approach;

beside I think we can separate this retry logic in a dedicated function, but then if we'd to separate it out anyway we can consume the clint-go utils you linked. Infact we should consider using these utils wherever we find a fit to encourage consumption from these standard libs we are vendoring. (all this can be done in a subsequent iteration if thats feasible).

I like the idea of using standard libs. I've pushed an updated version in 7b8317c. We can start small here and incrementally update UpdateServiceWithRetry or any other instances.

navidshaikh

suggested a few nits and a wait for service related question, also please add CHANGELOG entry

navidshaikh · 2020-11-04T04:39:36Z

+### Options
+
+```
+      --async              DEPRECATED: please use --no-wait instead. Do not wait for 'service import' operation to be completed.


I think we can stop --async support now, it has been 3+ releases since we introduced --no-wait?

navidshaikh · 2020-11-04T04:41:28Z

+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"knative.dev/pkg/kmeta"
+	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
+
+	"github.com/spf13/cobra"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/yaml"
+	"knative.dev/client/pkg/kn/commands"
+	clientservingv1 "knative.dev/client/pkg/serving/v1"
+
+	clientv1alpha1 "knative.dev/client/pkg/apis/client/v1alpha1"


regroup wrt third party and knative.dev/client imports?

navidshaikh · 2020-11-04T04:42:07Z

+		Use:   "import FILENAME",
+		Short: "Import a service and its revisions",
+		Example: `
+ # Import a service in YAML format


Suggested change

# Import a service in YAML format

# Import a service from YAML format

navidshaikh · 2020-11-04T04:42:21Z

+ # Import a service in YAML format
+ kn service import /path/to/file.yaml
+
+ # Import a service in JSON format


Suggested change

# Import a service in JSON format

# Import a service from JSON format

navidshaikh · 2020-11-04T04:44:44Z

+		return err
+	}
+	if export.Spec.Service.Name == "" {
+		return fmt.Errorf("provided import doesn't content service name, please note that only kn' custom Export format is supported")


Suggested change

return fmt.Errorf("provided import doesn't content service name, please note that only kn' custom Export format is supported")

return fmt.Errorf("provided import file doesn't contain service name, please note that only kn's custom Export format is supported")

navidshaikh · 2020-11-04T08:27:29Z

+	// Retrieve current Configuration to be use in OwnerReference
+	retries := 0
+	var currentConf *servingv1.Configuration
+	for {
+		currentConf, err = client.GetConfiguration(serviceName)
+		if err != nil {
+			if apierrors.IsNotFound(err) && retries < 5 {
+				retries++
+				time.Sleep(time.Second)
+				continue
+			} else {
+				return err
+			}
+		}
+		break
+	}


I am with fine this approach;

beside I think we can separate this retry logic in a dedicated function, but then if we'd to separate it out anyway we can consume the clint-go utils you linked. Infact we should consider using these utils wherever we find a fit to encourage consumption from these standard libs we are vendoring. (all this can be done in a subsequent iteration if thats feasible).

navidshaikh · 2020-11-04T08:29:19Z

+	// Create revision with current Configuration's OwnerReference
+	if len(export.Spec.Revisions) > 0 {
+		for _, r := range export.Spec.Revisions {
+			tmp := r


I think we should do deep copy here?

navidshaikh · 2020-11-04T08:32:52Z

+		}
+	}
+
+	err = waitIfRequested(client, serviceName, waitFlags, "Importing", "imported", out)


a service has 5 revisions among which 2 are active (configured in traffic block); will service still report true if any of those 3 non-active revisions failed to become ready ?

The current service export impl doesn't produce such export output, only active revisions are included.

I wonder whether we should allow later in service export, too, to export all revisions, so that the import then also would import the history. But let's leave that question for future refinement of this feature.

navidshaikh · 2020-11-04T08:33:29Z

+	if err != nil {
+		return err
+	}
+
+	return nil


Suggested change

if err != nil {

return err

}

return nil

return err

rhuss

Thanks ! Looks good in general, but I'm not yet 100% sure how it behaves in the wild. E.g. whether the ordering is correct (which is also reflected in kn revision list). I'm also not sure if the export includes the latest revision generated from the service in the revision list, because in this case we do have an issue, as the initial service creation implicitly also creates a revision.

We definitely require an E2E test for the long run, to verify the full behaviour. But for now I would be happy to merge this, but also would mark the command as "experimental" (would add this to the changelog.adoc and the command description)

rhuss · 2020-11-05T10:32:27Z

+			serviceName, client.Namespace())
+	}
+
+	err = client.CreateService(&export.Spec.Service)


when we create a service like this, then an initial revision is created. This is fine, but I think we might get into ordering issues as the generation of this revision would be 1 (and which might conflict with the revision history that we are importing, where there would be also 1 generation). At least this is what I suspect. (would need to be verified)

Well, your suspicion is exactly on point here, revision history is mangled after import. I'll try to double-check if there's an alternative to the current import order.

➜ client git:(service-import) kn revision list NAME SERVICE TRAFFIC TAGS GENERATION AGE CONDITIONS READY REASON hello-xbgkn-2 hello 25% 2 60s 4 OK / 4 True hello-brqcf-3 hello 50% 1 60s 4 OK / 4 True hello-kphzr-1 hello 25% 1 60s 4 OK / 4 True

Currently revision list is sorted by

// revisionListSortFunc sorts by namespace, service, generation and name

In fact it's Configuration's generation, here. That's in line with revision history in my comment above. However, I don't see any clean way how to reproduce the history in the import wihout leaning towards "replay" approach actually.

rhuss · 2020-11-05T10:34:26Z

+		}
+	}
+
+	err = waitIfRequested(client, serviceName, waitFlags, "Importing", "imported", out)


I wonder whether we should allow later in service export, too, to export all revisions, so that the import then also would import the history. But let's leave that question for future refinement of this feature.

rhuss · 2020-11-05T10:38:18Z

+	r.GetService("foo", nil, nil)
+	_, err = executeServiceCommand(client, "import", file)
+
+	assert.ErrorContains(t, err, "because the service already exists")


In general, for tests, I would assert that all required context information is included, i.e. here the service name and maybe the namespace (if this is part of the error message). The exact wording is secondary.

This is true for the other checks below, too, that check for some messages

rhuss · 2020-11-05T10:42:33Z

 	return call.Result[0].(*servingv1.Configuration), mock.ErrorOrNil(call.Result[1])
 }

+// CreateRevision records a call CreateRevision


Could you please also add a call to the recorder and the mock methods to client_mock_test ? This keeps the test coverage happy :)

Sure, I've missed that.

maximilien

LGTM

rhuss

Beside the issue of building up revision with the proper generation ordering, this PR looks good to me.

I suggest, to merge this PR as now (it's still marked as experimental), but think about another solution after this.

E.g would it be possible to remove the revision that has been created implicitly when we created the Service and after we have added the revision list ? Also we could experiment with building up a Configuration first and only add the Service as a final step (with the same name as the Configuration, as this is the correlation between Service and Configuration). Also, the ownerReference needs between Service and Configuration then would be needed to be set as the last step.

wdyt, should we move forward now ?

Co-authored-by: Roland Huß <rhuss@redhat.com>

knative-metrics-robot · 2020-11-09T10:28:01Z

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/import.go	Do not exist	80.0%
pkg/kn/commands/service/service.go	91.3%	91.7%	0.4
pkg/serving/v1/client.go	68.0%	65.4%	-2.7
pkg/serving/v1/client_mock.go	93.7%	94.2%	0.6

dsimansk · 2020-11-09T11:29:42Z

@rhuss sure, sounds good to me.

I've been experimenting with temp initial revision to recreate generation history correctly without too much luck yet. I can explore the options further, but it shouldn't prevent "experimental" import to be merged now.

rhuss · 2020-11-09T14:03:19Z

Ok, then let's go for it

/Lgtm
/Approve

knative-prow-robot · 2020-11-09T14:03:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, maximilien, rhuss

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [maximilien,rhuss]

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 requested review from navidshaikh and rhuss October 15, 2020 15:14

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 15, 2020

knative-prow-robot reviewed Oct 15, 2020

View reviewed changes

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 15, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 15, 2020

maximilien reviewed Oct 15, 2020

View reviewed changes

knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 15, 2020

itsmurugappan reviewed Oct 24, 2020

View reviewed changes

dsimansk force-pushed the service-import branch from 289b707 to b013632 Compare October 30, 2020 11:24

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 30, 2020

dsimansk changed the title ~~WIP: feat: Add service import command~~ feat: Add service import command Oct 30, 2020

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

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

dsimansk force-pushed the service-import branch from 4b8b24f to 79bbf1e Compare November 3, 2020 11:29

dsimansk commented Nov 3, 2020

View reviewed changes

navidshaikh reviewed Nov 4, 2020

View reviewed changes

navidshaikh added this to the v0.19.0 milestone Nov 5, 2020

rhuss reviewed Nov 5, 2020

View reviewed changes

maximilien approved these changes Nov 5, 2020

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2020

rhuss reviewed Nov 9, 2020

View reviewed changes

Comment thread CHANGELOG.adoc Outdated

dsimansk and others added 11 commits November 9, 2020 11:22

feat: Add service import command

24544fd

fix: Fix e2e test

249dcfd

fix: Add retry when retrieving Configuration

7e07b3f

fix: Reflect review feedback

eb7951d

fix: Fix error message

37907b8

Co-authored-by: Roland Huß <rhuss@redhat.com>

fix: Add missing mock tests

dcb83b0

fix: Polish unit test assertions

12ad981

fix: Mark import as experimental

0e94562

chore: Add changelog entry

86b4441

Update CHANGELOG.adoc

179583f

Co-authored-by: Roland Huß <rhuss@redhat.com>

fix: Remove deprecated flag

e29755d

dsimansk force-pushed the service-import branch from b175789 to e29755d Compare November 9, 2020 10:24

knative-prow-robot assigned rhuss Nov 9, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 9, 2020

knative-prow-robot merged commit b72e4be into knative:master Nov 9, 2020

	# Import a service in YAML format
	# Import a service from YAML format

	# Import a service in JSON format
	# Import a service from JSON format

	return fmt.Errorf("provided import doesn't content service name, please note that only kn' custom Export format is supported")
	return fmt.Errorf("provided import file doesn't contain service name, please note that only kn's custom Export format is supported")

Conversation

dsimansk commented Oct 15, 2020

Description

Changes

Reference

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itsmurugappan commented Oct 15, 2020

Uh oh!

maximilien left a comment

Choose a reason for hiding this comment

Uh oh!

dsimansk commented Oct 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhuss commented Oct 27, 2020

Uh oh!

dsimansk commented Oct 30, 2020

Uh oh!

dsimansk commented Oct 30, 2020

Uh oh!

dsimansk commented Oct 30, 2020

Uh oh!

itsmurugappan commented Oct 31, 2020

Uh oh!

dsimansk commented Nov 2, 2020

Uh oh!

dsimansk Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navidshaikh Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navidshaikh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navidshaikh Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dsimansk Nov 3, 2020 •

edited

Loading

navidshaikh Nov 4, 2020 •

edited

Loading

navidshaikh Nov 4, 2020 •

edited

Loading

dsimansk Nov 5, 2020 •

edited

Loading