Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

Treat resourceVersion as opaque string #467

Closed
wants to merge 3 commits into from
Closed

Treat resourceVersion as opaque string #467

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
34bba70
Treat ResourceVersion as opaque string instead of integer
nathancoleman Nov 29, 2022
35c453a
Update unit tests for ReconcileManager
nathancoleman Nov 29, 2022
be3c624
Update unit tests for K8sGatewayClasses
nathancoleman Nov 29, 2022
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
5 changes: 2 additions & 3 deletions internal/k8s/reconciler/gatewayclass.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (

"github.com/hashicorp/consul-api-gateway/internal/k8s/gatewayclient"
rstatus "github.com/hashicorp/consul-api-gateway/internal/k8s/reconciler/status"
"github.com/hashicorp/consul-api-gateway/internal/k8s/utils"
apigwv1alpha1 "github.com/hashicorp/consul-api-gateway/pkg/apis/v1alpha1"
)

Expand Down Expand Up @@ -51,8 +50,8 @@ func (g *K8sGatewayClasses) Upsert(ctx context.Context, class *K8sGatewayClass)
g.mutex.Lock()
defer g.mutex.Unlock()

if current, ok := g.gatewayClasses[class.class.Name]; ok {
if utils.ResourceVersionGreater(current.class.ResourceVersion, class.class.ResourceVersion) {
if cachedClass, ok := g.gatewayClasses[class.class.Name]; ok {
if cachedClass.class.ResourceVersion == class.class.ResourceVersion {
Copy link
Contributor

Choose a reason for hiding this comment

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

So part of the problem with this is that I believe we can actually corrupt our cache if we're simultaneously reconciling resources and only doing equality checks (i.e. an old resource is going to potentially override a newer resource). Since a lot of our internal state storage has changed fairly substantially, I'm not entirely sure if this is still an issue, but it clearly was early on -- which is why we explicitly made the decision early on to ignore the warning against resource version parsing (it has been a monotonically increasing global 64 bit integer based on etcd's revision field since day one IIRC).

I'm fine if we can remove the reliance on that, but just be aware that we may get

  1. (in the best case), additional log spam due to failures in whatever status Update calls we're doing
  2. (in the bad case), potential, hopefully temporary, cache corruption that may or may not affect what we're syncing to Consul
  3. (in the worst case), due to the above, potentially wedging a resource in a reconciliation loop due to affecting upstream dependencies that are stale in cache (i.e. we write in an old gateway somehow, but routes are reconciling that require a gateway status update for such a thing as the number of routes bound to it, the update fails, so the route gets re-reconciled, but never completes until we purge/fix the stale cache entry)

All of this said, while this fix seems fairly straightforward, it can potentially have some pretty large consequences, so I would want to thoroughly vet it.

Copy link

Choose a reason for hiding this comment

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

@andrewstucki if I understand this logic correctly, the caller of K8sGatewayClasses.Upsert() wants to mutate if and only if the cached version of the class they have is up-to-date, right? Instead of doing the resource version comparison manually here, why not pass the resource version through the client to the server as a precondition and allow the server to reject the call if it's out-of-date? Store in g.gatewayClasses[class.class.Name] if successful and ignore the conflict error if you want your client not to re-try with an updated version from the cache.

Copy link

Choose a reason for hiding this comment

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

Letting the server do the resourceVersion comparison for you would free you from having to carefully manage when you do and do not add to your client-side cache, meaning your option 2 & 3 would not occur. Although, one more question - are you using the controller-runtime cache-backed clients, and, if so, what is the additional utility to the in-memory cache in question here?

Copy link
Member Author

@nathancoleman nathancoleman Nov 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think the concern is less about where we do the comparing and more that the meaning of "out of date" with this PR changes from "a lower incrementing resourceVersion integer than the one I currently have" to "a resource version that exactly matches the one I currently have".

The current state deals with a watch trigger series for versions 1->3->2 by rejecting the last version because 2 < 3, ending up holding resourceVersion 3.

The proposed state would accept the last version because 2 != 3, ending up holding resourceVersion 2.

Copy link

Choose a reason for hiding this comment

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

I'm sorry, I don't think I follow (yet!).

In the current implementation, the reconciler has a cached version of an object, with some resourceVersion=a. The intent with the current Upsert() semantic is that this reconciler's client call should only go through if the cache has not seen a more-recent version resourceVersion=b of the object yet.

If, instead of doing the check in-memory, the reconclier unconditionally made the client call to the sever, and passed that resourceVersion=a in the precondition on the UpdateStatus(), the server would ensure that the there had not been any committed changes to the object past a - so the invariant remains that the UpdateStatus() succeeds if and only if the client's cached version (a) is the latest. In fact, the invariant becomes stronger, since you are guaranteed not only that a is newer than any revision you have happened to observe and store in your in-memory cache, but that it is the newest revision available in the database. Storing the object only on a successful call would allow your cache to stay coherent.

With the current implementation, if you want to make sure your upsert is not clobbering newer data on the server (perhaps from other actors), you still need to pass the precondition to the server to guard against the following case:

  • cache observes object with resourceVersion=a
  • reconclier uses cached state to issue an upsert call
  • in-memory resourceVersion comparison is successful, as no new data exists in the cache
  • server updates the object to resourceVersion=b
  • reconclier issues a call to the server to UPDATE the resource
  • cache observes object with resourceVersion=b

Copy link

Choose a reason for hiding this comment

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

The reason I mentioned the controller-runtime cache-backed client, as well, is that you have a full view of the objects your reconclier is operating over in an in-memory cache as part of the controller-runtime infrastructure. Therefore, a simple pattern to ensure correctness is to pass the resourceVersion on objects you see in a reconcile.Request to the server as a precondition. If the server call fails with a conflict, the cached view that your reconclier used was out-of-date. However, you'll see the up-to-date version at some point in the future, at which point the reconciler's client call will succeed and you've made forward progress.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stevekuznetsov The cache/out-of-k8s storage is required for shipping SDS and syncing all of our data to Consul -- having a working set of how things need to be and actually enforcing that when users change things externally (simple example of enforcing intentions updates when a user may add or remove an additional source or destination to a service that we're routing to). RE your comment in your issue about the in-memory store, the original design of this external storage layer was to be pluggable with future implementations, including using durable storage backends, in part to pave the way for working with both on k8s and off-k8s execution modes -- some of that is no longer true though.

RE the current use of resourceVersion I'm thinking that part of our use of resourceVersion could be ameliorated by actually doing the external storage sync after delegating to Kubernetes for the version check during our status updates, if the update succeeds, then as you mentioned, we have made forward progress since we were using a fresh entry and we can go ahead and do the rest of the sync. This does make tracking whether or not we were able to sync state to our external sources as part of the user reflected Status more difficult though, as that now happens after the initial remote update and would require a second pass through the reconciler to actually update the remote sync status.

Just to set expectations, given that there's likely a good amount of work involved with both changing this resourceVersion check and ensuring correctness with both Kubernetes usage and our external state syncing code and the current workload/priorities of our team, I'm not sure that we'll be able to change this behavior in short order. I would like to fix it, but it could take some time.

Copy link

Choose a reason for hiding this comment

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

Absolutely - if you have appetite for it, I am more than happy to take on the work. By no means did I mean to show up and ask you to do more - I am sure you are very busy! If that sounds like a good idea to you, I'd love to take more time to learn the code and perhaps sync with you to understand the problem space a little better. I'm sure there's a solution here that does not require breaking that particular API convention.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure there's a solution here that does not require breaking that particular API convention.

Totally agree that this is tractable and thanks for your desire to contribute! If you do wind up tackling this, let me know if you have any questions or need some feedback.

// we have an old gatewayclass update ignore
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion internal/k8s/reconciler/gatewayclass_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func TestGatewayClasses(t *testing.T) {

gatewayClass = NewK8sGatewayClass(&gwv1beta1.GatewayClass{
ObjectMeta: meta.ObjectMeta{
ResourceVersion: "0",
ResourceVersion: "1",
},
Spec: gwv1beta1.GatewayClassSpec{
ParametersRef: &gwv1beta1.ParametersReference{
Expand Down
4 changes: 2 additions & 2 deletions internal/k8s/reconciler/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ func (m *GatewayReconcileManager) UpsertGateway(ctx context.Context, g *gwv1beta
if current == nil {
return true
}
return !utils.ResourceVersionGreater(current.(*K8sGateway).ResourceVersion, gateway.ResourceVersion)
return current.(*K8sGateway).ResourceVersion != gateway.ResourceVersion
})
}

Expand Down Expand Up @@ -235,7 +235,7 @@ func (m *GatewayReconcileManager) upsertRoute(ctx context.Context, r Route, pare
if current == nil {
return true
}
return !utils.ResourceVersionGreater(current.(*K8sRoute).GetResourceVersion(), route.GetResourceVersion())
return current.(*K8sRoute).GetResourceVersion() != route.GetResourceVersion()
})
}

Expand Down
45 changes: 29 additions & 16 deletions internal/k8s/reconciler/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ import (
"testing"

"github.com/golang/mock/gomock"
"github.com/google/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
gwv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
Expand Down Expand Up @@ -34,24 +37,34 @@ func TestUpsertGatewayClass(t *testing.T) {
Logger: hclog.NewNullLogger(),
})

inner := &gwv1beta1.GatewayClass{}
expected := errors.New("expected")
client.EXPECT().UpdateStatus(gomock.Any(), inner).Return(expected)
require.Equal(t, expected, manager.UpsertGatewayClass(context.Background(), inner))
t.Run("error validating GatewayClassConfig", func(t *testing.T) {
inner := &gwv1beta1.GatewayClass{
Spec: gwv1beta1.GatewayClassSpec{
ParametersRef: &gwv1beta1.ParametersReference{
Group: apigwv1alpha1.Group,
Kind: apigwv1alpha1.GatewayClassConfigKind,
},
},
}
expected := errors.New("expected")
client.EXPECT().GetGatewayClassConfig(gomock.Any(), gomock.Any()).Return(nil, expected)

client.EXPECT().UpdateStatus(gomock.Any(), inner)
require.NoError(t, manager.UpsertGatewayClass(context.Background(), inner))
err := manager.UpsertGatewayClass(context.Background(), inner)
assert.EqualError(t, err, expected.Error())
})

// validation
client.EXPECT().GetGatewayClassConfig(gomock.Any(), gomock.Any()).Return(nil, expected)
require.Equal(t, expected, manager.UpsertGatewayClass(context.Background(), &gwv1beta1.GatewayClass{
Spec: gwv1beta1.GatewayClassSpec{
ParametersRef: &gwv1beta1.ParametersReference{
Group: apigwv1alpha1.Group,
Kind: apigwv1alpha1.GatewayClassConfigKind,
},
},
}))
t.Run("error updating status", func(t *testing.T) {
inner := &gwv1beta1.GatewayClass{ObjectMeta: v1.ObjectMeta{ResourceVersion: uuid.NewString()}}
expected := errors.New("expected")
client.EXPECT().UpdateStatus(gomock.Any(), inner).Return(expected)
assert.EqualError(t, manager.UpsertGatewayClass(context.Background(), inner), expected.Error())
})

t.Run("successful update", func(t *testing.T) {
inner := &gwv1beta1.GatewayClass{ObjectMeta: v1.ObjectMeta{ResourceVersion: uuid.NewString()}}
client.EXPECT().UpdateStatus(gomock.Any(), inner)
assert.NoError(t, manager.UpsertGatewayClass(context.Background(), inner))
})
}

func TestUpsertGateway(t *testing.T) {
Expand Down
17 changes: 0 additions & 17 deletions internal/k8s/utils/versions.go
View file
Open in desktop

This file was deleted.