support gossip for AliCloud #6319
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @LilyFaFa. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
the
size/XXL
k8s-ci-robot
added
size/XL
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
k8s-ci-robot
added
the
needs-rebase
|
Looks great! I added some comments about some of the questions. You need to rebase, but... /approve |
k8s-ci-robot
added
the
lgtm
| } | ||
| ecsEndpoint := os.Getenv("ALIYUN_ECS_ENDPOINT") | ||
| if ecsEndpoint == "" { | ||
| // TODO: shall we raise error here? |
There was a problem hiding this comment.
If you have a default, I'd say probably not, but you might log e.g. glog.Infof("Defaulting ALIYUN_ECS_ENDPOINT to %s", ecsEndpoint)
|
|
||
| // AttachVolume attaches the specified volume to this instance, returning the mountpoint & nil if successful | ||
| func (a *ALIVolumes) AttachVolume(volume *Volume) error { | ||
| // TODO: what if this volume has already been attached to another instance? |
There was a problem hiding this comment.
That's actually good - this acts as a sort of mutex for leader election. We do assume that the cloud API won't accidentally let us make two attachments, even if we make concurrent "attach volume" calls.
| } | ||
|
|
||
| // TODO: Do we have to wait for attach to complete? | ||
| // retrieve device info |
There was a problem hiding this comment.
I don't believe you have to wait, no, because we'll wait for the mount later. You do need to know where the volume is going to show up though (the device)
k8s-ci-robot
added
the
approved
k8s-ci-robot
removed
lgtm
|
Thanks for rebasing @LilyFaFa /approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb, LilyFaFa 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:
Approvers can indicate their approval by writing |
To support Gossip DNS for AliCloud
This is part of #4127.