Implement volume task for Openstack platform #3893
Conversation
k8s-ci-robot
added
needs-ok-to-test
zengchen1024
force-pushed
the
cinder_volume_task
branch
from
9d1813d to
f9b9e56
Compare
zengchen1024
force-pushed
the
cinder_volume_task
branch
from
f9b9e56 to
0949d59
Compare
|
/assign @zmerlynn |
| @@ -205,3 +212,33 @@ func (b *MasterVolumeBuilder) addGCEVolume(c *fi.ModelBuilderContext, name strin | |||
| func (b *MasterVolumeBuilder) addVSphereVolume(c *fi.ModelBuilderContext, name string, volumeSize int32, zone string, etcd *kops.EtcdClusterSpec, m *kops.EtcdMemberSpec, allMembers []string) { | |||
| fmt.Print("addVSphereVolume to be implemented") | |||
| } | |||
|
|
|||
| func (b *MasterVolumeBuilder) addOpenstackVolume(c *fi.ModelBuilderContext, name string, volumeSize int32, zone string, etcd *kops.EtcdClusterSpec, m *kops.EtcdMemberSpec, allMembers []string) error { | |||
| volumeType := fi.StringValue(m.VolumeType) | |||
There was a problem hiding this comment.
Ideally we would put this in validation as well - we have some cloud specific validation e.g. https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/validation/aws.go
| "k8s.io/kubernetes/federation/pkg/dnsprovider" | ||
| ) | ||
|
|
||
| const TagNameEtcdClusterPrefix = "k8s.io/etcd/" | ||
| const TagNameRolePrefix = "k8s.io/role/" | ||
| const TagClusterName = "KubernetesCluster" |
There was a problem hiding this comment.
FYI, we use this tag because that's what the AWS cloudprovider originally defined for isolating two clusters from each other. Not sure if you have a similar concept in the openstack cloudprovider already - if not I'd recommend choosing a better value. On AWS we're trying to move to k8s.io/cluster/<clustername>: https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/validation/aws.go
| glog.V(4).Infof("setting tags to cinder volume %q: %v", id, tags) | ||
|
|
||
| opt := cinder.UpdateOpts{Metadata: tags} | ||
| done, err := vfs.RetryWithBackoff(writeBackoff, func() (bool, error) { |
There was a problem hiding this comment.
We should probably pull this into a retry package if we're going to be using it throughout the code, but not a problem...
|
/ok-to-test /lgtm Looks great @zengchen1024 :-) A few suggestions but I think this PR is great as-is. |
k8s-ci-robot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-ci-robot
added
the
approved
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
Implement volume task to create volume for ETCD cluster.
Which issue this PR fixes: #3886