-
Notifications
You must be signed in to change notification settings - Fork 580
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
RKE host structure and config changes #56
Conversation
bc73ca9
to
8f0f9d1
Compare
@alena1108 @moelsayed Can you please review? |
8f0f9d1
to
62b2de1
Compare
363ff98
to
7bf08eb
Compare
@@ -90,7 +90,7 @@ func IsHostListChanged(currentHosts, configHosts []Host) bool { | |||
for _, host := range currentHosts { | |||
found := false | |||
for _, configHost := range configHosts { | |||
if host.AdvertisedHostname == configHost.AdvertisedHostname { | |||
if host.Address == configHost.Address { | |||
found = true |
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 should break when found=true
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.
fixed it
cluster/validation.go
Outdated
if role == services.ControlRole { | ||
bothRoles[services.ControlRole] = true | ||
} | ||
if role == services.WorkerRole { |
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.
should be elseif
cluster/validation.go
Outdated
return fmt.Errorf("Role [%s] for host (%d) is not recognized", role, i+1) | ||
} | ||
} | ||
bothRoles := make(map[string]bool) |
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.
any reason why we use a map here? there are only 2 values, workerRole and controlRole, may be can use bool instead to simplify the outlook?
cluster/validation.go
Outdated
@@ -13,16 +15,79 @@ func (c *Cluster) ValidateCluster() error { | |||
if len(c.EtcdHosts) == 0 { |
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.
Should we validate number of etcd hosts (it should be an odd number)?
cluster/validation.go
Outdated
} | ||
|
||
func validateNetworkOptions(c *Cluster) error { | ||
if c.Network.Plugin != "flannel" && c.Network.Plugin != "calico" && c.Network.Plugin != "canal" { |
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.
I suggest you define the constants for the network plugins.
7bf08eb
to
41c4887
Compare
) | ||
|
||
func (c *Cluster) ValidateCluster() error { | ||
// make sure cluster has at least one controlplane/etcd host | ||
if len(c.ControlPlaneHosts) == 0 { | ||
return fmt.Errorf("Cluster must have at least one control plane host") | ||
} | ||
if len(c.EtcdHosts) == 0 { | ||
return fmt.Errorf("Cluster must have at least one etcd host") | ||
if len(c.EtcdHosts)%2 == 0 { |
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.
what if len is 0?
#53
#52
#50
#41
#36
IP
toAddress
AdvertiseAddress
toInternalAddress
AdvertisedHostname
toHostnameOverride
Hostname will be optional, and if not set it will be equal to Address.