support alibaba cloud ccm by gujingit · Pull Request #119 · openshift/cluster-cloud-controller-manager-operator

gujingit · 2021-09-13T03:25:51Z

No description provided.

openshift-ci · 2021-09-13T03:26:19Z

Hi @gujingit. 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.

JoelSpeed

Please take a look at the cloud_test file again. There are several tests in there that the alibaba platform is needed to be added to, but I think you've only added the new platform to half of them

JoelSpeed · 2021-09-13T09:03:11Z

+            name: cloud-conf
+            items:
+              - key: cloud.conf
+                path: cloud-config.conf


Please ensure all files end with a new line (it helps with git diffs in the future)

JoelSpeed · 2021-09-13T09:03:30Z

+              --kubeconfig=/etc/kubernetes/cloud-controller-manager.conf \
+              --address=127.0.0.1 \
+              --allow-untagged-cloud=true \
+              --leader-elect=true \


There are more leader election flags required to be configured, the unit tests should explain the issues (you can also copy/paste the leader election flags from any other platform)

JoelSpeed · 2021-09-13T09:04:38Z

 package cloud

 import (
+	"github.com/openshift/cluster-cloud-controller-manager-operator/pkg/cloud/alibaba"


Please move this import into the group with the rest of the cloud providers, it should come before the AWS import on line 9

JoelSpeed · 2021-09-13T09:05:41Z

 		isAzureStack: true,
 		expected:     azurestack.GetResources(),
+	}, {
+		name:     "AlibabaCloud resources are empty",


This test description isn't correct

Suggested change

name: "AlibabaCloud resources are empty",

name: "AlibabaCloud resources returned as expected",

JoelSpeed · 2021-09-13T13:43:14Z

There are several tests in there that the alibaba platform is needed to be added to, but I think you've only added the new platform to half of them

Never mind this comment, I just merged #121 which removes the extra lists. I think you should be fine after a rebase and all the tests should be covered

/ok-to-test

JoelSpeed · 2021-09-15T12:23:08Z

Changes look good, but you'll need to run go mod vendor and then add the resulting changes as an additional commit to pass the verify step. Once that's done I think we are good to go :)

elmiko · 2021-09-15T22:15:21Z

+                source /etc/kubernetes/apiserver-url.env
+              fi
+              exec /cloud-controller-manager \
+              --kubeconfig=/etc/kubernetes/cloud-controller-manager.conf \


do we need this explicit kubeconfig flag given that we specify --use-service-account-credentials=true below?

It's ok to remove the kubeconfig flag

elmiko · 2021-09-16T21:50:36Z

i think we might be having some issues with our usage of --use-service-account-credentials=true, i've tried to capture what i think is happening in this issue openshift/cloud-provider-alibaba-cloud#6

elmiko · 2021-09-17T19:31:50Z

i think this will need to update the https://github.com/openshift/cluster-cloud-controller-manager-operator/blob/master/manifests/0000_26_cloud-controller-manager-operator_01_images.configmap.yaml file as well.

gujingit · 2021-09-22T07:29:08Z

Hi, what's the default image reference for alibaba ccm? https://github.com/openshift/cluster-cloud-controller-manager-operator/blob/0083ec2523f754f7e9e1b6bf861c20db25cf297e/pkg/cloud/cloud_test.go#L44

lobziik · 2021-09-22T13:23:51Z

Hi, what's the default image reference for alibaba ccm?

cluster-cloud-controller-manager-operator/pkg/cloud/cloud_test.go

Line 44 in 0083ec2

CloudControllerManagerOperator: "registry.ci.openshift.org/openshift:cluster-cloud-controller-manager-operator",

@gujingit Hi!

you could use quay.io/repository/openshift/origin-alibaba-cloud-controller-manager for alibaba ccm image reference, i think

kwoodson · 2021-10-28T01:59:01Z

@gujingit
Please run make fmt to clean up formatting.
Please also run make unit to verify the unit tests are passing. I see a few errors for the Alibaba test.

