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.

Sign up for GitHub

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

More robust WaitForServiceLatestRevision function for rolling upgrades #5956

Merged
merged 3 commits into from
Nov 9, 2019
Merged

More robust WaitForServiceLatestRevision function for rolling upgrades #5956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5ef555a
More robust WaitForServiceLatestRevision function
mgencur Nov 5, 2019
6e1a941
Do not return error in IsRevisionPinned
mgencur Nov 7, 2019
68707a1
Fixes after rebase
mgencur Nov 7, 2019
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
6 changes: 6 additions & 0 deletions test/v1/revision.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ func IsRevisionReady(r *v1.Revision) (bool, error) {
return r.Generation == r.Status.ObservedGeneration && r.Status.IsReady(), nil
}

// IsRevisionPinned will check if the revision is pinned to a route.
func IsRevisionPinned(r *v1.Revision) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that if you just return false that will percolate nicely without error? (return CheckRevisionState)

Copy link
Contributor Author

@mgencur mgencur Nov 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah. That's right. I just wanted to align the function signature with the other functions of this type so that they can be used similarly. But I don't have a problem changing that if required.

Copy link
Contributor

Choose a reason for hiding this comment

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

Signature is fine. But returning false rather than error would achieve same benefit, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I meant signature including return parameters/type. Returning false would be simpler in this case indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ehmm. Actually, the CheckRevisionState function expects the signature I have now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, don't change the signature, change the logic :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooh, ok. Do you mean something like this?

// IsRevisionPinned will check if the revision is pinned to a route.
func IsRevisionPinned(r *v1beta1.Revision) (bool, error) {
	_, pinned := r.Annotations[serving.RevisionLastPinnedAnnotationKey]
	return pinned, nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep.

Copy link
Contributor

Choose a reason for hiding this comment

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

And in the call site just
return X(...), rather than if _, err := X(...); err != nil....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed changes. Unfortunately, the last suggestion from you is not possible because I'm not directly calling IsRevisionPinned but I'm passing it to CheckRevisionState which, itself returns only error. But the enclosing function expects again (bool, error). So there's double conversion of return types throughout the method calls.

_, pinned := r.Annotations[serving.RevisionLastPinnedAnnotationKey]
return pinned, nil
}

// IsRevisionAtExpectedGeneration returns a function that will check if the annotations
// on the revision include an annotation for the generation and that the annotation is
// set to the expected value.
Expand Down
11 changes: 9 additions & 2 deletions test/v1/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"testing"

"github.com/mattbaird/jsonpatch"
"github.com/pkg/errors"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -174,18 +175,24 @@ func WaitForServiceLatestRevision(clients *test.Clients, names test.ResourceName
err := WaitForServiceState(clients.ServingClient, names.Service, func(s *v1.Service) (bool, error) {
if s.Status.LatestCreatedRevisionName != names.Revision {
revisionName = s.Status.LatestCreatedRevisionName
// We also check that the revision is pinned, meaning it's not a stale revision.
// Without this it might happen that the latest created revision is later overriden by a newer one
// and the following check for LatestReadyRevisionName would fail.
if revErr := CheckRevisionState(clients.ServingClient, revisionName, IsRevisionPinned); revErr != nil {
return false, nil
}
return true, nil
}
return false, nil
}, "ServiceUpdatedWithRevision")
if err != nil {
return "", err
return "", errors.Wrapf(err, "LatestCreatedRevisionName not updated")
}
err = WaitForServiceState(clients.ServingClient, names.Service, func(s *v1.Service) (bool, error) {
return (s.Status.LatestReadyRevisionName == revisionName), nil
}, "ServiceReadyWithRevision")

return revisionName, err
return revisionName, errors.Wrapf(err, "LatestReadyRevisionName not updated with %s", revisionName)
}

// Service returns a Service object in namespace with the name names.Service
Expand Down
6 changes: 6 additions & 0 deletions test/v1alpha1/revision.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ func IsRevisionReady(r *v1alpha1.Revision) (bool, error) {
return r.Generation == r.Status.ObservedGeneration && r.Status.IsReady(), nil
}

// IsRevisionPinned will check if the revision is pinned to a route.
func IsRevisionPinned(r *v1alpha1.Revision) (bool, error) {
_, pinned := r.Annotations[serving.RevisionLastPinnedAnnotationKey]
return pinned, nil
}

