Custom CRD Download Strategy Support for Karmada Operator #5185

jabellard · 2024-07-12T19:51:12Z

What type of PR is this?

/kind api-change

What this PR does / why we need it:
This is an implementation of the design outlined in this proposal.

Which issue(s) this PR fixes:
Fixes #5122

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-operator`: Introduced CRD download strategy that allows downloading CRD from a private source.

codecov-commenter · 2024-07-12T20:09:07Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 56 lines in your changes missing coverage. Please review.

Project coverage is 28.37%. Comparing base (7f0961f) to head (1ae5bd1).
Report is 119 commits behind head on master.

Files	Patch %	Lines
operator/pkg/tasks/init/crd.go	0.00%	36 Missing ⚠️
operator/pkg/init.go	0.00%	15 Missing ⚠️
operator/pkg/tasks/init/karmadaresource.go	0.00%	5 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5185      +/-   ##
==========================================
+ Coverage   28.29%   28.37%   +0.07%     
==========================================
  Files         632      632              
  Lines       43635    43823     +188     
==========================================
+ Hits        12348    12433      +85     
- Misses      30386    30486     +100     
- Partials      901      904       +3

Flag	Coverage Δ
unittests	`28.37% <0.00%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

RainbowMango · 2024-08-06T09:02:15Z

@chaunceyjiang would you like to take a look? to evaluate if there is any effect on your deployment.

RainbowMango

Generally looks good to me, thanks to @jabellard.

operator/pkg/tasks/init/crd.go

chaunceyjiang · 2024-08-06T14:03:05Z

/assign

Signed-off-by: Joe Nathan Abellard <contact@jabellard.com> Update crds Signed-off-by: Joe Nathan Abellard <contact@jabellard.com> Remove debug logs Signed-off-by: Joe Nathan Abellard <contact@jabellard.com> Remove debug logs Signed-off-by: Joe Nathan Abellard <contact@jabellard.com> Remove debug logs Signed-off-by: Joe Nathan Abellard <contact@jabellard.com> Remove debug logs Signed-off-by: Joe Nathan Abellard <contact@jabellard.com> Fix linting errors Signed-off-by: Joe Nathan Abellard <contact@jabellard.com> Address comments Signed-off-by: Joe Nathan Abellard <contact@jabellard.com> Update api Signed-off-by: Joe Nathan Abellard <contact@jabellard.com> Update api Signed-off-by: Joe Nathan Abellard <contact@jabellard.com> Address comments Signed-off-by: Joe Nathan Abellard <contact@jabellard.com> Address comments Signed-off-by: Joe Nathan Abellard <contact@jabellard.com> Address comments Signed-off-by: Joe Nathan Abellard <contact@jabellard.com>

RainbowMango · 2024-08-07T01:18:44Z

/retest

RainbowMango

/lgtm

Let's wait @chaunceyjiang for a while.

chaunceyjiang · 2024-08-07T10:27:16Z

operator/pkg/tasks/init/crd.go

+		klog.V(2).InfoS("[skipCrdsDownload] CrdDownloadPolicy is 'Always', skipping cache check")
+		return false, nil
+	}
+


This code is not compatible with the previous behavior.

I believe it is necessary to be compatible with the previous behavior, as this will facilitate user upgrades.

Therefore, I suggest that this code needs to check whether the path path.Join(data.DataDir(), data.KarmadaVersion()) exists. If it does, it should also skip the CRD download."

As I mentioned in your proposal earlier, we have already stored the CRD files in this path in advance. We don't want the karmada-operator to download any files from the internet.

I understand your concern is about changing the cache directory, before this feature, the tarball is cached at data.DataDir()/<KarmadaVersion>, and after this feature, the path becomes data.DataDir()/cache/<hashkey>.

Since you embed the tarball into the image, after this change you have to adopt the new path, which probably needs to make some changes to your Dockerfile. Am I right?

which probably needs to make some changes to your Dockerfile

Yes, I need to modify the Dockerfile. But I am more concerned about: I am not clear about the value of hashkey in the path data.DataDir()/cache/<hashkey>. It seems that every Karmada version needs to guess what this value is.

The approach you're using for pre-loading the CRD tarball can still work. For instance, the CRD tarball for version v1.10.3, by default, would be downloaded via this URL: https://github.com/karmada-io/karmada/releases/download/v1.10.3/crds.tar.gz. To preload, just store it at location data.DataDir()/cache/<cacheKey>, where cacheKey is simply the SHA-256 hash of that URL. It's deterministic.

I think this caching strategy works just fine and follows an approach similar to how OCI images are pulled from registries.