gujingit · 2021-10-29T04:53:57Z

@kwoodson We have updated the dockerfile of https://github.com/kubernetes/cloud-provider-alibaba-cloud in order to pass the unit tests, please pull the latest code. Thanks.

JoelSpeed · 2021-11-04T10:15:47Z

/approve

/hold

I just want to check that we have the alibaba CCM included in the release payload before we go ahead with merging this. Otherwise I think it's good to go

openshift-ci · 2021-11-04T10:17:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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~~ [JoelSpeed]

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

kwoodson · 2021-11-08T18:50:48Z

@JoelSpeed How do we verify the Alibaba CCM is in the release payload?

JoelSpeed · 2021-11-09T09:29:40Z

@Fedosin has put in the requests for this to be done, perhaps he can give an update

kwoodson · 2021-11-15T20:08:28Z

@JoelSpeed I have seen the latest image is being built in the 4.10 image. I saw the ose-alibaba-cloud-controller-manager.yaml.

I think this PR is ready.

elmiko

/lgtm

are we including the images yet @JoelSpeed ?

JoelSpeed · 2021-11-23T10:41:53Z

The images are still marked as WIP, we need to bring in some changes from the upstream provider to fix the build before we can merge this

kwoodson · 2021-11-23T16:00:43Z

@elmiko @JoelSpeed Looks like the fix to the -mod readonly has merged here kubernetes/cloud-provider-alibaba-cloud@ef08fd2. What is the process to force an update?

elmiko · 2021-11-23T16:55:41Z

@elmiko @JoelSpeed Looks like the fix to the -mod readonly has merged here kubernetes/cloud-provider-alibaba-cloud@ef08fd2. What is the process to force an update?

good question, i imagine we will need to update our code repo to match but the complicating factor here is the refactor of the code base. if we are good to move forward with testing the latest releases to the ccm then we could manually make a merge from the upstream to our copy. i think @JoelSpeed was looking into this.

kwoodson · 2021-11-24T01:01:12Z

@gujingit Now that the CCM has been updated, I found that there are a couple of options that are not supported. These two options are causing a failure to start the CCM:

unknown flag: --allow-untagged-cloud
unknown flag: --allocate-node-cidrs

Is there a plan to revisit this PR and test that it works? I also noticed that when launching the container that I received the following error message:

/bin/sh: /cloud-controller-manager: not found

I was able to fix this by using CGO_ENABLED=0. This enabled me to run the container.

gujingit · 2021-11-24T07:48:45Z

@gujingit Now that the CCM has been updated, I found that there are a couple of options that are not supported. These two options are causing a failure to start the CCM:
unknown flag: --allow-untagged-cloud
unknown flag: --allocate-node-cidrs
Is there a plan to revisit this PR and test that it works? I also noticed that when launching the container that I received the following error message:
/bin/sh: /cloud-controller-manager: not found
I was able to fix this by using CGO_ENABLED=0. This enabled me to run the container.

@kwoodson @mtulio we have added these options, please pull the upstream

kwoodson · 2021-12-02T15:20:57Z

Barring any other feedback I believe this is ready to go.

cc @JoelSpeed @elmiko

JoelSpeed · 2021-12-02T16:14:06Z

/hold cancel

openshift-bot · 2021-12-02T16:32:13Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

kwoodson · 2021-12-02T16:34:23Z

/retest-required

openshift-bot · 2021-12-02T18:43:21Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-12-02T20:38:46Z

