Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update: Change default to server-side generated revision names #1236

Conversation

rhuss
Copy link
Contributor

@rhuss rhuss commented Feb 22, 2021

Fixes #1144

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 22, 2021
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 22, 2021
@dsimansk
Copy link
Contributor

/retest

@dsimansk
Copy link
Contributor

dsimansk commented Feb 22, 2021

@rhuss E2E will need updates as well.

This test fails due to "same change" isn't generating new revision. I'll look into a bit to spot other issues.
https://github.com/knative/client/blob/master/test/e2e/revision_test.go#L53-L54

Missing revision name in update.
https://github.com/knative/client/blob/master/test/e2e/traffic_split_test.go#L291-L295

Changing service visibility doesn't produce new revision also.

service 'hello-private-public' with latest revision 'hello-private-public-00001' (unchanged) is available at url:

https://github.com/knative/client/blob/master/test/e2e/service_test.go#L96-L99

Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

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

/ok-to-test
/lgtm

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. lgtm Indicates that a PR is ready to be merged. labels Feb 22, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien

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:

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 knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2021
@rhuss
Copy link
Contributor Author

rhuss commented Feb 23, 2021

This test fails due to "same change" isn't generating new revision. I'll look into a bit to spot other issues.
https://github.com/knative/client/blob/master/test/e2e/revision_test.go#L53-L54

Interesting that there is a timeout. Regardless what, that should never happen.

@dsimansk
Copy link
Contributor

This test fails due to "same change" isn't generating new revision. I'll look into a bit to spot other issues.
https://github.com/knative/client/blob/master/test/e2e/revision_test.go#L53-L54

Interesting that there is a timeout. Regardless what, that should never happen.

Indeed the timeout is caused by out waiting function. In case of executing same kn service update twice there's no resource change on server side, therefore out wait will hange waiting for next MODIFIED event that's never received.

First successful update

➜  client git:(pr/switch-to-server-generated-revision-names-by-default) ✗ kn service update hello --env TARGET="v2"                                      
Updating Service 'hello' in namespace 'default':

Event type: ADDED
Event type: MODIFIED
  0.058s The Configuration is still working to reflect the latest desired specification.
Event type: MODIFIED
Event type: MODIFIED
  5.835s Traffic is not yet migrated to the latest revision.
Event type: MODIFIED
  5.891s Ingress has not yet been reconciled.
Event type: MODIFIED
  5.947s Waiting for load balancer to be ready
Event type: MODIFIED
  6.112s Ready to serve.

Service 'hello' updated to latest revision 'hello-00002' is available at URL:
http://hello.default.example.com

The second one blocked waiting:

➜  client git:(pr/switch-to-server-generated-revision-names-by-default) ✗ kn service update hello --env TARGET="v2"
Updating Service 'hello' in namespace 'default':

Event type: ADDED

With my naive fix as an example.

@@ -177,6 +177,13 @@ func (w *waitForReadyConfig) waitForReadyCondition(watcher watch.Interface, star
                        //  resource version. All following watch events are for all changes that occurred after the resource
                        //  version the watch started at."
                        if event.Type != watch.Modified {
+                               inSync, err := generationCheck(event.Object)
+                               if err != nil {
+                                       return false, false, err
+                               }
+                               if inSync {
+                                       return false, false, nil
+                               }
                                continue
                        }

Update is "instantly" executed.

➜  client git:(pr/switch-to-server-generated-revision-names-by-default) ✗ kn service update hello --env TARGET="v2"
Updating Service 'hello' in namespace 'default':

Event type: ADDED
  0.003s Ready to serve.

Service 'hello' with latest revision 'hello-00002' (unchanged) is available at URL:
http://hello.default.example.com

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2021
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 23, 2021
@rhuss
Copy link
Contributor Author

rhuss commented Feb 23, 2021

/hold

On some local tests I have issues with the wait loop

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 23, 2021
@knative-metrics-robot
Copy link

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/configuration_edit_flags.go 85.2% 85.0% -0.2
pkg/wait/wait_for_ready.go 76.9% 75.8% -1.1

@rhuss
Copy link
Contributor Author

rhuss commented Feb 23, 2021

@maximilien @dsimansk would need a lgtm here to get it merged

/unhold

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 23, 2021
@dsimansk
Copy link
Contributor

🤞
/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2021
@rhuss
Copy link
Contributor Author

rhuss commented Feb 23, 2021

/retest

1 similar comment
@rhuss
Copy link
Contributor Author

rhuss commented Feb 23, 2021

/retest

@rhuss
Copy link
Contributor Author

rhuss commented Feb 23, 2021

/hold

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 23, 2021
@knative-prow-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
pull-knative-client-integration-tests 087df2e link /test pull-knative-client-integration-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.

@rhuss
Copy link
Contributor Author

rhuss commented Feb 24, 2021

Close, fixed by #1240

@rhuss rhuss closed this Feb 24, 2021
@knative-prow-robot
Copy link
Contributor

@rhuss: 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 knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use server-generated revisions names as default for services (instead of BYO revision name)
5 participants