With this feature, although the directory structure will change to accommodate the use of custom download strategies, the use case for pre-populating the CRD in the filesystem can still easily be supported.

However, given that this design is extensible, I do think we can provide a more natural solution for this use case. Today, an HTTPSource is supported, but, for instance, it's possible to also support a LocalFileSystemSource that may help solve this problem more naturally, but I think that's out of scope for this PR.

https://github.com/calvin0327/karmada/blob/master/cluster/images/operator-offline.Dockerfile#L22-L27

This is the Dockerfile we are using. How do I calculate the cacheKey? Can I use sha256sum?

@chaunceyjiang , any other comments/concerns?

Thank you for your reply.

chaunceyjiang · 2024-08-09T02:21:23Z

/lgtm

RainbowMango

It seems we have reached an agreement!
Great thanks to @jabellard for the excellent job!

/approve

karmada-bot · 2024-08-09T03:56:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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:

~~charts/OWNERS~~ [RainbowMango]
~~operator/OWNERS~~ [RainbowMango]
~~operator/pkg/apis/OWNERS~~ [RainbowMango]

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

karmada-bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jul 12, 2024

karmada-bot requested review from calvin0327, chaosi-zju and Poor12 July 12, 2024 19:51

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 12, 2024

jabellard force-pushed the custom_crd_url2 branch from 65656be to 2c6835b Compare July 12, 2024 19:55

jabellard mentioned this pull request Jul 12, 2024

Proposal: Custom CRD Download Strategy Support for Karmada Operator #5145

Merged

jabellard force-pushed the custom_crd_url2 branch from bc44a76 to 2a1ab1b Compare July 17, 2024 19:17

jabellard marked this pull request as draft July 19, 2024 01:24

karmada-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 19, 2024

jabellard marked this pull request as ready for review July 23, 2024 19:31

karmada-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 23, 2024

karmada-bot requested a review from lonelyCZ July 23, 2024 19:31

jabellard changed the title ~~Custom CRD Download URL Support for Karmada Operator~~ Custom CRD Download Strategy Support for Karmada Operator Jul 23, 2024

RainbowMango reviewed Aug 6, 2024

View reviewed changes

operator/pkg/tasks/init/crd.go Outdated Show resolved Hide resolved

operator/pkg/tasks/init/crd.go Outdated Show resolved Hide resolved

karmada-bot assigned chaunceyjiang Aug 6, 2024

jabellard force-pushed the custom_crd_url2 branch from 5a704d2 to 1ae5bd1 Compare August 6, 2024 16:15

RainbowMango reviewed Aug 7, 2024

View reviewed changes

karmada-bot assigned RainbowMango Aug 7, 2024

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 7, 2024

chaunceyjiang reviewed Aug 7, 2024

View reviewed changes

RainbowMango approved these changes Aug 9, 2024

View reviewed changes

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2024

karmada-bot merged commit 04c3844 into karmada-io:master Aug 9, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom CRD Download Strategy Support for Karmada Operator #5185

Custom CRD Download Strategy Support for Karmada Operator #5185

jabellard commented Jul 12, 2024 •

edited by RainbowMango

Loading

codecov-commenter commented Jul 12, 2024 •

edited

Loading

RainbowMango commented Aug 6, 2024

RainbowMango left a comment

chaunceyjiang commented Aug 6, 2024

RainbowMango commented Aug 7, 2024

RainbowMango left a comment

chaunceyjiang Aug 7, 2024

chaunceyjiang Aug 7, 2024

RainbowMango Aug 8, 2024

chaunceyjiang Aug 8, 2024

jabellard Aug 8, 2024

jabellard Aug 8, 2024

chaunceyjiang Aug 8, 2024

jabellard Aug 8, 2024

jabellard Aug 8, 2024

chaunceyjiang Aug 9, 2024

chaunceyjiang commented Aug 9, 2024

RainbowMango left a comment

karmada-bot commented Aug 9, 2024

Custom CRD Download Strategy Support for Karmada Operator #5185

Custom CRD Download Strategy Support for Karmada Operator #5185

Conversation

jabellard commented Jul 12, 2024 • edited by RainbowMango Loading

codecov-commenter commented Jul 12, 2024 • edited Loading

Codecov Report

RainbowMango commented Aug 6, 2024

RainbowMango left a comment

Choose a reason for hiding this comment

chaunceyjiang commented Aug 6, 2024

RainbowMango commented Aug 7, 2024

RainbowMango left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaunceyjiang commented Aug 9, 2024

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Aug 9, 2024

jabellard commented Jul 12, 2024 •

edited by RainbowMango

Loading

codecov-commenter commented Jul 12, 2024 •

edited

Loading