@gujingit: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/verify	`f70a24b`	link	`/test verify`
ci/prow/e2e-vsphere-ccm	`a49b6c3`	link	false	`/test e2e-vsphere-ccm`
ci/prow/e2e-gcp-ccm	`a49b6c3`	link	false	`/test e2e-gcp-ccm`
ci/prow/e2e-vsphere-ccm-install	`a49b6c3`	link	false	`/test e2e-vsphere-ccm-install`
ci/prow/e2e-azure	`a49b6c3`	link	false	`/test e2e-azure`
ci/prow/e2e-gcp-ccm-install	`a49b6c3`	link	false	`/test e2e-gcp-ccm-install`
ci/prow/e2e-aws-ccm-install	`a49b6c3`	link	false	`/test e2e-aws-ccm-install`
ci/prow/e2e-azure-upgrade	`a49b6c3`	link	false	`/test e2e-azure-upgrade`
ci/prow/e2e-azure-ccm-install	`a49b6c3`	link	false	`/test e2e-azure-ccm-install`
ci/prow/e2e-openstack-ccm	`a49b6c3`	link	false	`/test e2e-openstack-ccm`

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.

openshift-bot · 2021-12-02T20:39:15Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-12-02T21:05:13Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci Bot requested review from Fedosin and lobziik September 13, 2021 03:26

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 Sep 13, 2021

JoelSpeed reviewed Sep 13, 2021

View reviewed changes

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 Sep 13, 2021

elmiko reviewed Sep 15, 2021

View reviewed changes

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

gujingit force-pushed the master branch from f70a24b to d8943c6 Compare September 22, 2021 07:25

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

This was referenced Sep 22, 2021

alicloud: unable to split instanceid and region from providerID openshift/cloud-provider-alibaba-cloud#9

Closed

Enable Alibaba Cloud provider instances with expected ProviderID openshift/machine-config-operator#2777

Merged

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

gujingit force-pushed the master branch from d8943c6 to c05b098 Compare October 19, 2021 02:29

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

gujingit force-pushed the master branch from c05b098 to 2ec0e08 Compare October 29, 2021 04:47

support Alibaba Cloud ccm

0641bc6

gujingit force-pushed the master branch from 2ec0e08 to 0641bc6 Compare October 29, 2021 04:51

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 4, 2021

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

remove unneccessary options

a49b6c3

elmiko reviewed Nov 22, 2021

View reviewed changes

openshift-ci Bot assigned elmiko Nov 22, 2021

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

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2021

openshift-merge-robot merged commit 2c0f143 into openshift:master Dec 2, 2021

	name: "AlibabaCloud resources are empty",
	name: "AlibabaCloud resources returned as expected",

Conversation

gujingit commented Sep 13, 2021

Uh oh!

openshift-ci Bot commented Sep 13, 2021

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Sep 13, 2021

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Sep 13, 2021

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Sep 13, 2021

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Sep 13, 2021

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Sep 13, 2021

Uh oh!

JoelSpeed commented Sep 15, 2021

Uh oh!

elmiko Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

gujingit Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

elmiko commented Sep 16, 2021

Uh oh!

elmiko commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gujingit commented Sep 22, 2021

Uh oh!

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

Uh oh!

Uh oh!

kwoodson commented Oct 28, 2021

Uh oh!

gujingit commented Oct 29, 2021

Uh oh!

JoelSpeed commented Nov 4, 2021

Uh oh!

openshift-ci Bot commented Nov 4, 2021

Uh oh!

kwoodson commented Nov 8, 2021

Uh oh!

JoelSpeed commented Nov 9, 2021

Uh oh!

kwoodson commented Nov 15, 2021

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Nov 23, 2021

Uh oh!

kwoodson commented Nov 23, 2021

Uh oh!

elmiko commented Nov 23, 2021

Uh oh!

kwoodson commented Nov 24, 2021

Uh oh!

gujingit commented Nov 24, 2021

Uh oh!

kwoodson commented Dec 2, 2021

Uh oh!

JoelSpeed commented Dec 2, 2021

Uh oh!

openshift-bot commented Dec 2, 2021

Uh oh!

kwoodson commented Dec 2, 2021

Uh oh!

openshift-bot commented Dec 2, 2021

Uh oh!

openshift-ci Bot commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elmiko commented Sep 17, 2021 •

edited

Loading

lobziik commented Sep 22, 2021 •

edited

Loading

openshift-ci Bot commented Dec 2, 2021 •

edited

Loading