Enables AdoptedResource and adds an integration test [on branch update-runtime-0.12] #29
Conversation
ack-bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mbaijal 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 |
mbaijal
force-pushed
the
adopted-resource-0.12
branch
from
146b350 to
cff6684
Compare
|
/test applicationautoscaling-kind-e2e |
1 similar comment
|
/test applicationautoscaling-kind-e2e |
mbaijal
changed the title
ack-bot
removed
the
do-not-merge/work-in-progress
|
/test applicationautoscaling-kind-e2e |
1 similar comment
|
/test applicationautoscaling-kind-e2e |
|
/test applicationautoscaling-release-test |
mbaijal
changed the title
| r.ko.Status.ACKResourceMetadata = &ackv1alpha1.ResourceMetadata{} | ||
| } | ||
| r.ko.Spec.ResourceID = identifier.NameOrID | ||
| r.ko.Status.ACKResourceMetadata.ARN = identifier.ARN |
There was a problem hiding this comment.
spec for the adopted resource has nameorID
https://github.com/aws-controllers-k8s/applicationautoscaling-controller/pull/29/files#diff-01e70988feb091048af2992f4465414c1a0a8aad0622fd4d13d54224b3b445a9R7
and this SetIdentifiers method would execute when this is called - https://github.com/aws-controllers-k8s/runtime/blob/main/pkg/runtime/adoption_reconciler.go#L152, but it's looking for identifier.ARN instead of NameOrID
Can you help me understand how the adopted resource is working before I approve this PR?
There was a problem hiding this comment.
Spec for adopted resource has optional nameOrID OR arn fields - https://github.com/aws-controllers-k8s/runtime/blob/86654532edbd5e5cf99d7f89c2aa561bce810f14/apis/core/v1alpha1/identifiers.go#L17
The user can specify one or the other. Depending on what the describe call expects, it will use that value to populate the CR spec.
There was a problem hiding this comment.
based on further discussion with Nick,
it probably works like this:
- The adoption reconciler creates an empty ScalableTarget CR
- We set ARN = identifier.ARN (which is an empty string) - so we still have an empty ScalableTarget CR
- We call ReadOne, which under the hood calls ReadMany with an empty ResourceIds list
- The result of the ReadOne is the first element in the ReadMany call and there is an element in result because resourceId is an optional parameter
- We set the CR to that result. so it might be some random scalable target in the service namespace, not necessarily the one it was supposed to adopt.
Recommend the following:
- add an assert for that spec.resourceId in the test
- work on fixing this as must have before release
- (for future) setIdentifiers can check it is assigning a nil value
There was a problem hiding this comment.
for 2 -> aws-controllers-k8s/code-generator#155
this should help
There was a problem hiding this comment.
PR linked above will not work out of the box because it will be looking for ScalableTargetIds or ScalableTargetId - REF
but for this resource required field is actually resourceIds
There was a problem hiding this comment.
Yea, also need to change the field identified as arn/name below. Both ScalableTarget and ScalingPolicy have bugs atm.
| policy_resource = k8s.wait_resource_consumed_by_controller(policy_reference) | ||
| assert policy_resource is not None | ||
|
|
||
| assert policy_resource["spec"].get("resourceID", None) == sdk_resource_id |
There was a problem hiding this comment.
@surajkota This assertion is there and IT WAS failing for me locally - it started passing on prow and thus I removed the WIP from this PR and assumed all was ok!
There was a problem hiding this comment.
It would work on Prow because Prow has no other ScalableTargets in the account (or shouldn't). Therefore the result of any ReadMany that doesn't specify ResourceIds should contain only one element - the scalable target you created in the fixture. It didn't work in your personal account because you may have had more than one scalable target already.
| ) | ||
|
|
||
| wait_sagemaker_endpoint_status(endpoint_name, ENDPOINT_STATUS_INSERVICE) | ||
| assert resource_id is not None |
There was a problem hiding this comment.
nit: Can resource_id ever be None, looks like we are setting it as a string
|
Closing for now since some issues need to be addressed. |
5 participants
PR is based off of the Update Runtime branch.
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.