Skip to content

Conversation

@RedbackThomson
Copy link
Contributor

@RedbackThomson RedbackThomson commented Aug 4, 2021

Fixes: aws-controllers-k8s/community#886

The reconciler will indefinitely requeue even when there have been no changes made to the spec or status. This is happening in both the adoption and resource reconciler types.

The root cause comes from controller-runtime requeue-ing the resource when we patch the status subresource - see this issue for details. This bug was introduced for the resource reconciler as part of #39 , since we now call patchResourceStatus on every reconcile loop.

This fix adds an event filter to each manager, with a predicate that the resource must have changed generation. The generation is not changed unless there has been a modification to the spec.

@ack-bot ack-bot requested review from jaypipes and vijtrip2 August 4, 2021 21:45
@vijtrip2
Copy link
Contributor

vijtrip2 commented Aug 4, 2021

"The generation is not changed unless there has been a modification to the spec or status"

I think this statement is not 100% correct. Change in status does not update generation. So I think the statement should be "The .metadata.generation value is incremented for all changes, except for changes to .metadata or .status" - official documentation

@vijtrip2
Copy link
Contributor

vijtrip2 commented Aug 4, 2021

The solution looks good but I have been testing ECR controller locally with latest runtime and I do not see infinite requeues happening. The reconciler stops after successfully reconciling the ecr repository resource and the CR looks like following:

apiVersion: ecr.services.k8s.aws/v1alpha1
kind: Repository
metadata:
  creationTimestamp: "2021-08-04T17:45:56Z"
  finalizers:
  - finalizers.ecr.services.k8s.aws/Repository
  generation: 2
  managedFields:
  - apiVersion: ecr.services.k8s.aws/v1alpha1
    fieldsType: FieldsV1
    fieldsV1:
      f:spec:
        .: {}
        f:imageScanningConfiguration:
          .: {}
          f:scanOnPush: {}
        f:imageTagMutability: {}
        f:name: {}
    manager: kubectl
    operation: Update
    time: "2021-08-04T17:45:56Z"
  - apiVersion: ecr.services.k8s.aws/v1alpha1
    fieldsType: FieldsV1
    fieldsV1:
      f:metadata:
        f:finalizers:
          .: {}
          v:"finalizers.ecr.services.k8s.aws/Repository": {}
      f:spec:
        f:encryptionConfiguration:
          .: {}
          f:encryptionType: {}
      f:status:
        .: {}
        f:ackResourceMetadata:
          .: {}
          f:arn: {}
          f:ownerAccountID: {}
        f:conditions: {}
        f:createdAt: {}
        f:registryID: {}
        f:repositoryURI: {}
    manager: manager
    operation: Update
    time: "2021-08-04T17:45:57Z"
  name: some-repo-xyz
  namespace: default
  resourceVersion: "84786402"
  uid: 4e7e9094-96c4-45eb-86df-4b39d5e4d303
spec:
  encryptionConfiguration:
    encryptionType: AES256
  imageScanningConfiguration:
    scanOnPush: false
  imageTagMutability: MUTABLE
  name: some-repo-xyz
status:
  ackResourceMetadata:
    arn: arn:aws:ecr:us-west-2:30******:repository/some-repo-xyz
    ownerAccountID: "309******"
  conditions:
  - lastTransitionTime: "2021-08-04T17:45:57Z"
    message: Late initialization successful
    reason: Late initialization successful
    status: "True"
    type: ACK.LateInitialized
  createdAt: "2021-08-04T17:45:57Z"
  registryID: "309*****"
  repositoryURI: 309*****.dkr.ecr.us-west-2.amazonaws.com/some-repo-xyz

Hopefully this will assist with some deep dive into this issue If needed. I am also happy with the fix that you put out.

@vijtrip2
Copy link
Contributor

vijtrip2 commented Aug 4, 2021

/approve

@vijtrip2
Copy link
Contributor

vijtrip2 commented Aug 4, 2021

/lgtm

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 4, 2021
@ack-bot
Copy link
Collaborator

ack-bot commented Aug 4, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RedbackThomson, vijtrip2

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:
  • OWNERS [RedbackThomson,vijtrip2]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-bot ack-bot merged commit d7b6e45 into aws-controllers-k8s:main Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adopted resource reconciler fails with already exists error message

3 participants