support alibabacloud oss for image registry by menglingwei · Pull Request #724 · openshift/cluster-image-registry-operator

menglingwei · 2021-10-12T09:35:10Z

Add Alibabacloud oss storage for image resigry operator.

update k8s.io/client-go to 0.22.1
update k8s.io/apiserver to 0.22.1
replace openshift/api with menglingwei/api , openshift/api has another PR imageresitry support Alibabacloud oss api#1009

openshift-ci · 2021-10-12T09:35:42Z

Hi @menglingwei. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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/test-infra repository.

menglingwei · 2021-10-12T09:37:16Z

replace PR #720 .

dmage · 2021-10-15T10:42:00Z

/ok-to-test

kwoodson · 2021-10-28T02:26:35Z

@menglingwei Can we rebase this to clean up the conflicts? Thanks!

menglingwei · 2021-10-28T02:32:52Z

@menglingwei Can we rebase this to clean up the conflicts? Thanks!

Yes, i can clean up the conflicts.

This PR is a draf PR for openshift/api#1009. So, I think we can merge this PR first? Then I resubmit a new PR for cluster-image-registry-operator.

ricardomaraschini

Looks good. I haven't found the part where this driver is actually used during the reconciliation. Is that something that is coming next?

ricardomaraschini · 2021-10-28T10:39:01Z

+		generatedName := false
+		// Retry up to 5000 times if we get a naming conflict
+		const numRetries = 5000
+		for i := 0; i < numRetries; i++ {


I imagine you have copied this from other drivers. Can we simply try it once (instead of 500 times) and return an error in case of a failure? The reconcile cycle will happen again later on and then we retry this. wdyt?

menglingwei · 2021-10-28T11:15:06Z

@ricardomaraschini You review api PR first , is that ok? When api PR is merged. i will update go.mod dependences.

dmage · 2021-10-29T10:02:46Z

@menglingwei we don't merge API changes until we have PRs (that have a temporary replace directive) verified by QE that use new API. We have experience of delivering untested API and we don't want to repeat it as we need to maintain backward compatibility and therefore even incorrect API cannot be removed once it is released.

menglingwei · 2021-10-29T10:29:13Z

@menglingwei we don't merge API changes until we have PRs (that have a temporary replace directive) verified by QE that use new API. We have experience of delivering untested API and we don't want to repeat it as we need to maintain backward compatibility and therefore even incorrect API cannot be removed once it is released.

I understand. so i need to fix this drat PR, and make the test run success. and then you will merge the API PR?

kwoodson · 2021-10-29T13:21:09Z

@menglingwei Thanks for the efforts here. Please fix the conflicts with a rebase as well as the tests. Then we can perform the necessary tests.

menglingwei · 2021-10-29T13:52:24Z

rebase

I got it. i will fix the conflict ASAP

menglingwei · 2021-11-01T03:33:34Z

ci/prow/e2e-ovirt job failed. but i don't know how to make this job run success.any suggestions ?

menglingwei · 2021-11-01T07:34:39Z

@menglingwei Thanks for the efforts here. Please fix the conflicts with a rebase as well as the tests. Then we can perform the necessary tests.

@kwoodson I fixed the conflicts with rebase master. but I found some jobs also run failed.

dmage · 2021-11-02T17:54:46Z

https://github.com/openshift/cluster-image-registry-operator/pull/724/commits

PRs shouldn't contain merge commits. Please rebase using git rebase.

kwoodson · 2021-11-02T19:19:15Z

@ricardomaraschini Could you comment on the state of this PR? Have you had an opportunity to test? I can assist or if you could point me to the test suite for the image registry and I can perform this.

dmage · 2021-11-02T20:14:51Z

@kwoodson tests should be part of this PR, it's a new feature and it should have new tests.

Manual tests:

if you change spec.storage in configs.imageregistry.operator.openshift.io/cluster to
```
storage:
    oss: {}
```
on Alibaba cluster, the operator should automatically create an OSS bucket and configure the registry to use it. It should be possible to push/pull images into/from the registry after that.
if you change spec.storage and provide all configuration parameters by yourself, the operator should use them.
there should be a way to configure the operator to use OSS on other platforms (i.e. there should be a way to override secrets that are used by the operator).

dmage · 2021-11-02T20:16:27Z

on a freshly installed cluster that uses Alibaba platform, the operator should automatically configure the registry to use OSS.

menglingwei · 2021-12-22T15:50:48Z

@dmage @kwoodson @xiuwang I updated the oss policy.

The correct tagging action are :

                "oss:PutBucketTagging",
                "oss:GetBucketTagging",
                "oss:DeleteBucketTagging",

mtulio · 2021-12-22T17:48:00Z

@menglingwei, @kwoodson just create a new cluster successfully. Now we can see that those new policies were created. Thanks.

$ aliyun ram GetPolicy \
    --PolicyName test-rglgq-openshift-image-registry-installer-cloud-credentials-policy-policy \
    --PolicyType Custom \
    | jq  -r '.DefaultPolicyVersion.PolicyDocument' \
    | jq '.Statement[].Action' |grep -E '(Tagging|ListBuckets)'
  "oss:PutBucketTagging",
  "oss:GetBucketTagging",
  "oss:DeleteBucketTagging",
  "oss:ListBuckets",

$ BUCKET_NAME=$(oc get configs.imageregistry.operator.openshift.io/cluster -n openshift-image-registry -o jsonpath='{.spec.storage}' |jq -r .oss.bucket)

$ echo $BUCKET_NAME 
test-rglgq-image-registry-us-east-1-jknktcahorkmcvlwvnnfgykxrh

$ ./ossutil64 ls oss://${BUCKET_NAME}
Object Number is: 0

1.219796(s) elapsed

$ oc get pods -l docker-registry=default -n openshift-image-registry
NAME                              READY   STATUS    RESTARTS   AGE
image-registry-77b9bb4bbd-6thxc   1/1     Running   0          38m
image-registry-77b9bb4bbd-c7z8b   1/1     Running   0          38m

@xiuwang could you please validate it?

@ricardomaraschini @dmage please see the feedback from the final review from QE.

/lgtm

mtulio · 2021-12-22T18:39:49Z

/retest-required

kwoodson · 2021-12-22T21:13:21Z

/retest-required

menglingwei · 2021-12-23T00:02:09Z

/retest-required

menglingwei · 2021-12-23T00:02:53Z

/retest

menglingwei · 2021-12-23T01:26:27Z

/retest

openshift-ci · 2021-12-23T03:24:34Z

@menglingwei: all tests passed!

Full PR test history. Your PR dashboard.

Details

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/test-infra repository. I understand the commands that are listed here.

xiuwang · 2021-12-23T07:48:44Z

Validate the image registry works well with OSS stoage, the error never shown,

oss: service returned error: StatusCode=403, ErrorCode=AccessDenied, ErrorMessage="The bucket you access does not belong to you"

$oc get pods -l docker-registry=default -n openshift-image-registry
NAME READY STATUS RESTARTS AGE
image-registry-5ff9448d65-62rz2 1/1 Running 0 8m44s
image-registry-5ff9448d65-rfndv 1/1 Running 0 8m44s

/lgtm

ricardomaraschini · 2021-12-23T08:48:14Z

@xiuwang can you please qe-approved this ?

ricardomaraschini · 2021-12-23T08:48:21Z

/lgtm cancel

ricardomaraschini · 2021-12-23T08:50:47Z

@sferich888 could you please re add px-approved here? This PR went through some changes and ended up loosing all its flags.

ricardomaraschini · 2021-12-23T08:51:14Z

/lgtm

ricardomaraschini · 2021-12-23T08:56:49Z

/lgtm cancel

I was not aware that once I add lgtm the PR is automatically approved. We need to wait for the others' approved flag first.

xiuwang · 2021-12-23T09:12:38Z

/label qe-approved

mtulio · 2021-12-23T11:17:12Z

@sferich888 could you please re add px-approved here? This PR went through some changes and ended up loosing all its flags.

@ricardomaraschini there is already a px-approved label.

ricardomaraschini · 2021-12-23T11:21:50Z

/lgtm

openshift-ci · 2021-12-23T11:22:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jianli-wei, kwoodson, menglingwei, mtulio, ricardomaraschini, xiuwang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ricardomaraschini]

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