// IsRevisionAtExpectedGeneration returns a function that will check if the annotations
// on the revision include an annotation for the generation and that the annotation is
// set to the expected value.
Expand Down
27 changes: 18 additions & 9 deletions test/v1alpha1/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,6 @@ import (
"encoding/pem"
"errors"
"fmt"
"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/watch"
"knative.dev/pkg/apis/istio/v1alpha3"
"knative.dev/pkg/test/spoof"
"math/big"
"net"
"net/http"
Expand All @@ -42,7 +38,13 @@ import (
"testing"
"time"

"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/watch"
"knative.dev/pkg/apis/istio/v1alpha3"
"knative.dev/pkg/test/spoof"

"github.com/mattbaird/jsonpatch"
perrors "github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand All @@ -58,13 +60,14 @@ import (

const (
// Namespace is the namespace of the ingress gateway
Namespace = "knative-serving"
Namespace = "knative-serving"

// GatewayName is the name of the ingress gateway
GatewayName = "knative-ingress-gateway"
GatewayName = "knative-ingress-gateway"
)

var (
domainName *string
domainName *string
)

func validateCreatedServiceStatus(clients *test.Clients, names *test.ResourceNames) error {
Expand Down Expand Up @@ -305,18 +308,24 @@ func WaitForServiceLatestRevision(clients *test.Clients, names test.ResourceName
err := WaitForServiceState(clients.ServingAlphaClient, names.Service, func(s *v1alpha1.Service) (bool, error) {
if s.Status.LatestCreatedRevisionName != names.Revision {
revisionName = s.Status.LatestCreatedRevisionName
// We also check that the revision is pinned, meaning it's not a stale revision.
// Without this it might happen that the latest created revision is later overriden by a newer one
// and the following check for LatestReadyRevisionName would fail.
if revErr := CheckRevisionState(clients.ServingAlphaClient, revisionName, IsRevisionPinned); revErr != nil {
return false, nil
}
return true, nil
}
return false, nil
}, "ServiceUpdatedWithRevision")
if err != nil {
return "", err
return "", perrors.Wrapf(err, "LatestCreatedRevisionName not updated")
}
err = WaitForServiceState(clients.ServingAlphaClient, names.Service, func(s *v1alpha1.Service) (bool, error) {
return (s.Status.LatestReadyRevisionName == revisionName), nil
}, "ServiceReadyWithRevision")

return revisionName, err
return revisionName, perrors.Wrapf(err, "LatestReadyRevisionName not updated with %s", revisionName)
}

// LatestService returns a Service object in namespace with the name names.Service
Expand Down
6 changes: 6 additions & 0 deletions test/v1beta1/revision.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ func IsRevisionReady(r *v1beta1.Revision) (bool, error) {
return r.Generation == r.Status.ObservedGeneration && r.Status.IsReady(), nil
}

// IsRevisionPinned will check if the revision is pinned to a route.
func IsRevisionPinned(r *v1beta1.Revision) (bool, error) {
_, pinned := r.Annotations[serving.RevisionLastPinnedAnnotationKey]
return pinned, nil
}

// IsRevisionAtExpectedGeneration returns a function that will check if the annotations
// on the revision include an annotation for the generation and that the annotation is
// set to the expected value.
Expand Down
11 changes: 9 additions & 2 deletions test/v1beta1/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"testing"

"github.com/mattbaird/jsonpatch"
"github.com/pkg/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/wait"
Expand Down Expand Up @@ -173,18 +174,24 @@ func WaitForServiceLatestRevision(clients *test.Clients, names test.ResourceName
err := WaitForServiceState(clients.ServingBetaClient, names.Service, func(s *v1beta1.Service) (bool, error) {
if s.Status.LatestCreatedRevisionName != names.Revision {
revisionName = s.Status.LatestCreatedRevisionName
// We also check that the revision is pinned, meaning it's not a stale revision.
// Without this it might happen that the latest created revision is later overriden by a newer one
// and the following check for LatestReadyRevisionName would fail.
if revErr := CheckRevisionState(clients.ServingBetaClient, revisionName, IsRevisionPinned); revErr != nil {
return false, nil
}
return true, nil
}
return false, nil
}, "ServiceUpdatedWithRevision")
if err != nil {
return "", err
return "", errors.Wrapf(err, "LatestCreatedRevisionName not updated")
}
err = WaitForServiceState(clients.ServingBetaClient, names.Service, func(s *v1beta1.Service) (bool, error) {
return (s.Status.LatestReadyRevisionName == revisionName), nil
}, "ServiceReadyWithRevision")

return revisionName, err
return revisionName, errors.Wrapf(err, "LatestReadyRevisionName not updated with %s", revisionName)
}

// Service returns a Service object in namespace with the name names.Service
Expand Down