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

Add alibaba cloud provider to machine controller #643

Merged
merged 19 commits into from
Nov 7, 2019
Merged

Add alibaba cloud provider to machine controller #643

merged 19 commits into from
Nov 7, 2019

Conversation

moadqassem
Copy link
Member

@moadqassem moadqassem commented Oct 24, 2019 •
edited

What this PR does / why we need it:
Adding a cloud provider for AlibabaCloud in the machine-controller

Which issue(s) this PR fixes
Fixes #4356

NONE

@kubermatic-bot kubermatic-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. approved Indicates a PR has been approved by an approver from all required OWNERS files. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 24, 2019
@moadqassem
Copy link
Member Author

/test pull-machine-controller-build

@alvaroaleman
Copy link
Contributor

@moadqassem You need to rebase it onto master first, it become incompatible

@moadqassem moadqassem force-pushed the add-alibaba-cloud-provider-to-machine-controller branch 2 times, most recently from defb326 to 1018db2 Compare October 24, 2019 21:52
@kubermatic-bot kubermatic-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2019
@moadqassem moadqassem force-pushed the add-alibaba-cloud-provider-to-machine-controller branch from 1018db2 to 4375810 Compare October 24, 2019 21:59
@kubermatic-bot kubermatic-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2019
@moadqassem moadqassem force-pushed the add-alibaba-cloud-provider-to-machine-controller branch 2 times, most recently from 7af82e5 to 9e3f287 Compare October 24, 2019 22:13
@moadqassem
Copy link
Member Author

/retest pull-machine-controller-e2e-alibaba

@alvaroaleman
Copy link
Contributor

/test pull-machine-controller-e2e-alibaba

@moadqassem moadqassem force-pushed the add-alibaba-cloud-provider-to-machine-controller branch 2 times, most recently from c03827f to ba6be5a Compare October 29, 2019 11:13
@moadqassem
Copy link
Member Author

/test pull-machine-controller-e2e-alibaba

moadqassem and others added 10 commits October 29, 2019 23:01
@moadqassem
adding alibaba cloud provider
5a6ec26 
Signed-off-by: Moath Qasim <moad.qassem@gmail.com>

