The etcd returner is using the wrong API due to a refactor of salt.utils.etcd_util.

[arizvisa]

What does this PR do?

The salt.returners.etcd module expects everything returned from the etcd client to be the EtcdResult type which is the native type from the "python-etcd" module. The salt.utils.etcd_util actually hides this interface in order to capture exceptions and convert EtcdResult types into results that make sense to salt's interface.

This PR fixes this issue by modify the _get_conn() function to return the actual "python-etcd" client which is what the original implementation of salt.returners.etcd was developed around.

What issues does this PR fix or reference?

This closes issue #51345.

Previous Behavior

The previous behaviour resulted in the module not working because the EtcdResult properties such as .children and .value were being masked away by try-excepts from the salt.utils.etcd_util module. This resulted in many parts of this returner failing due to the client returning a different type than expected.

New Behavior

Now that this module is using the correct api that returns an EtcdResult, the module works as originally intended.

Commits signed with GPG?

Nope.

[arizvisa]

I gave some hella love to etcd_return with these commits.

[arizvisa]

PR #51346 which adds event_return() has now been merged into this one. It was becoming too tedious to maintain both at the same time.

[arizvisa]

And rebased + force pushed. Let's see how this turns out...

[arizvisa]

Grr...first force-push was re-based against develop instead of origin/develop...

[arizvisa]

Okay. Good to go and ready for a reviewer.

[dwoz]

@arizvisa I think we need to deprecate the ttl option: https://docs.saltstack.com/en/latest/topics/development/deprecations.html

[arizvisa]

@dwoz, sure. What's the process for this? Literally test for the ttl parameter and call warn_until?

[arizvisa]

@dwoz, does it make sense to call warn_utils everytime returner() and save_load() gets called, or is there a better place since it exists only during option parsing?

[arizvisa]

Umm...yeah. Any word on my questions on how to best warn about this?

Does it really make sense to warn everytime a returner sends something back to the master?

[arizvisa]

I'd like to get this PR merged instead of having to maintain it because this is something that you guys claim to support, but the returner doesn't even work at all(?).

[arizvisa]

Actually better yet, I can avoid deprecating ttl entirely by changing its semantics a little bit. It seems that when a returner receives a job where the jid is set to "req", it's the job of the returner to generate a new job id and update the load.

A number of returners do not do this (as it doesn't seem to be documented, since returners are "easy to write"). However, the mysql returner (which seems to be the only one with unit-testing) seems to do this explicitly.

Although the existence of the ttl option is a horrible idea in my opinion as it allows a data structure that Salt uses to change without any means of informing Salt about it (it would be nice if returners were a persistent coroutine that Salt would use so that Salt could receive events instead of having to poll.).

So wrt the ttl, this might be useful for the "req" jid since it's a magic-job-id that can be submitted by any minion (or expicitly by specifying it as a the jid causing a name-clash) and it's up to the returner to convert the magic-job-id into a real one. If for some reason a code-path explicitly calls save_load, we can use the ttl here to allow the temporary job request to be automatically removed since "nothing" should call save_load() without returner() getting ahold of it first.

In summary, save_load will use the default ttl option to expire jobs that use req as a jid since returners have no other means of removing this job type.

[arizvisa]

Or another place to use it would be in the implementation of clean_old_jobs so that way the job id itself can be checked against the ttl before actually removing it. Although this is similar to the way the original author of the etcd returner intended to us the ttl, etcd did not actually work this way as directories would not automatically remove their contents when they expired. Or another situation that occurs is if a key was written to a directory, the key would clear the ttl attribute of the directory which (when applied to saltstack) results in a lot of orphanned or partial jobs existing within the data-store.

I think the best bet is a combination of these two usage-scenarios with ttl so that warn_until isn't complaining about the deprecation everytime a minion/master interfaces with the returner. I was trying to avoid implementing clean_old_jobs as tinkering with saltstack isn't related to my job/career and thus re-implementing these things collides with family time, but it seems that in order for the rest of this returner to make sense this function is necessary.

[arizvisa]

force pushed due to rebase.

[arizvisa]

Ok. I've been using this returner for quite a bit now and I haven't had any problems with it. This also fixes issue #51809 for the etcd returner although I really believe that this 'req' hack really needs to be looked into because it leads to being unable to locate the job id for minion-initiated jobs.

[arizvisa]

(Btw, y'all might want to squash-merge this, unless you want to retain all the dev history that was done on this module.)

[arizvisa]

This windows test is failing due to the signal module not supporting posix signals which seems like a test issue. any word?

[arizvisa]

Hey, is there any word on this? Do you need me to rebase it again?

[arizvisa]

Wrt to your review suggestion to deprecate the ttl. The ttl is actively used in the PR so it wouldn't make sense to get rid of it.

[arizvisa]

Wow. Thanks you!

[arizvisa]

@waynew, do you want me to port this to master for you guys real quick?

[arizvisa]

For the record, PR #55186 ports this PR to master and fixes a logging issue when enabling debug logging. So reference that one if you want to untag this, or clear it out of project 6, "PRs to port to master".