-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Create cluster requirements for DigitalOcean #3248
Conversation
|
||
package resources | ||
|
||
func (c *ClusterResources) listResourcesDO() (map[string]*ResourceTracker, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is temporary, only to get cluster state creation working, going to work on a refactor to make this work better
@@ -141,6 +141,8 @@ func (b *MasterVolumeBuilder) addAWSVolume(c *fi.ModelBuilderContext, name strin | |||
} | |||
|
|||
func (b *MasterVolumeBuilder) addDOVolume(c *fi.ModelBuilderContext, name string, volumeSize int32, subnet *kops.ClusterSubnetSpec, etcd *kops.EtcdClusterSpec, m *kops.EtcdMemberSpec, allMembers []string) { | |||
// required that names start with a lower case and only contains letters, numbers and hyphens | |||
name = "kops-" + strings.Replace(name, ".", "-", -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a good idea to check for a length limit as well - that's the other one that is pretty common. We have this code for ELBs: https://github.com/kubernetes/kops/blob/master/pkg/model/context.go#L75-L91 .
@@ -0,0 +1,2 @@ | |||
KubeAPIServer: | |||
CloudProvider: external |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are trying to move this logic to https://github.com/kubernetes/kops/blob/master/pkg/model/components/apiserver.go
This does rely on the do
tag, which I do see you define later, but soon we hope to have fully moved the tag / options logic to code (it is much easier to understand, IMO)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted. It was really difficult trying to figure out who to change the CloudProvider flag for kube-apiserver, I'll follow up with this in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an issue to clean this up for all clouds #3253
@@ -649,7 +648,7 @@ func (c *ApplyClusterCmd) Run() error { | |||
BootstrapScript: bootstrapScriptBuilder, | |||
Lifecycle: clusterLifecycle, | |||
}) | |||
|
|||
case kops.CloudProviderDO: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to have a // Digital Ocean tasks will go here
comment here, just so we don't think this is a fall-through
@@ -66,6 +66,10 @@ func buildCloudupTags(cluster *api.Cluster) (sets.String, error) { | |||
{ | |||
tags.Insert("_aws") | |||
} | |||
case "digitalocean": | |||
{ | |||
tags.Insert("_do") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully we'll be rid of these tags soon, but I agree that for now we should add them - more logically coherent to be consistent with the other providers, doesn't create a lot more work
/lgtm |
/test all [submit-queue is verifying that this PR is safe to merge] |
/lgtm cancel //PR changed after LGTM, removing LGTM. @andrewsykim @justinsb |
Comments addressed |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim, 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 |
/test all [submit-queue is verifying that this PR is safe to merge] |
/retest |
Flakiness is being tracked in upstream issue: kubernetes/kubernetes#51128, hopefully fixed by kubernetes/kubernetes#51144 |
/retest |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue |
Initial changes required to create a cluster state. Running
kops update cluster --yes
does not work yet.Note that DO has already adopted cloud controller managers (https://github.com/digitalocean/digitalocean-cloud-controller-manager) so we set
--cloud-provider=external
. This will end up being the case for aws, gce and vsphere over the next couple of releases.#2150