Skip to content
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

Reset configmap log_file option #208

Merged
merged 1 commit into from
Dec 23, 2022

Conversation

timdengyun
Copy link
Contributor

The log_file config option in DEFAULT section is used for both nsx-ncp-config and nsx-node-agent-config.
So, the log_file will be used for bootstrap, ncp and node-agent pods once it is set. All pods will write log in the same log_file. However, ncp and node-agent write log_file with ncp user, bootstrap pod write log_file with root user.

The log_file is set in operator configmap, boostrap, ncp and node-agent pod will be restarted. If bootstrap pod create log_file before than ncp/node-agent pods is trying to access. Bootstrap pod creates and write log_file with root users. In such case, the ncp/node-agent will hit Permission denied issue when writing log_file and the ncp/node-agent pod will be crashed.

This patch is to make a workaround solution to fix this issue:

  1. Reset log_file to the default None value so that ncp, bootstrap and node-agent will use their own default logfile, respectively.
  2. Print a log message when detecting log_file option is set by user.

The log_file config option in DEFAULT section is used for both nsx-ncp-config
and nsx-node-agent-config.
So, the log_file will be used for bootstrap, ncp and node-agent pods once it
is set. All pods will write log in the same log_file. However, ncp and node-agent
write log_file with ncp user, bootstrap pod write log_file with root user.

The log_file is set in operator configmap, boostrap, ncp and node-agent pod will
be restarted. If bootstrap pod create log_file before than ncp/node-agent pods is
trying to access. Bootstrap pod creates and write log_file with root users.
In such case, the ncp/node-agent will hit Permission denied issue when writing
log_file and the ncp/node-agent pod will be crashed.

This patch is to make a workaround solution to fix this issue:
1. Reset log_file to the default None value so that ncp, bootstrap and node-agent
   will use their own default logfile, respectively.
2. Print a log message when detecting log_file option is set by user.
@@ -158,6 +168,16 @@ func (adaptor *ConfigMapOc) validateConfigMap(configmap *corev1.ConfigMap) []err
if err != nil {
appendErrorIfNotNil(&errs, errors.Wrapf(err, "mtu is invalid"))
}
// Check DEFAULT section log_file option
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part is the same as 132-141, maybe add a method instead of same code block.

Copy link
Contributor Author

@timdengyun timdengyun Dec 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are trying to merge the two func (adaptor *ConfigMapOc) validateConfigMap(configmap *corev1.ConfigMap) and func (adaptor *ConfigMapK8s) validateConfigMap(configmap *corev1.ConfigMap) since the logic of these two functions are basically the same. but it's a little bit rush to do it in the operator 4.1.0. We can consider this in the next release.

@timdengyun timdengyun merged commit b47d400 into vmware:master Dec 23, 2022
// Revert DEFAULT log_file to default value
fillDefault(cfg, "DEFAULT", "log_file", "", true)
// Write config back to ConfigMap data
(*data)[operatortypes.ConfigMapDataKey], err = iniWriteToString(cfg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to reset the value call iniWriteToString if in any case we are failing validation and therefore setting operator in degraded state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there is failing or error when calling iniWriteToString, we write error in NCP install CRD status, so, it means NCP install CRD is a degraded state. I feel this approach should be ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants