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

[database]: Fix "program:redisprogram:redis_bmp" ERR from supervisor … #20897

Conversation

wdoekes
Copy link
Contributor

@wdoekes wdoekes commented Nov 22, 2024

Why I did it

Backport of 4807ac5 for 202405.

The problem isn't there yet, but this fix is correct. And the problem might crop up once someone starts using two DBs.

Original issue

Issue: #20636
PR-for-master: #20726

@FengPan-Frank can you merge this one as well?

…onic-net#20726)

== Why I did it ==

Commit 06c469e added an extra redis instance. This resulted in a
two item string without linefeeds in /etc/supervisor/critical_processes:

  program:redisprogram:redis_bmp

That resulted in an error in syslog and docker-database failing.

== Work item tracking ==

ERR database#supervisor-proc-exit-listener: Syntax of the line
  program:redisprogram:redis_bmp#012 in processes file is incorrect.
  Exiting... (sonic-net#20636)

#17

== How I did it ==

Replace the jinja2 whitespace eating hyphens from BOL to EOL.

Note that j2 and the jinja2 parser in sonig-cfggen do not behave the
same. The sonig-cfggen is the relevant one.

Before:

  $ j2 ./dockers/docker-database/critical_processes.j2.old -f json \
      <<< '{"INSTANCES":{"foo":"bar","baz":"..."}}'
  |
  |
  program:foo
  program:baz

  # docker exec database sonic-cfggen \
      -j /var/run/redis/sonic-db/database_config.json \
      -t /usr/share/sonic/templates/critical_processes.j2.old
  program:redisprogram:redis_bmp

After:

  $ j2 ./dockers/docker-database/critical_processes.j2 -f json \
      <<< '{"INSTANCES":{"foo":"bar","baz":"..."}}'
  program:foo
  program:baz

  # docker exec database sonic-cfggen \
      -j /var/run/redis/sonic-db/database_config.json \
      -t /usr/share/sonic/templates/critical_processes.j2
  program:redis
  program:redis_bmp
  |

After this fix, the output in /etc/supervisor/critical_processes is
correct and the error from docker-database is gone.
@wdoekes wdoekes requested a review from lguohan as a code owner November 22, 2024 10:17
Copy link
Contributor

@FengPan-Frank FengPan-Frank left a comment

Choose a reason for hiding this comment

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

I remember we only need to add a label to backport.
btw, I'm not sure if we have real multiple redis instance use case for 202405, since redis_bmp was not in 202405.

@wdoekes
Copy link
Contributor Author

wdoekes commented Nov 22, 2024

I remember we only need to add a label to backport.

Not sure what you mean. Do labels fix backports automatically? I have no permissions to add them either way.

btw, I'm not sure if we have real multiple redis instance use case for 202405, since redis_bmp was not in 202405

Indeed we don't. But someone might a second instance in their stable branch. And by merging this they won't run into this problem.

@FengPan-Frank
Copy link
Contributor

I remember we only need to add a label to backport.

Not sure what you mean. Do labels fix backports automatically? I have no permissions to add them either way.

btw, I'm not sure if we have real multiple redis instance use case for 202405, since redis_bmp was not in 202405

Indeed we don't. But someone might a second instance in their stable branch. And by merging this they won't run into this problem.

OK, I've added label

@bingwang-ms
Copy link
Contributor

/azp run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bingwang-ms
Copy link
Contributor

PR #20898 is already merged. Do we still need this PR?

@wdoekes
Copy link
Contributor Author

wdoekes commented Dec 10, 2024

2544e48 (202405) and 4807ac5 (202411 and master) are both included. So yes, this can be closed.

@wdoekes wdoekes closed this Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants