-
Notifications
You must be signed in to change notification settings - Fork 3
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
Adding pika authentication and sleep time to queue consumer #221
Conversation
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.
Hey Luis, got some comments here.
Dockerfile
Outdated
WORKDIR /usr/src/app | ||
|
||
<<<<<<< HEAD |
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.
This should be from the conflict between the two branches. We should fix this.
# subscribe to the corresponding queue | ||
SUB_QUEUE = os.environ.get("SUB_QUEUE") | ||
|
||
MQ_SRVC = os.environ.get("MQ_SRVC") |
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.
If there are new environment variables added, they should be added to the env.template, so we can have examples when deploying this.
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.
Hi @congwang09, @sajith, MQ_SRVC is a docker-compose variable that receives the existing .env variable RABBITMQ_DEFAULT_HOST, which has the docker-compose service name rabbitmq3
logger = logging.getLogger(__name__) | ||
|
||
|
||
class RpcConsumer(object): | ||
def __init__(self, thread_queue, exchange_name, topology_manager): | ||
self.logger = logging.getLogger(__name__) | ||
SLEEP_TIME = 5 |
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.
The constant should be defined on top of the file. Also it'll be nice to also set it as an env var, with a default value. This makes it configurable.
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.
Why do we a need to add a delay anyway?
I suspect this is to wait for the RabbitMQ service to start up, in the case of RabbitMQ and SDX Controller spawned by the same compose file. If that is the case, we should be using depends_on in the compose file.
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.
Hi @sajith, docker depends on is only responsible for managing the status of Docker services, not the internal services running inside the container, such as RabbitMQ. To avoid repetition, I plan to add a new variable to the .env file, as we will use this value multiple times in sdx lc and controller.
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.
I do not understand. The message queue would not be running in the same container. What is the delay for?
@@ -38,9 +46,17 @@ def on_request(self, ch, method, props, message_body): | |||
response = message_body | |||
self._thread_queue.put(message_body) | |||
|
|||
self.logger = logging.getLogger(__name__) | |||
SLEEP_TIME = 5 |
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.
This has been set in L25.
@@ -38,9 +46,17 @@ def on_request(self, ch, method, props, message_body): | |||
response = message_body | |||
self._thread_queue.put(message_body) | |||
|
|||
self.logger = logging.getLogger(__name__) |
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.
This logger has been set in L24.
self.connection = pika.BlockingConnection( | ||
pika.ConnectionParameters(host=MQ_HOST) | ||
pika.ConnectionParameters(MQ_SRVC, 5672, "/", credentials) |
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.
It would be nice to have the MQ port number configurable, as an env var.
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.
Sure, I will add this as well. we need to update sdx lc and controller with this change
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.
The way I have understood, we can achieve password auth with something like this:
MQ_USER = os.getenv("MQ_USER") or "guest"
MQ_PASS = os.getenv("MQ_PASS") or "guest"
creds = pika.PlainCredentials(username=MQ_USER, password=MQ_PASS)
params = pika.ConnectionParameters(host=MQ_HOST, credentials=creds)
That is the general idea. Adding a sleep time is probably unnecessary, and if there is a reason, we should document it.
Also please be sure to run the tests in your local development setup. How to do this is documented in the README file of this repository. If something is unclear, we should address that.
Dockerfile
Outdated
FROM python:3.9-slim-bullseye | ||
|
||
RUN apt-get update \ | ||
&& apt-get -y upgrade \ | ||
&& apt-get install -y --no-install-recommends gcc python3-dev git \ | ||
&& apt-get clean \ | ||
&& rm -rf /var/lib/apt/lists/* \ | ||
&& mkdir -p /usr/src/app | ||
FROM flask-base | ||
|
||
RUN mkdir -p /usr/src/app | ||
WORKDIR /usr/src/app | ||
|
||
<<<<<<< HEAD | ||
COPY ./container-sdx-controller /usr/src/app | ||
======= | ||
WORKDIR /usr/src/app | ||
COPY . /usr/src/app | ||
>>>>>>> main |
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.
These changes to Dockerfile
are not necessary, and they are not within the scope of this PR. They contain errors too, with git merge markers. Please revert the changes.
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.
There is a different Dockerfile that needs to be used while running in compose mode. To avoid affecting the original code, I moved the file to a different directory. There was an error on Github, and although I did a merge, the file was not updated. Therefore, I will update it manually.
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.
This happens when there's conflict between your branch and your merged branch. You need to resolve the conflict between the branches.
# subscribe to the corresponding queue | ||
SUB_QUEUE = os.environ.get("SUB_QUEUE") | ||
|
||
MQ_SRVC = os.environ.get("MQ_SRVC") |
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.
It looks like MQ_HOST
was renamed to MQ_SRVC
, but not with good reason. Everywhere else (environment files, tests, CI setup) we use MQ_HOST
.
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.
MQ_SRVC has the Docker service name value and MQ_HOST has the IP address. I suspect the test error is caused because adding authentication is not tested, and MQ_HOST is not used anymore.
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.
Please continue to use MQ_HOST
, and please make just the necessary changes to make password authentication work.
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.
No problem! So, password authentication is currently working fine. I'll go ahead and bring back MQ_HOST, and if the test still doesn't go as planned, I'm happy to keep working on it. But I'd really appreciate your help in figuring out what's causing the issue and finding a solution to make the deployment happen. Thanks so much!
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.
I'm looking into the test failures.
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.
I think the problem is that not all environment variables are set when running the tests: especially MQ_SRVC
(which should be MQ_HOST
) in this case. It will also fail if MQ_USER
or MQ_PASS
is not set.
It would be nice to have some checks to see that all configuration items are set, but that is a separate issue (see #27) from this. We should definitely use some defaults in this PR -- like using RabbitMQ default username/password of guest
/guest
when no username and password environment variable is set, as I've stated in another review comment.
So: it looks like addressing the test failures should be straightforward: rename MQ_SRVC
back to MQ_HOST
, and use defaults for MQ_USER
and MQ_PASS
. PRs are tested against an RabbitMQ service that uses the default username and password.
In the meanwhile, could you set up a local development environment and try running the tests yourself?
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.
You probably want to reset your PR branch, and cherry-pick the relevant commits to the one relevant file, like so:
$ git switch luisdev
$ git fetch origin
$ git reset --hard origin/main
$ git cherry-pick 9ba4b92 c0d3459
Now, rename MQ_SRVC
back to MQ_HOST
, and use user/auth defaults. Also remove the time.sleep()
calls. We should not add unnecessary latency, unless there is a good reason. And then update this PR with:
$ git push <remote> luisdev --force-with-lease
Obviously replace <remote>
with whatever remote name that you are using.
Pull Request Test Coverage Report for Build 7810077489
💛 - Coveralls |
sleep time and pika authentication added to: sdx_controller/messaging/rpc_queue_consumer.py