CFE-1132: EFS Access Point Tags Update DAY2 #313
Conversation
|
@anirudhAgniRedhat: This pull request references CFE-1132 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
|
@jsafrane this PR adds support for updating tags on efs resources. Here I am annotating the volumes with the sorted hash as discussed in this thread. Can you please review these changes as well. /cc @jsafrane @TrilokGeer |
|
@anirudhAgniRedhat: This pull request references CFE-1132 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
77f5f8c to
58488f9
Compare
|
/retest |
5c27d53 to
13d121b
Compare
| hook := func(spec *opv1.OperatorSpec, deployment *appsv1.Deployment) error { | ||
| infraLister := c.GetInfraInformer().Lister() | ||
| infra, err := infraLister.Get(infrastructureName) | ||
| if err != nil { |
There was a problem hiding this comment.
I would move this code to a different function and perform this computation only if container in question is csi-driver
| return nil | ||
| } | ||
|
|
||
| func (c *EFSAccessPointTagsController) updateVolumeWithRetry(ctx context.Context, pv *v1.PersistentVolume) error { |
There was a problem hiding this comment.
No retries here. I would add the PV to the queue via syncContext.Queue().Add()
There was a problem hiding this comment.
I see an issue with using the syncContext Queue as it will again call the sync function if anything is added to the queue.
I have new retry worker irrespective of the sync function logic and will solve our retry issues.
| return err | ||
| } | ||
|
|
||
| limiter := rate.NewLimiter(rate.Limit(awsTagsRequestRateLimit), awsTagsRequestBurstSize) |
There was a problem hiding this comment.
Can you explain why we added this Limiter here? We are performing update on a single PV, so I am trying to understand how this is helping.
There was a problem hiding this comment.
I have improved the implementation of the rate limiter here.
I am using the limiter to not to hit more than 10 req/sec as listed in AWS Service Quotas
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anirudhAgniRedhat The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
7de43ae to
4bb4f56
Compare
|
/retest |
2631686 to
f63f0ef
Compare
|
@anirudhAgniRedhat: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This PR introduces a custom EFSAccessPointsTagController that monitors the OpenShift Infrastructure resource for changes in AWS ResourceTags. When tags are updated, the controller automatically fetches all AWS EFS-backed PersistentVolumes (PVs) in the cluster, retrieves their volume IDs, and updates the associated EFS Access Points tags in AWS.
Key Changes:
Monitors Infrastructure resource for AWS ResourceTags updates.
Directly fetches all PVs using the AWS EFS CSI driver (efs.csi.aws.com).
Updates AWS EFS AccessPoint tags by merging new and existing tags using the AWS SDK.