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

Transport Refactor #60867

Closed
wants to merge 55 commits into from
Closed

Transport Refactor #60867

wants to merge 55 commits into from

Conversation

dwoz
Copy link
Contributor

@dwoz dwoz commented Sep 12, 2021

Requires #60852

What does this PR do?

What issues does this PR fix or reference?

Fixes:

Previous Behavior

Remove this section if not relevant

New Behavior

Remove this section if not relevant

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@dwoz dwoz requested a review from a team as a code owner September 12, 2021 01:46
@dwoz dwoz requested review from joechainz and removed request for a team September 12, 2021 01:46
@dwoz dwoz changed the title Transport refactor [WIP] Transport Refactor Sep 12, 2021
@dwoz dwoz marked this pull request as draft September 12, 2021 01:47
salt/transport/client.py Outdated Show resolved Hide resolved
@dwoz dwoz force-pushed the wip_transport_refactor branch 4 times, most recently from 9742328 to d9023c6 Compare September 20, 2021 02:12
@dwoz dwoz force-pushed the wip_transport_refactor branch 4 times, most recently from 057adfb to 4c873ea Compare September 26, 2021 07:19
@dwoz
Copy link
Contributor Author

dwoz commented Sep 26, 2021

re-run all

salt/channel/client.py Outdated Show resolved Hide resolved
salt/channel/client.py Outdated Show resolved Hide resolved
salt/channel/client.py Outdated Show resolved Hide resolved
salt/channel/server.py Outdated Show resolved Hide resolved
salt/transport/tcp.py Show resolved Hide resolved
salt/channel/server.py Outdated Show resolved Hide resolved
salt/transport/tcp.py Outdated Show resolved Hide resolved
salt/channel/server.py Outdated Show resolved Hide resolved
salt/channel/client.py Show resolved Hide resolved
@dwoz dwoz force-pushed the wip_transport_refactor branch 5 times, most recently from c737472 to 5ec9f21 Compare October 4, 2021 20:37
Copy link
Contributor

@devkits devkits left a comment

Choose a reason for hiding this comment

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

I am updating the Rabbit POC to match the latest code in this branch.
Wondering whether salt/salt/cli/caller.py needs to be updated in this branch as well so that a new caller can be implemented with a new transport.

@dwoz dwoz requested a review from s0undt3ch October 5, 2021 21:29
salt/transport/base.py Outdated Show resolved Hide resolved
salt/transport/base.py Outdated Show resolved Hide resolved
@devkits
Copy link
Contributor

devkits commented Oct 6, 2021

A few general questions/comments:

  1. self.event.fire_event() seems to use the hard-coded salt.transport.ipc.IPCMessageClient which, in turn, uses local IPC primitives. Do we expect this to work side-by-side with broker-based event bus where there may not be any sockets or streams?

  2. FYI: following the text pattern search [ "zeromq", "tcp" ], a few additional files that I had to modify to fit in rabbitmq:

  • salt/moduls/mine.py
  • salt/utils/minions.py
  • salt/utils/minion.py
  • salt/modules/publish.py
  • salt/output/key.py
  • salt/cli/caller.py
  • noxfile.py

@damon-atkins
Copy link
Contributor

Will this change require the master and minion to be upgraded at the same time?
If so consider allowing the minion to discover what protocol the Master is using. Like SSH and TLS which both have a little bit of communication in the clear first.

read A < /dev/tcp/localhost/22 ; echo $A
SSH-2.0-OpenSSH_9.7

@damon-atkins
Copy link
Contributor

@devkits is the RabbitMQ code, allow for the RabbitMQ to run on a different server to the master?

@saltstack saltstack deleted a comment from github-actions bot Dec 13, 2021
@dwoz
Copy link
Contributor Author

dwoz commented Dec 15, 2021

re-run full all

@dwoz dwoz requested review from waynew and Ch3LL December 15, 2021 09:47
Ch3LL
Ch3LL previously approved these changes Dec 17, 2021
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

What a job! Nice work on this refactor!

A couple of questions/typos in docstrings, but I think there's a typo in thin.py that's a proper bug.

doc/topics/channels/index.rst Show resolved Hide resolved
salt/transport/base.py Show resolved Hide resolved
salt/transport/base.py Show resolved Hide resolved
salt/transport/tcp.py Show resolved Hide resolved
salt/transport/tcp.py Show resolved Hide resolved
tests/unit/test_pillar.py Show resolved Hide resolved
salt/utils/thin.py Show resolved Hide resolved
salt/transport/zeromq.py Show resolved Hide resolved
salt/transport/zeromq.py Show resolved Hide resolved
@dwoz dwoz mentioned this pull request Dec 22, 2021
3 tasks
@dwoz dwoz requested a review from waynew January 6, 2022 20:31
@dwoz dwoz force-pushed the wip_transport_refactor branch 2 times, most recently from 809acac to bfa9651 Compare January 9, 2022 21:18
@dwoz
Copy link
Contributor Author

dwoz commented Jan 9, 2022

re-run full all

@github-actions
Copy link

Hi! I'm your friendly PR bot!

You might be wondering what I'm doing commenting here on your PR.

Yes, as a matter of fact, I am...

I'm just here to help us improve the documentation. I can't respond to
questions or anything, but what I can do, I do well!

Okay... so what do you do?

I detect modules that are missing docstrings or "CLI Example" on existing docstrings!
When I was created we had a lot of these. The documentation for these
modules need some love and attention to make Salt better for our users.

So what does that have to do with my PR?

I noticed that in this PR there are some files changed that have some of these
issues. So I'm leaving this comment to let you know your options.

Okay, what are they?

Well, my favorite, is that since you were making changes here I'm hoping that
you would be the most familiar with this module and be able to add some other
examples or fix any of the reported issues.

If I can, then what?

Well, you can either add them to this PR or add them to another PR. Either way is fine!

Well... what if I can't, or don't want to?

That's also fine! We appreciate all contributions to the Salt Project. If you
can't add those other examples, either because you're too busy, or unfamiliar,
or you just aren't interested, we still appreciate the contributions that
you've made already.

Whatever approach you decide to take, just drop a comment here letting us know!

Detected Issues (click me)
Check Known Missing Docstrings...........................................Failed
- hook id: invoke
- duration: 1.69s
- exit code: 1

The function 'recv' on 'salt/modules/cp.py' does not have a 'CLI Example:' in it's docstring
The function 'recv_chunked' on 'salt/modules/cp.py' does not have a 'CLI Example:' in it's docstring
The function 'envs' on 'salt/modules/cp.py' does not have a 'CLI Example:' in it's docstring
Found 3 errors


Thanks again!

@dwoz
Copy link
Contributor Author

dwoz commented Jan 11, 2022

re-run full all

@dwoz dwoz mentioned this pull request Jan 12, 2022
3 tasks
@dwoz
Copy link
Contributor Author

dwoz commented Jan 12, 2022

Replaced by #61450

@dwoz dwoz closed this Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Phosphorus v3005.0 Release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants