Skip to content
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

Jump to bottom

Optimize OCI Implementation for Apps & Marketplace #45062

Merged
merged 3 commits into from
Apr 12, 2024
Merged

Optimize OCI Implementation for Apps & Marketplace #45062

merged 3 commits into from
Apr 12, 2024

Conversation

rohitsakala
Copy link
Member

@rohitsakala rohitsakala commented Apr 9, 2024 •
edited
Loading

  1. A preivous implementation of OCI was merged in this PR [2.9] Add OCI support for Helm Repos #43823
  2. This is a new implementation of the OCI where we created a new onChange wrangler handler to make satisfy the new design implementation. Please refer to the JIRA issue for more information.

UI Issue - rancher/dashboard#9815

Release Notes

  • Added OCI Feature for Apps & Marketplace and it is in the experimental stage. So please put it in the Experimental Features section.
  • Increased the syncup internval from 5 minutes to 6 hours so that our gitmirror and oci registries are not stressed.

Engineering Testing

  • Added Unit Tests
  • Added Integration Tests

QA Testing

Test Cases for QA Testing

Test Case Scenario - With Authentication

  • Creation of the ClusterRepo with OCI-based Helm Repo creates a successful config map with an index.yaml inside it.
  • Deletion of the ClusterRepo with OCI-based Helm Repo deletes the config map with the index.yaml inside it.
  • Update of the ClusterRepo with OCI-based Helm Repo updates the config map with the index.yaml inside it.
  • Installation of a Helm Chart from OCI registry. Will be tested using Steve API Calls.
  • Uninstallation of a Helm Chart from OCI registry. Will be tested using Steve API Calls.

Test Case Scenario - Without Authentication

  • Creation of the ClusterRepo with OCI-based Helm Repo creates a successful config map with an index.yaml inside it.
  • Deletion of the ClusterRepo with OCI-based Helm Repo deletes the config map with the index.yaml inside it.
  • Update of the ClusterRepo with OCI-based Helm Repo updates the config map with the index.yaml inside it.
  • Installation of a Helm Chart from OCI registry. Will be tested using Steve API Calls.
  • Uninstallation of a Helm Chart from OCI registry. Will be tested using Steve API Calls.

Test Case Scenario - Different fields of ClusterRepo

  • CABundle field of ClusterRepo
  • InsecureSkipTLSverify field of ClusterRepo
  • InsecureSkipTLSverify field of ClusterRepo
  • ForceUpdate field of ClusterRepo
  • DisableSameOriginCheck field of ClusterRepo

@rohitsakala rohitsakala requested a review from a team April 9, 2024 18:31
@rohitsakala rohitsakala added this to the v2.9-Next1 milestone Apr 9, 2024
@rohitsakala rohitsakala force-pushed the ability-to-use-oci-registries-in-appsmarketplace branch 4 times, most recently from 8c3a3e7 to bd1842a Compare April 9, 2024 21:42
@rohitsakala rohitsakala requested a review from pjbgf April 9, 2024 21:58
@rohitsakala rohitsakala force-pushed the ability-to-use-oci-registries-in-appsmarketplace branch 4 times, most recently from a5ca835 to 103f1fb Compare April 9, 2024 23:13
@rohitsakala rohitsakala marked this pull request as ready for review April 10, 2024 00:55
diogoasouza
diogoasouza previously approved these changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@diogoasouza diogoasouza left a comment

Choose a reason for hiding this comment

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

Just need to remove the temporary commit before merging

@rohitsakala rohitsakala force-pushed the ability-to-use-oci-registries-in-appsmarketplace branch 2 times, most recently from 419f8e5 to 5a5e777 Compare April 10, 2024 04:49
pjbgf
pjbgf reviewed Apr 10, 2024
View reviewed changes
pkg/catalogv2/oci/client.go
Comment on lines +39 to +44
var retryPolicy retry.GenericPolicy = retry.GenericPolicy{
Retryable: retry.DefaultPredicate,
MinWait: 1 * time.Second,
MaxWait: 5 * time.Second,
MaxRetry: 5,
}
Copy link
Member

Choose a reason for hiding this comment

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

When accessing a rate-limited server under distress we probably want to way longer before retrying. If I read this correctly, we would do our 6 requests in 20 seconds.

It is also worth checking the Retry-After header. If set, it can help us avoid prolonging the rate-limited state by not retrying when asked not to for a given period of time.

Copy link
Contributor

Choose a reason for hiding this comment

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

One limitation here is that the retry is handled by the oras library, and afaik, we can't set one time for 429 and another for other errors. Regarding the header, oras handles it here

Copy link
Member Author

@rohitsakala rohitsakala Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also @pjbgf those values are configurable by the user.

pjbgf
pjbgf reviewed Apr 10, 2024
View reviewed changes
pkg/catalogv2/oci/client.go
Comment on lines +227 to +243
if o.exponentialBackOffValues != nil {
if o.exponentialBackOffValues.MaxRetries > 0 {
retryPolicy.MaxRetry = o.exponentialBackOffValues.MaxRetries
}
if o.exponentialBackOffValues.MaxWait != nil {
retryPolicy.MaxWait = o.exponentialBackOffValues.MaxWait.Duration
}
if o.exponentialBackOffValues.MinWait != nil {
retryPolicy.MinWait = o.exponentialBackOffValues.MinWait.Duration
}
}
retryPolicy.Backoff = retry.ExponentialBackoff(retryPolicy.MinWait, 2, 0.2)

retryTransport := retry.NewTransport(baseTransport)
retryTransport.Policy = func() retry.Policy {
return &retryPolicy
}
Copy link
Member

Choose a reason for hiding this comment

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

This could keep the reconciliation loop busy just waiting. The retry policy could be combined with the reconciler's Result.RequeueAfter may enable a better use of resources.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes @pjbgf We would like to work on iterations and use this comment as a feedback for improvement on the next PR.

cc @diogoasouza

@rohitsakala rohitsakala force-pushed the ability-to-use-oci-registries-in-appsmarketplace branch from 5a5e777 to 54cde74 Compare April 12, 2024 18:38
@rohitsakala rohitsakala merged commit 3b084f0 into rancher:release/v2.9 Apr 12, 2024
2 checks passed
@diogoasouza diogoasouza mentioned this pull request Jul 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pjbgf pjbgf pjbgf left review comments

@diogoasouza diogoasouza diogoasouza approved these changes

Assignees

@diogoasouza diogoasouza

@rohitsakala rohitsakala

Labels
None yet
Projects
None yet
Milestone
v2.9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@rohitsakala @pjbgf @diogoasouza