search

SUSE / DeepSea

A collection of Salt files for deploying, managing and automating Ceph.
GNU General Public License v3.0
161 stars 75 forks source link

[SES6] Prevent IOLoop error by calling Salt modules correctly #1740

Closed swiftgist closed 5 years ago

swiftgist commented 5 years ago

Signed-off-by: Eric Jackson ejackson@suse.com bnc#1123257

Checklist:

swiftgist commented 5 years ago

The originally call creates a Salt client inside the module. Apparently, newer Salt versions catch this and issue an error. This happened on OpenStack since the kernel is kernel-default-base which is missing some modules for ceph.

swiftgist commented 5 years ago

@susebot run teuthology

mgfritch commented 5 years ago

@swiftgist Do you intend to apply this change against the master branch?

mgfritch commented 5 years ago

@smithfarm @kshtsk any reason there is no lint test or teuthology?

swiftgist commented 5 years ago

@swiftgist Do you intend to apply this change against the master branch?

I don't know. With our work on containers and SES6 in its own maintenance branch, I suspect the next branch will become master.

swiftgist commented 5 years ago

@smithfarm @kshtsk any reason there is no lint test or teuthology?

I suspect Joshua either didn't ask for it or may have asked Nathan and Kyr to hold off. The lint configuration seems to be up for discussion. With respect to teuthology, I expect some significant changes to support containers.

mgfritch commented 5 years ago

@swiftgist Do you intend to apply this change against the master branch?

I don't know. With our work on containers and SES6 in its own maintenance branch, I suspect the next branch will become master.

hmm, but until next is final and merged back .. shouldn't we still commit fixes to master and then backport from there?

mgfritch commented 5 years ago

@smithfarm @kshtsk any reason there is no lint test or teuthology?

I suspect Joshua either didn't ask for it or may have asked Nathan and Kyr to hold off. The lint configuration seems to be up for discussion. With respect to teuthology, I expect some significant changes to support containers.

But would these changes to support containers break the existing tests for older releases (e.g. SES5 and SES6) ? It would seem like we need those test still working to support maint. releases ..

jschmid1 commented 5 years ago

@smithfarm @kshtsk any reason there is no lint test or teuthology?

This is indeed weird. Could the referenced people have a look, please?

jschmid1 commented 5 years ago

@swiftgist Do you intend to apply this change against the master branch?

I don't know. With our work on containers and SES6 in its own maintenance branch, I suspect the next branch will become master.

hmm, but until next is final and merged back .. shouldn't we still commit fixes to master and then backport from there?

That's also what I thought. Since next is still not 100% guaranteed to succeed, we still need a fallback solution which would indeed be master.

One could argue that we could also continue to merge directly into SES6 to eventually use the SES6 branch as our fallback solution. The major advantage is that we wouldn't deviate master from next all too much, hence have a easier time merging next->master. This would however break our scheme of keeping master up to date and backporting to our release branches.

smithfarm commented 5 years ago

@jschmid SES6 is a new branch, heretofore without any CI. Just as there would be no CI for a branch named "foo". (Maybe a Jira ticket in CI/CD would be warranted?)

kshtsk commented 5 years ago

retest this please

jschmid1 commented 5 years ago

@swiftgist please fix linter complaints:

11:56:58 ************* Module srv.salt._modules.kernel
11:56:58 W: 35, 4: Unused import salt.client (unused-import)
jschmid1 commented 5 years ago

This'd need a forwardport aswell @swiftgist