-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
Use and modify docker-mirror to mirror repositories to public ECR registries #9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check if these deps can be used:
"github.com/cenkalti/backoff" seems to be ok - MIT license.
docker "github.com/fsouza/go-dockerclient"
I did not see a license on the fsouza/go-dockerclient just a copyright.
@@ -343,6 +343,7 @@ jobs: | |||
docker tag $IMAGE_NAME public.ecr.aws/$ECR_REPO:latest | |||
docker push public.ecr.aws/$ECR_REPO:$TAG | |||
docker push public.ecr.aws/$ECR_REPO:latest | |||
go run tools/release/adot-operator-images-mirror |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you change is extended from aws-otel-collector release workflow in a separate repo. How are you going to do the operator release? will it override/duplicate the other releases on the collector? (eg, ECR, Dockerhub, S3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Operator release will just mirror the related images from Quay and GCR and push them to two ECR registries - adot-operator
and mirror-kube-rbac-proxy
. It won't affect the Collector release.
return nil | ||
} | ||
|
||
return e.create(name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're creating Production ECR Repo. Do you think we can add some name edit check to make sure we can arbitrary create some test repo name in Prod ECR namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I'm not sure if I understand this correctly. By name edit check, do you mean that we need to check if the registry name is as expected (e.g. adot-operator
)? Thank you!
} | ||
|
||
func getSleepTime(rateLimitReset string, now time.Time) time.Duration { | ||
rateLimitResetInt, err := strconv.ParseInt(rateLimitReset, 10, 64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this constant sleep time is longer enough for rate throttling reset, and exponential backoff retries seems better :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will use exponential backoff retries for this. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Min, I refactored the code structure in mirror.go to apply exponential backoff algorithm to the HTTP request as well as keeping the original sleep time if we get the X-RateLimit-Reset
value from HTTP response header.
time.Sleep(sleepTime) | ||
retries-- | ||
} else if res.StatusCode < 200 || res.StatusCode >= 300 { | ||
log.Printf("Get %s failed with %d, retrying", url, res.StatusCode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have some sleep time for all retries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Let me add this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, I'll approve after addressing the comments
This PR is merged upstream. I will close this one. |
Description:
This PR uses and modifies a third-party repository (docker-mirror) to mirror two repositories (operator repo & kube-rbac-proxy repo) to aws-observability ECR public gallery. The names of the two corresponding repositories are
adot-operator
andmirror-kube-rbac-proxy
respectively.This implementation made three changes/enhancements to the original
docker-mirror
repository:The above changes will be submitted to the
docker-mirror
repository through a series of PRs.