Skip to content

address regression with User code, fix RG deletion test #35

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

Merged
merged 1 commit into from
Jun 22, 2021
Merged

address regression with User code, fix RG deletion test #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 3 additions & 5 deletions pkg/resource/user/delta_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,10 @@ func filterDelta(
) {
// the returned AccessString can be different than the specified one; as long as the last requested AccessString
// matches the currently desired one, remove this difference from the delta
//TODO: revert this call to Spec.AccessString once we have a new implementation of it
if delta.DifferentAt("AccessString") {
if delta.DifferentAt("Spec.AccessString") {
if *desired.ko.Spec.AccessString == *desired.ko.Status.LastRequestedAccessString {

//TODO: revert the call to Spec.AccessString once removeFromDelta implementation changes
removeFromDelta(delta, "AccessString")
removeFromDelta(delta, "Spec.AccessString")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you have multiple "AccessString" fields in the Status struct?

elasticache-controller/generator.yaml

Lines 122 to 131 in e0d2cb9

LastRequestedAccessString:
is_read_only: true
from:
operation: CreateUser
path: AccessString
ExpandedAccessString:
is_read_only: true
from:
operation: CreateUser
path: AccessString

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The access string provided by the ElastiCache API differs from the specified access string (there's some behind-the-scenes expansion).

Because there's no straightforward way to translate between the two representations, adding a state-keeping field Status.LastRequestedAccessString allows us to determine when the desired access string has changed. This field is only updated upon a successful create or update (see the set_output methods).

Since the generated delta code will always detect a difference between desired.Spec.AccessString and latest.Spec.AccessString there's a hook after that to then remove the difference if desired.Spec.AccessString is still equal to desired.Status.LastRequestAccessString. See filterDelta() in pkg/resource/user/delta_util.go.

Status.ExpandedAccessString is purely informational and was added because that's what the user would see if they were directly using the ElastiCache API.

Copy link
Contributor

Choose a reason for hiding this comment

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

@echen-98 why aren't you simply updating Spec.AccessString to the actual expanded value upon return from CreateUser and just get rid of the two other fields in the Status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaypipes do you mean updating desired.Spec.AccessString or latest.Spec.AccessString? We're already doing the latter, and unless I'm missing something big, I thought the former wasn't possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i.e. I thought patchResource() only patched Status because desired.Spec should not be modified

Copy link
Contributor

Choose a reason for hiding this comment

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

@jaypipes

desired.Spec.AccessString: on ~app::* -@all +@read
latest.Spec.AccessString: on ~app::* -@all +@read +@hash +@bitmap +@geo -setbit -bitfield -hset -hsetnx -hmset -hincrby -hincrbyfloat -hdel -bitop -geoadd -georadius -georadiusbymember

There is always a Spec difference in AccessString. Without any custom code, this path gets added to the delta, and an Update is always invoked. So the default behavior of the generated code is to make (mostly useless) Update calls in an infinite loop.

If we were saving Spec.AccessString as the value returned by the server after a CreateUser or UpdateUser call, there would be no infinite loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So in the Create case, it was my impression that in the reconciler, after a resource is found to not exist and a Create is called, then a ReadOne will be called next. If this ReadOne populates latest.Spec.AccessString with the expanded access string, there will be a Spec difference (since desired.Spec has the user-requested access string). And as long as there is a Spec difference, then Update will be invoked.

What I'm trying to say is it doesn't matter whether we save the server-returned value in latest.Spec.AccessString in a Create or Update call (especially since latest.Spec is thrown away upon patching in these two cases). The infinite loop depends solely upon what happens in ReadOne, and ReadOne will always populate latest.Spec.AccessString with a value different from desired.Spec.AccessString.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I should add that there may not be an infinite loop (that was the initial behavior when I submitted my first iteration of the User code before the split repos, and there have been several runtime changes since then). However, this would still cause an unnecessary Update call after the creation of the resource, because the ReadOne right after the Create would pick up the Spec difference.

Regardless, the point is that we need to store the last requested access string. Otherwise we won't know when the desired access string has changed. If the user updates another field other than the access string, we want to know whether or not to add the access string to the Update payload (I don't think it's best practice to add a field unless it actually has changed).

This is the approach that we decided on before the split repos, and we decided that we should keep it once the User resource was brought back into scope. It works, and if it's sub-optimal, that just reflects that we sometimes have to do sub-optimal things when dealing with the ElastiCache API.

The real point of this discussion is the more general question of: how do you handle the case where an API returns a value different from the one requested? I think that's a very important question to answer, but I also think that this PR is not the place to have this discussion. The point of this PR was to finally have succeeding Prow runs, and to continue to block it until we have a clear answer to that question doesn't make sense to me.

Also, even if we were to settle on a different solution, that would be in a follow-up PR anyway.

echen-98/ack-community@f059064
#32

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @echen-98
We shall let this PR go through and have different issues to track discussion on Spec, Status population and Delta computation and resource patching related aspects for special fields (readonly, server modifiable, not specified in spec but server provided defaults etc.).

Copy link
Contributor

Choose a reason for hiding this comment

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

@jaypipes please suggest if its fine to merge this PR in its current state.

}
}
}
Expand All @@ -43,7 +41,7 @@ func removeFromDelta(
differences := delta.Differences

// identify index of Difference to remove
//TODO: change once we get a Path.Equals or similar method
//TODO: this could require a stricter Path.Equals down the road
var i *int = nil
for j, diff := range differences {
if diff.Path.Contains(subject) {
Expand Down
6 changes: 2 additions & 4 deletions pkg/resource/user/post_build_request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,11 @@ func (rm *resourceManager) populateUpdatePayload(
r *resource,
delta *ackcompare.Delta,
) {
//TODO: change all calls to DifferentAt to include full path once the implementation is clarified

if delta.DifferentAt("AccessString") && r.ko.Spec.AccessString != nil {
if delta.DifferentAt("Spec.AccessString") && r.ko.Spec.AccessString != nil {
input.AccessString = r.ko.Spec.AccessString
}

if delta.DifferentAt("NoPasswordRequired") && r.ko.Spec.NoPasswordRequired != nil {
if delta.DifferentAt("Spec.NoPasswordRequired") && r.ko.Spec.NoPasswordRequired != nil {
input.NoPasswordRequired = r.ko.Spec.NoPasswordRequired
}

Expand Down
27 changes: 22 additions & 5 deletions test/e2e/tests/test_replicationgroup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,24 @@ def rg_cmd_update(rg_cmd_update_input, make_replication_group, rg_deletion_waite
rg_deletion_waiter.wait(ReplicationGroupId=input_dict["RG_ID"])


@pytest.fixture(scope="module")
def rg_deletion_input(make_rg_name):
return {
"RG_ID": make_rg_name("rg-delete"),
"ENGINE_VERSION": "6.x",
"NUM_NODE_GROUPS": "1",
"REPLICAS_PER_NODE_GROUP": "1"
}


@pytest.fixture(scope="module")
def rg_deletion(rg_deletion_input, make_replication_group):
input_dict = rg_deletion_input

(reference, resource) = make_replication_group("replicationgroup_cmd_update", input_dict, input_dict["RG_ID"])
return (reference, resource) # no teardown, as the teardown is part of the actual test


@service_marker
class TestReplicationGroup:

Expand Down Expand Up @@ -229,9 +247,8 @@ def test_rg_auth_token(self, rg_auth_token):
k8s.patch_custom_resource(reference, updated_spec)
assert k8s.wait_on_condition(reference, "ACK.ResourceSynced", "True", wait_periods=30)

def test_rg_delete(self, rg_cmd_update_input, rg_deletion_waiter):
input_dict = rg_cmd_update_input
(reference, _) = make_replication_group("replicationgroup_cmd_update", input_dict, input_dict["RG_ID"])
def test_rg_deletion(self, rg_deletion_input, rg_deletion, rg_deletion_waiter):
(reference, _) = rg_deletion
assert k8s.wait_on_condition(reference, "ACK.ResourceSynced", "True", wait_periods=30)

# assertions after initial creation
Expand All @@ -244,7 +261,7 @@ def test_rg_delete(self, rg_cmd_update_input, rg_deletion_waiter):

resource = k8s.get_resource(reference)
assert resource['metadata']['deletionTimestamp'] is not None
# uncomment when reconciler->cleanup() invokes patchResource()
# TODO: uncomment when reconciler->cleanup() invokes patchResource()
Copy link
Contributor

Choose a reason for hiding this comment

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

reconciler->cleanup() does not invoke patchResource() because it finalizes the deletion of the resource from Kubernetes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That discussion is out of scope of this PR, I just added a TODO here since it felt like the comment needed one.

More details:
aws-controllers-k8s/runtime#21
#34

@kumargauravsharma can elaborate if needed.

# assert k8s.wait_on_condition(reference, "ACK.ResourceSynced", "False", wait_periods=1)

rg_deletion_waiter.wait(ReplicationGroupId=input_dict["RG_ID"])
rg_deletion_waiter.wait(ReplicationGroupId=rg_deletion_input["RG_ID"])