fix: check if environment settings are achieved #2763
Conversation
ref: longhorn/longhorn#7571 Signed-off-by: James Lu <james.lu@suse.com>
ref: longhorn/longhorn#7571 Signed-off-by: James Lu <james.lu@suse.com>
ref: longhorn/longhorn#7571 Signed-off-by: James Lu <james.lu@suse.com>
ref: longhorn/longhorn#7571 Signed-off-by: James Lu <james.lu@suse.com>
ref: longhorn/longhorn#7571 Signed-off-by: James Lu <james.lu@suse.com>
ref: longhorn/longhorn#7571 Signed-off-by: James Lu <james.lu@suse.com>
|
|
||
| kernelConfigPath := "/boot/config-" + kernelVersion | ||
| args := []string{kernelConfigPath} | ||
| if _, err := nsexec.Execute(nil, "ls", args, lhtypes.ExecuteDefaultTimeout); err != nil { |
There was a problem hiding this comment.
Need to be careful about invoking binaries with nsenter, this could be blocking when installing on Talos Linux.
There was a problem hiding this comment.
Consider utilizing ReadDirectory.
Also, need to check other binary invocations introduced in this PR.
There was a problem hiding this comment.
It is great, thank you.
That is a file, and I would use GetFileInfo.
| currentNodeID, err := util.GetRequiredEnv(types.EnvNodeName) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to detect the node name") | ||
| } | ||
|
|
||
| if err := environmentCheck(kubeconfigPath, currentNodeID); err != nil { | ||
| return errors.Wrap(err, "Failed environment check, please make sure you have iscsiadm/open-iscsi installed on the host") |
There was a problem hiding this comment.
Since environmentCheck() checks everything, not just iscsi, this error message may be misleading.
There was a problem hiding this comment.
Agree with @james-munson's comment.
You can combine the errors in environmentCheck and just do
return errors.Wrap(err, "Failed environment check")
here.
| @@ -244,8 +253,54 @@ func startManager(c *cli.Context) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func environmentCheck() error { | |||
| func environmentCheck(kubeconfigPath, currentNodeID string) error { | |||
There was a problem hiding this comment.
nit:
| func environmentCheck(kubeconfigPath, currentNodeID string) error { | |
| func environmentCheck(kubeconfigPath, nodeID string) error { |
| } | ||
|
|
||
| // Check if kernel version is recommended | ||
| if err := checkKernelVersion(kubeNode, currentNodeID); err != nil { |
There was a problem hiding this comment.
kubeNode already contains the nodeID. Can we remove the parameter currentNodeID?
| if err := checkKernelVersion(kubeNode, currentNodeID); err != nil { | |
| if err := checkKernelVersion(kubeNode); err != nil { |
| namespaces := []lhtypes.Namespace{lhtypes.NamespaceMnt, lhtypes.NamespaceNet} | ||
| // Check if necessary packages are installed | ||
| if err := checkPackagesInstalled(kubeNode, namespaces, currentNodeID); err != nil { |
| } | ||
|
|
||
| // Check if nfs client versions are supported | ||
| if err := checkNFSClientVersion(kubeNode, namespaces, currentNodeID); err != nil { |
| } | ||
| } | ||
|
|
||
| return fmt.Errorf("NFS clients %v not found. At least one should be enabled", nfsClientVersions) |
There was a problem hiding this comment.
If a cluster doesn't require RWX volumes, then NFS server and client are not necessary. Should we error out in this use case?
Which issue(s) this PR fixes:
Issue # longhorn/longhorn#7571
What this PR does / why we need it:
Check if the environment settings are achieved, ref: environment_check.sh
Special notes for your reviewer:
Additional documentation or context