Skip to content

Reuse Channels#10332

Merged
thatch45 merged 1 commit into
saltstack:developfrom
jacksontj:channelreuse
Mar 13, 2014
Merged

Reuse Channels#10332
thatch45 merged 1 commit into
saltstack:developfrom
jacksontj:channelreuse

Conversation

@jacksontj
Copy link
Copy Markdown
Contributor

with support for multiple ttypes

Submitting merge request to get jenkins run. Last time this was merged there were some problems which seem to be related to this but i'm unable to reproduce them locally-- so hopefully we can get something from Jenkins :)

@jacksontj jacksontj mentioned this pull request Feb 11, 2014
@ghost
Copy link
Copy Markdown

ghost commented Feb 11, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1334/

@s0undt3ch
Copy link
Copy Markdown
Contributor

Closing and re-opening to trigger another jenkins build, we need it to download the remote logs.

@s0undt3ch s0undt3ch closed this Feb 11, 2014
@s0undt3ch s0undt3ch reopened this Feb 11, 2014
@ghost
Copy link
Copy Markdown

ghost commented Feb 11, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1342/

@jacksontj
Copy link
Copy Markdown
Contributor Author

Left it running overnight on my dev box here and still running with no exceptions, anything interesting from the logs?

@s0undt3ch
Copy link
Copy Markdown
Contributor

ok, we need your versions report, salt --versions-report

@jacksontj
Copy link
Copy Markdown
Contributor Author

Hopefully this is helpful :/ if it helps at all it also works on my fedora tower at home. Below is the output from my work machine (rhel 6)

[root@thjackso-ld thjackso]# salt --versions-report
           Salt: 2014.1.0-1406-g3d5ade5
         Python: 2.6.6 (r266:84292, Apr 11 2011, 15:50:32)
         Jinja2: 2.7.2
       M2Crypto: 0.20.2
 msgpack-python: 0.3.0
   msgpack-pure: Not Installed
       pycrypto: 2.6
         PyYAML: 3.10
          PyZMQ: 13.0.2
            ZMQ: 3.2.2

Comment thread salt/transport/__init__.py Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No

@s0undt3ch
Copy link
Copy Markdown
Contributor

Here's what I'm getting https://gist.github.com/s0undt3ch/456a8a0b0c9154e29d1e

@s0undt3ch
Copy link
Copy Markdown
Contributor

vampas @ arched in ~/projects/salt/salt exited 1 on git:(v2014.1-1231-g116fc99) workon Py27
$ salt --versions-report
           Salt: 2014.1.0-1231-g116fc99
         Python: 2.7.6 (default, Nov 26 2013, 12:52:49)
         Jinja2: 2.7.1
       M2Crypto: 0.21.1
 msgpack-python: 0.4.0
   msgpack-pure: Not Installed
       pycrypto: 2.6.1
         PyYAML: 3.10
          PyZMQ: 14.0.1
            ZMQ: 4.0.3

@s0undt3ch
Copy link
Copy Markdown
Contributor

If you have a chance to get docker running on your system, just pass something like --docked=ubuntu-12.04 to the runtests.py script to see it happening...

@jacksontj
Copy link
Copy Markdown
Contributor Author

Ok, i have it failing on some local tests now-- i think i may even have it fixed! Just running a few more tests locally.

@s0undt3ch
Copy link
Copy Markdown
Contributor

Awesome!

@jacksontj
Copy link
Copy Markdown
Contributor Author

In the unittest framework it creates the master and a few minions on the same pid, so my cache key wasn't "unique". It looks like most of the tests pass now-- going to get a merge build going to see :)

@ghost
Copy link
Copy Markdown

ghost commented Feb 17, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1550/

@ghost
Copy link
Copy Markdown

ghost commented Feb 17, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1551/

@jacksontj
Copy link
Copy Markdown
Contributor Author

K, just pushed update for pep8 test-- but the rest of the tests passed so i expect this next run to pass :)

@ghost
Copy link
Copy Markdown

ghost commented Feb 17, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1552/

@ghost
Copy link
Copy Markdown

ghost commented Feb 17, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1554/

@ghost
Copy link
Copy Markdown

ghost commented Feb 17, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1556/

@jacksontj
Copy link
Copy Markdown
Contributor Author

The tests seem to have failed again? (they are grey not red or green...) if so, any details on why its failing?

@s0undt3ch
Copy link
Copy Markdown
Contributor

Gray is bad, and if there are no logs, the remote minion is not working, kind of similar to the previous problem?

@jacksontj
Copy link
Copy Markdown
Contributor Author

Hmm, I'll have to mess with it some more. This problem is actually harder
in the unit tests because they are threaded.
On Feb 18, 2014 1:12 AM, "Pedro Algarvio" notifications@github.com wrote:

Gray is bad, and if there are no logs, the remote minion is not working,
kind of similar to the previous problem?

Reply to this email directly or view it on GitHubhttps://github.com//pull/10332#issuecomment-35365104
.

@thatch45
Copy link
Copy Markdown
Contributor

Thanks @jacksontj, If we can get this working it will really speed things up!

@ghost
Copy link
Copy Markdown

ghost commented Feb 21, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1675/

@ghost
Copy link
Copy Markdown

ghost commented Mar 1, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1925/

@ghost
Copy link
Copy Markdown

ghost commented Mar 1, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1926/

@ghost
Copy link
Copy Markdown

ghost commented Mar 1, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/1927/