Signed-off-by: Moath Qasim <moad.qassem@gmail.com>
@nikhita @moadqassem
Init Alibaba provider (#630)
594704b 
* Init Alibaba provider

* Update dependencies

* Add boilerplate header

* Add example deployment file

* Add e2e test for Alibaba

* Add omitempty json tags
@nikhita @moadqassem
Fix CreateInstanceRequest
95e3a29
@moadqassem
adding the instance creation in Alibaba
8463c82 
Signed-off-by: Moath Qasim <moad.qassem@gmail.com>

Signed-off-by: Moath Qasim <moad.qassem@gmail.com>
@moadqassem
adding disk creation and some refactoring
b876d91 
Signed-off-by: Moath Qasim <moad.qassem@gmail.com>

Signed-off-by: Moath Qasim <moad.qassem@gmail.com>
@nikhita @moadqassem
Derive imageID from providerconfig
7c712e4
@jiachengxu @moadqassem
some cleanup of alibaba cloud (#632)
393a86d 
* some cleanup of alibaba cloud

* dep ensure
@moadqassem
adding the alibaba provider and updating the testing template
110c007 
Signed-off-by: Moath Qasim <moad.qassem@gmail.com>

Signed-off-by: Moath Qasim <moad.qassem@gmail.com>
@moadqassem
update vendors
ece00cd 
Signed-off-by: Moath Qasim <moad.qassem@gmail.com>

Signed-off-by: Moath Qasim <moad.qassem@gmail.com>
@moadqassem
fix the name of the testing template for alibaba cloud provider
76dd4a7 
Signed-off-by: Moath Qasim <moad.qassem@gmail.com>

Signed-off-by: Moath Qasim <moad.qassem@gmail.com>
@moadqassem moadqassem force-pushed the add-alibaba-cloud-provider-to-machine-controller branch from e337ca3 to 2733908 Compare November 5, 2019 15:58
@moadqassem
Adding a finalizer and refactoring error messages
48f0968 
Signed-off-by: Moath Qasim <moad.qassem@gmail.com>
@moadqassem
Copy link
Member Author

/test pull-machine-controller-e2e-alibaba

@moadqassem
Copy link
Member Author

/retest

@moadqassem
Copy link
Member Author

/test pull-machine-controller-e2e-alibaba

2 similar comments
@moadqassem
Copy link
Member Author

/test pull-machine-controller-e2e-alibaba

@moadqassem
Copy link
Member Author

/test pull-machine-controller-e2e-alibaba

@moadqassem
Copy link
Member Author

/test pull-machine-controller-e2e-vsphere

@moadqassem moadqassem force-pushed the add-alibaba-cloud-provider-to-machine-controller branch from 6d0829a to cc2554f Compare November 6, 2019 11:28
@moadqassem
Copy link
Member Author

/test pull-machine-controller-e2e-ubuntu-upgrade

@moadqassem
Copy link
Member Author

/test pull-machine-controller-e2e-alibaba

@moadqassem
Copy link
Member Author

/retest

alvaroaleman
alvaroaleman reviewed Nov 6, 2019
View reviewed changes
pkg/cloudprovider/provider/alibaba/provider.go Outdated
}

createInstanceRequest := ecs.CreateCreateInstanceRequest()
createInstanceRequest.ImageId, _ = p.getImageIDForOS(machine.Spec, pc.OperatingSystem)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can we handle the error here?

pkg/cloudprovider/provider/alibaba/provider.go

func getInstance(client *ecs.Client, instanceName string) (*ecs.Instance, error) {
describeInstanceRequest := ecs.CreateDescribeInstancesRequest()
describeInstanceRequest.InstanceName = instanceName
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow, how does that code address anything? The code here still filters the retrieved instance only based on the name and not on the tags

pkg/cloudprovider/provider/alibaba/provider.go Outdated
return nil, fmt.Errorf("failed to get alibaba instance %v due to %v", machine.Name, err)
}

tag := ecs.AddTagsTag{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please configure the tags as part of the creation by setting it on the createInstanceRequest to make this atomic. Otherwise if the creation succeeds at the labeling does not, we won't find our instance in the Get func and will orphan it

pkg/cloudprovider/provider/alibaba/provider.go
deleteInstancesRequest.InstanceId = &[]string{foundInstance.InstanceId}

deleteInstancesRequest.Force = requests.Boolean("True")
if _, err = client.DeleteInstances(deleteInstancesRequest); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update this code to ignore 404, i.E. if someone deleted the instance out of bounds, else we are stuck here because we won't remove the finalizer but the deletion doesn't succeed here

Copy link
Member Author

@moadqassem moadqassem Nov 6, 2019 •
edited

Choose a reason for hiding this comment

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

We do this already here:
https://github.com/kubermatic/machine-controller/pull/643/files/cc2554f14d210965e8c11b7945929038359302e7#diff-e96854fadd3b15014f29f87ac940a01bR252-R257

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. But then you need to move the finalizer cleanup after the if err == cloudprovidererrors.ErrInstanceNotFound

@moadqassem
Adding a machine UID tag to created instance
7898134 
Signed-off-by: Moath Qasim <moad.qassem@gmail.com>
@moadqassem moadqassem force-pushed the add-alibaba-cloud-provider-to-machine-controller branch from cc2554f to 7898134 Compare November 6, 2019 19:26
@moadqassem
Copy link
Member Author

/retest

@moadqassem
Copy link
Member Author

@alvaroaleman PTAL

alvaroaleman
alvaroaleman reviewed Nov 7, 2019
View reviewed changes
Copy link
Contributor

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

One point regarding the cleanup, other than that it looks good

pkg/cloudprovider/provider/alibaba/provider.go
deleteInstancesRequest.InstanceId = &[]string{foundInstance.InstanceId}

deleteInstancesRequest.Force = requests.Boolean("True")
if _, err = client.DeleteInstances(deleteInstancesRequest); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see. But then you need to move the finalizer cleanup after the if err == cloudprovidererrors.ErrInstanceNotFound

@moadqassem
Remove the finalizer when instance is not found
0983487 
Signed-off-by: Moath Qasim <moad.qassem@gmail.com>
@moadqassem moadqassem force-pushed the add-alibaba-cloud-provider-to-machine-controller branch from 5cdf351 to 0983487 Compare November 7, 2019 09:06
alvaroaleman
alvaroaleman approved these changes Nov 7, 2019
View reviewed changes
Copy link
Contributor

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2019
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 78342079182292998f6166c52a17cf88e956e24d

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, moadqassem

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:
  • OWNERS [alvaroaleman,moadqassem]

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

@moadqassem
Copy link
Member Author

/test pull-machine-controller-e2e-alibaba

@moadqassem
Copy link
Member Author

moadqassem commented Nov 7, 2019 •
edited

/retest

@kubermatic-bot kubermatic-bot merged commit 655808e into master Nov 7, 2019
@kubermatic-bot kubermatic-bot deleted the add-alibaba-cloud-provider-to-machine-controller branch November 7, 2019 09:46
@kubermatic-bot
Copy link
Contributor

@moadqassem: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-machine-controller-e2e-alibaba 0983487 link /test pull-machine-controller-e2e-alibaba

Full PR test history

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvaroaleman alvaroaleman alvaroaleman approved these changes

Assignees

@alvaroaleman alvaroaleman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@moadqassem @alvaroaleman @kubermatic-bot @nikhita @jiachengxu