Remove NodeIP configurable option from microshift config #1063
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ var ( | |
| configFile = findConfigFile() | ||
| dataDir = findDataDir() | ||
| manifestsDir = findManifestsDir() | ||
| nodeIP = findNodeIP() | ||
| ) | ||
|
|
||
| type ClusterConfig struct { | ||
|
|
@@ -60,7 +61,6 @@ type MicroshiftConfig struct { | |
| Roles []string `json:"roles"` | ||
|
|
||
| NodeName string `json:"nodeName"` | ||
|
|
||
| Cluster ClusterConfig `json:"cluster"` | ||
| Debug DebugConfig `json:"debug"` | ||
|
|
@@ -78,6 +78,18 @@ func GetManifestsDir() []string { | |
| return manifestsDir | ||
| } | ||
|
|
||
| func GetNodeIP() string { | ||
| return nodeIP | ||
| } | ||
|
|
||
| func findNodeIP() string { | ||
| hostIP, err := util.GetHostIP() | ||
|
Contributor
There was a problem hiding this comment. We expect hosts to have multiple IPs. How does the user influence how this function picks the IP to use?
Contributor
There was a problem hiding this comment. It seems the nodeIP option is not repected in main branch, even it is user configurable. We probably want to fix it. This influence approach shall also work for microshift-ovs-init service script, which sets up the nodeIP interface for ovn-kubernetes CNI plugin.
Member
There was a problem hiding this comment. In which scenario would a user want to pick a specific IP? The IP to which core components of microshift bind to shouldn't really matter to the end-user since their applications will most likely only communicate via the different services.
Contributor
There was a problem hiding this comment. I remember that some popular distros also supported specifying the interface name or something like IP(eth0) to make it work with DHCP too
Member
There was a problem hiding this comment. If I am not mistaken, the nodeIP is not even configurable in OCP today. The closest thing I could find is this effort: https://github.com/openshift/enhancements/blob/ef85659d01738b9f89958d5f0da31cff05bb1182/enhancements/network/ip-interface-selection.md but this seem way too complex for MicroShift to need something like that right now.
I am not sure why that would be beneficial to do for control plane components. Another concern I have is what would happen if microshift was first started with a nodeIP, but then a user decided to change it on the fly. Everything that used to depend on the previous nodeIP will not be able to reach the new nodeIP if we don't have a way to propagate the update. I don't think that's supported in k8s in general since one would most likely have to delete the node to update the IP on the various components.
Contributor
There was a problem hiding this comment. Unlike OCP hosts, we have to deal with the fact that a host running MicroShift may change its IP or move between networks. So we either need a way to pick a node IP that will never change, or we need to support it being updated. Could we always use a localhost IP or the hardware's IPv6 local address, for example? What effect would that have? What needs to use the node IP? |
||
| if err != nil { | ||
| klog.Fatalf("failed to get host IP: %v", err) | ||
| } | ||
| return hostIP | ||
| } | ||
|
|
||
| // KubeConfigID identifies the different kubeconfigs managed in the DataDir | ||
| type KubeConfigID string | ||
|
|
||
|
|
@@ -98,18 +110,13 @@ func NewMicroshiftConfig() *MicroshiftConfig { | |
| if err != nil { | ||
| klog.Fatalf("Failed to get hostname %v", err) | ||
| } | ||
|
|
||
| defaultRoles := make([]string, len(validRoles)) | ||
| copy(defaultRoles, validRoles) | ||
| return &MicroshiftConfig{ | ||
| LogVLevel: 0, | ||
| Roles: defaultRoles, | ||
| NodeName: nodeName, | ||
| Cluster: ClusterConfig{ | ||
| URL: "https://127.0.0.1:6443", | ||
| ClusterCIDR: "10.42.0.0/16", | ||
|
|
@@ -220,9 +227,6 @@ func (c *MicroshiftConfig) ReadFromCmdLine(flags *pflag.FlagSet) error { | |
| if s, err := flags.GetString("node-name"); err == nil && flags.Changed("node-name") { | ||
| c.NodeName = s | ||
| } | ||
| if s, err := flags.GetString("url"); err == nil && flags.Changed("url") { | ||
| c.Cluster.URL = s | ||
| } | ||
|
|
||
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.
instead of defining that globally every time, could we perhaps create a structure that could be passed on to the various init functions alongside the configuration?