openshift-ci Bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 12, 2021

openshift-ci Bot requested review from bparees and smarterclayton October 12, 2021 09:35

This was referenced Oct 13, 2021

add Alibabacloud oss storage for image registry operator #720

Closed

imageresitry support Alibabacloud oss openshift/api#1009

Closed

openshift-ci Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 15, 2021

openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2021

ricardomaraschini suggested changes Oct 28, 2021

View reviewed changes

openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2021

menglingwei force-pushed the feature/support-alibaba-oss branch from 9a6e6a4 to e857b31 Compare November 3, 2021 01:44

openshift-ci Bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 3, 2021

menglingwei marked this pull request as draft November 4, 2021 11:26

openshift-ci Bot assigned xiuwang Dec 23, 2021

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2021

openshift-ci Bot assigned ricardomaraschini Dec 23, 2021

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2021

openshift-ci Bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 23, 2021

openshift-ci Bot removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 23, 2021

openshift-ci Bot added the qe-approved Signifies that QE has signed off on this PR label Dec 23, 2021

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2021

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 23, 2021

openshift-merge-robot merged commit 11278d4 into openshift:master Dec 23, 2021

Conversation

menglingwei commented Oct 12, 2021

Uh oh!

openshift-ci Bot commented Oct 12, 2021

Uh oh!

menglingwei commented Oct 12, 2021

Uh oh!

dmage commented Oct 15, 2021

Uh oh!

kwoodson commented Oct 28, 2021

Uh oh!

menglingwei commented Oct 28, 2021

Uh oh!

ricardomaraschini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ricardomaraschini Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

menglingwei commented Oct 28, 2021

Uh oh!

dmage commented Oct 29, 2021

Uh oh!

menglingwei commented Oct 29, 2021

Uh oh!

kwoodson commented Oct 29, 2021

Uh oh!

menglingwei commented Oct 29, 2021

Uh oh!

menglingwei commented Nov 1, 2021

Uh oh!

menglingwei commented Nov 1, 2021

Uh oh!

dmage commented Nov 2, 2021

Uh oh!

kwoodson commented Nov 2, 2021

Uh oh!

dmage commented Nov 2, 2021

Uh oh!

dmage commented Nov 2, 2021

Uh oh!

menglingwei commented Dec 22, 2021

Uh oh!

mtulio commented Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtulio commented Dec 22, 2021

Uh oh!

kwoodson commented Dec 22, 2021

Uh oh!

menglingwei commented Dec 23, 2021

Uh oh!

menglingwei commented Dec 23, 2021

Uh oh!

menglingwei commented Dec 23, 2021

Uh oh!

openshift-ci Bot commented Dec 23, 2021

Uh oh!

xiuwang commented Dec 23, 2021

Uh oh!

ricardomaraschini commented Dec 23, 2021

Uh oh!

ricardomaraschini commented Dec 23, 2021

Uh oh!

ricardomaraschini commented Dec 23, 2021

Uh oh!

ricardomaraschini commented Dec 23, 2021

Uh oh!

ricardomaraschini commented Dec 23, 2021

Uh oh!

xiuwang commented Dec 23, 2021

mtulio commented Dec 22, 2021 •

edited

Loading