@thatch45
Copy link
Copy Markdown
Contributor

thatch45 commented Mar 6, 2014

Thanks. We can merge this once we clean up the test suite and see how it is!

@jacksontj
Copy link
Copy Markdown
Contributor Author

Yea, i'm just waiting on a clean develop to rebase onto so i can get this test suite to run :/

@ghost
Copy link
Copy Markdown

ghost commented Mar 7, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2113/

@jacksontj
Copy link
Copy Markdown
Contributor Author

Unless I'm misreading it looks like the tests passed with an error about
docs?
On Mar 6, 2014 5:23 PM, "SaltStack Jenkins" notifications@github.com
wrote:

Test FAILed.
Refer to this link for build results:
http://jenkins.saltstack.com/job/salt-pr-build/2113/

Reply to this email directly or view it on GitHubhttps://github.com//pull/10332#issuecomment-36958704
.

@s0undt3ch
Copy link
Copy Markdown
Contributor

Nope, the build aborted.

If you rebase to current develop, you should get a better notion of what's going on since current develop seems to be building properly on PR's.

with support for multiple ttypes

Submitting merge request to get jenkins run. Last time this was merged there were some problems which seem to be related to this but i'm unable to reproduce them locally-- so hopefully we can get something from Jenkins :)
@ghost
Copy link
Copy Markdown

ghost commented Mar 11, 2014

Test FAILed.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/2278/

@s0undt3ch
Copy link
Copy Markdown
Contributor

Ah Ha!

The tests passed and the only issue Jenkins is complaining is a lint error which is unrelated to your PR.

@thatch45 this PR seems ready for another review. @jacksontj agreed?

@jacksontj
Copy link
Copy Markdown
Contributor Author

@s0undt3ch This looks good to go! 👍
@thatch45 review away :)

thatch45 added a commit that referenced this pull request Mar 13, 2014
@thatch45 thatch45 merged commit fa72900 into saltstack:develop Mar 13, 2014
@thatch45
Copy link
Copy Markdown
Contributor

Now to see if all of the platforms stay blue! (well, the ones that are blue...)

@thatch45
Copy link
Copy Markdown
Contributor

The tests are all timing out...

@jacksontj
Copy link
Copy Markdown
Contributor Author

Weird.... How did it pass before?
On Mar 12, 2014 9:38 PM, "Thomas S Hatch" notifications@github.com wrote:

The tests are all timing out...

Reply to this email directly or view it on GitHubhttps://github.com//pull/10332#issuecomment-37499699
.

@thatch45
Copy link
Copy Markdown
Contributor

Thats what I am wondering, I am going to revert it so we can get the tests going again

@thatch45
Copy link
Copy Markdown
Contributor

Yep, they are coming back after the revert, sorry @jacksontj ...

@jacksontj
Copy link
Copy Markdown
Contributor Author

Any logs or ideas why it was failing? Hard for me to debug when all tests
pass and it works on my tower.. seems like we've found a gap in the test suite :)
On Mar 12, 2014 11:42 PM, "Thomas S Hatch" notifications@github.com wrote:

Yep, they are coming back after the revert, sorry @jacksontjhttps://github.com/jacksontj...

Reply to this email directly or view it on GitHubhttps://github.com//pull/10332#issuecomment-37504376
.

@jacksontj
Copy link
Copy Markdown
Contributor Author

@thatch45 did we ever figure out why this was failing?

@jacksontj
Copy link
Copy Markdown
Contributor Author

@thatch45 ping :)

@jacksontj
Copy link
Copy Markdown
Contributor Author

@thatch45 ping again, this will make a big difference in our load/latency numbers. I just need to know why this failed so I can fix it

@thatch45
Copy link
Copy Markdown
Contributor

Sorry I missed you here. We were running into strange conditions. Maybe we should make it a configuration option for now?

@jacksontj
Copy link
Copy Markdown
Contributor Author

Sure I can set that up, when would we be able to change the default option
on the master side (part thats backwards incompatible)? Last time we did
this there was (or at least i was told) no mechanism to get the CI
machinery to work with that.

On Tue, May 27, 2014 at 2:26 PM, Thomas S Hatch notifications@github.comwrote:

Sorry I missed you here. We were running into strange conditions. Maybe we
should make it a configuration option for now?


Reply to this email directly or view it on GitHubhttps://github.com//pull/10332#issuecomment-44337692
.

@thatch45
Copy link
Copy Markdown
Contributor

I think we should make it configurable and then change the default for the Lithium release right after we cut the RC for Helium, so we have a good long time to sort out the hicups

@jacksontj
Copy link
Copy Markdown
Contributor Author

Sounds good, I'll put together a pull req this week.
On May 27, 2014 4:27 PM, "Thomas S Hatch" notifications@github.com wrote:

I think we should make it configurable and then change the default for the
Lithium release right after we cut the RC for Helium, so we have a good
long time to sort out the hicups


Reply to this email directly or view it on GitHubhttps://github.com//pull/10332#issuecomment-44348302
.

This was referenced Jun 1, 2014
jacksontj added a commit to jacksontj/salt that referenced this pull request Jun 11, 2014
…annel object.

This is an improvement over saltstack#10332 in that you don't have to worry about someone creating a ZeroMQChannel then forking (thereby breaking the thread-pid rules). This way once it moves it will create a new sreq as needed.

This is possible now that saltstack#13155 is merged
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