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.

Sign up for GitHub

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

Cleanup LDMSD's command-line options #1369

Merged
merged 11 commits into from
Mar 5, 2024
Merged

Cleanup LDMSD's command-line options #1369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d57eb39
Add the default_auth config command
nichamon Jan 5, 2024
98308f8
Add the set_memory config command
nichamon Jan 9, 2024
0e86bf1
Add the log_file command
nichamon Jan 10, 2024
1e7e6ca
Add publish_kernel command
nichamon Jan 11, 2024
6f20aa6
Add the daemon_name configuraton command
nichamon Jan 12, 2024
820992a
Add the worker_threads command
nichamon Jan 12, 2024
d3f1d3b
Add the default_credits command
nichamon Jan 12, 2024
6e8b776
Add pid_file configuration command
nichamon Feb 21, 2024
5e29983
Add the banner configuration command
nichamon Feb 21, 2024
8f5de72
Make LDMSD create a PID file only when a path is specified
nichamon Mar 4, 2024
d0b7f54
Make LDMSD always run in foreground
nichamon Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 65 additions & 62 deletions ldms/src/ldmsd/ldmsd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ char myname[512]; /* name to identify ldmsd */
/* DEFAULT: myhostname:port */
char myhostname[80];
char ldmstype[20];
int foreground;
int cfg_cntr = 0;
pthread_t event_thread = (pthread_t)-1;
char *logfile;
Expand Down Expand Up @@ -277,7 +276,7 @@ void cleanup(int x, const char *reason)
ldms = NULL;
}

if (!foreground && pidfile) {
if (pidfile) {
unlink(pidfile);
free(pidfile);
pidfile = NULL;
Expand All @@ -289,10 +288,6 @@ void cleanup(int x, const char *reason)
bannerfile = NULL;
}
}
if (pidfile) {
free(pidfile);
pidfile = NULL;
}

ovis_log(NULL, OVIS_LALWAYS, "LDMSD_ cleanup end.\n");

Expand Down Expand Up @@ -1869,6 +1864,13 @@ int ldmsd_process_cmd_line_arg(char opt, char *value)
max_mem_sz_str = strdup(value);
if (!max_mem_sz_str)
return ENOMEM;
max_mem_size = ovis_get_mem_size(max_mem_sz_str);
if (!max_mem_size) {
ovis_log(NULL, OVIS_LCRITICAL,
"Invalid memory size '%s'\n.",
max_mem_sz_str);
return EINVAL;
}
}
break;
case 'c':
Expand Down Expand Up @@ -2040,9 +2042,6 @@ int main(int argc, char *argv[])
opterr = 0;
while ((op = getopt_long(argc, argv, short_opts, long_opts, NULL)) != -1) {
switch (op) {
case 'F':
foreground = 1;
break;
case 'u':
if (check_arg("u", optarg, LO_NAME))
return 1;
Expand Down Expand Up @@ -2070,6 +2069,36 @@ int main(int argc, char *argv[])
case 'c':
/* Handle below */
break;
case 'k':
case 's':
ovis_log(NULL, OVIS_LCRIT,
"The options `-k` and `-s` are obsolete. "
"Please specify `publish_kernel path=<KERNEL_FILE> in a configuration file.\n");
cleanup(EINVAL, "Received an obsolete command-line option");
case 'n':
ovis_log(NULL, OVIS_LCRIT,
"The option `-n` is obsolete. "
"Please specify `daemon_name name=<DAEMON_NAME> in a configuration file.\n");
cleanup(EINVAL, "Received an obsolete command-line option");
case 'P':
ovis_log(NULL, OVIS_LCRIT,
"The option `-P` is obsolete. "
"Please specify `worker_threads num=<NUMBER OF THREADS> in a configuration file.\n");
cleanup(EINVAL, "Received an obsolete command-line option");
case 'C':
ovis_log(NULL, OVIS_LCRIT,
"The option `-C` is obsolete. "
"Please specify `default_credits credits=<INTEGER> in a configuration file.\n");
cleanup(EINVAL, "Received an obsolete command-line option");
case 'B':
ovis_log(NULL, OVIS_LCRIT,
"The option `-B` is obsolete. "
"Please specify `banner mode=<0|1|2>` in a configuration file.");
cleanup(EINVAL, "Received an obsolete command-line option");
case 'F':
ovis_log(NULL, OVIS_LCRIT,
"The option `-F` is obsolete. ");
cleanup(EINVAL, "Received an obsolete command-line option");
default:
ret = ldmsd_process_cmd_line_arg(op, optarg);
if (ret) {
Expand Down Expand Up @@ -2097,13 +2126,6 @@ int main(int argc, char *argv[])
exit(0);
}

if (!foreground) {
if (daemon(1, 1)) {
perror("ldmsd: ");
cleanup(8, "daemon failed to start");
}
}

/*
* TODO: It should be a better way to get this information.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nichamon can you elaborate on this comment?

Copy link
Collaborator Author

@nichamon nichamon Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I added the comment, when I refactored LDMSD to use the ovis_log library, as a reminder to find a better way for admins to change the time format of log messages from date-time to timestamp. Currently, admins can configure LDMSD to use timestamps in seconds from the epoch in log messages by setting the environment variable LDMSD_LOG_TIME_SEC. I wasn't a fan of this approach because it is hard to use.

Since I introduce this, there haven't been any use cases. Here are two path forwards:

  1. We decide there's no practical use for multiple time formats: In this case, I will remove the code block.
  2. We decide supporting multiple formats may benefit in the future: I would change the new log_file config command from
    log_file path=<log file path>
    to
    log dest=<log file path> [time_format=<date-time|timestamp|none>].
    The ovis_log library supports the three modes: date-time (LDMSD default), timestamp in second from epoch, and no time information. Beside modifying the config command, I would remove the logic to get the time format from the environment. The only way to change the time format in LDMSD's log messages is to use the config command `log.

What do you think?

*/
Expand Down Expand Up @@ -2164,7 +2186,6 @@ int main(int argc, char *argv[])
if ((max_mem_size = ovis_get_mem_size(max_mem_sz_str)) == 0) {
ovis_log(NULL, OVIS_LCRITICAL, "Invalid memory size '%s'. "
"See the -m option.\n", max_mem_sz_str);
usage(argv);
}
if (ldms_init(max_mem_size)) {
ovis_log(NULL, OVIS_LCRITICAL, "LDMS could not pre-allocate "
Expand All @@ -2173,25 +2194,7 @@ int main(int argc, char *argv[])
exit(1);
}

if (!foreground) {
/* Create pidfile for daemon that usually goes away on exit. */
/* user arg, then env, then default to get pidfile name */
if (!pidfile) {
char *pidpath = getenv("LDMSD_PIDFILE");
if (!pidpath) {
pidfile = malloc(strlen(LDMSD_PIDFILE_FMT)
+ strlen(basename(argv[0]) + 1));
if (pidfile)
sprintf(pidfile, LDMSD_PIDFILE_FMT, basename(argv[0]));
} else {
pidfile = strdup(pidpath);
}
if (!pidfile) {
ovis_log(NULL, OVIS_LERROR, "Out of memory\n");
av_free(auth_opt);
exit(1);
}
}
if (pidfile) {
if( !access( pidfile, F_OK ) ) {
ovis_log(NULL, OVIS_LERROR, "Existing pid file named '%s': %s\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nichamon, @valleydlr do we need this message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems this message could be helpful in identifying misconfiguration of the -r in LDMSD. For example, if two LDMSDs are configured to create a PID file at /home/my/var/pid/ldmsd.pid on different nodes that share the same home directory, they will overwrite each other's PID files.This scenario is more likely to occur in a test environment. Ultimately, I am neutral on whether to keep or remove the message.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nichamon, I think this is reasonable.

pidfile, "overwritten if writable");
Expand All @@ -2208,27 +2211,28 @@ int main(int argc, char *argv[])
fprintf(pfile,"%ld\n",(long)mypid);
fclose(pfile);
}
if (pidfile && banner) {
char *suffix = ".version";
bannerfile = malloc(strlen(suffix)+strlen(pidfile)+1);
if (!bannerfile) {
ovis_log(NULL, OVIS_LCRITICAL, "Memory allocation failure.\n");
av_free(auth_opt);
exit(1);
}
sprintf(bannerfile, "%s%s", pidfile, suffix);
if( !access( bannerfile, F_OK ) ) {
ovis_log(NULL, OVIS_LERROR, "Existing banner file named '%s': %s\n",
bannerfile, "overwritten if writable");
}
FILE *bfile = fopen_perm(bannerfile,"w", LDMSD_DEFAULT_FILE_PERM);
if (!bfile) {
int banerr = errno;
ovis_log(NULL, OVIS_LERROR, "Could not open the banner file named '%s': %s\n",
bannerfile, STRERROR(banerr));
free(bannerfile);
bannerfile = NULL;
} else {
}
if (pidfile && banner) {
char *suffix = ".version";
bannerfile = malloc(strlen(suffix)+strlen(pidfile)+1);
if (!bannerfile) {
ovis_log(NULL, OVIS_LCRITICAL, "Memory allocation failure.\n");
av_free(auth_opt);
exit(1);
}
sprintf(bannerfile, "%s%s", pidfile, suffix);
if( !access( bannerfile, F_OK ) ) {
ovis_log(NULL, OVIS_LERROR, "Existing banner file named '%s': %s\n",
bannerfile, "overwritten if writable");
}
FILE *bfile = fopen_perm(bannerfile,"w", LDMSD_DEFAULT_FILE_PERM);
if (!bfile) {
int banerr = errno;
ovis_log(NULL, OVIS_LERROR, "Could not open the banner file named '%s': %s\n",
bannerfile, STRERROR(banerr));
free(bannerfile);
bannerfile = NULL;
} else {

#define BANNER_PART1_A "Started LDMS Daemon with authentication "
#define BANNER_PART1_NOA "Started LDMS Daemon without authentication "
Expand All @@ -2241,13 +2245,12 @@ int main(int argc, char *argv[])
ldms_version.flags, OVIS_GIT_LONG

#if OVIS_LDMS_HAVE_AUTH
fprintf(bfile, BANNER_PART1_A
fprintf(bfile, BANNER_PART1_A
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nichamon, @valleydlr did we talk about getting rid of this banner file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

According to the spreadsheet we went over together in Austin (I left early), it was decided to remove the command-line option -B and add a new config command to replace the option.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@valleydlr, do you want to retain the Banner file?

#else /* OVIS_LDMS_HAVE_AUTH */
fprintf(bfile, BANNER_PART1_NOA
fprintf(bfile, BANNER_PART1_NOA
#endif /* OVIS_LDMS_HAVE_AUTH */
BANNER_PART2);
fclose(bfile);
}
BANNER_PART2);
fclose(bfile);
}
}

Expand Down
Loading
Loading