Few improvements here: https://pagure.io/fork/tkopecek/koji/c/59813a118ce17cbb64e005b65de824f529f4065a?branch=pr3543

What doesn't work nice now is that if (admin) forcefully logout some user it will just reconnect without even knowing that something happened. Not sure if adding some ClientSession option (relogin=True) is worth adding as probably everyone will turn it on. @mikem ?

Note, that we have to add also absolute timeout (probably separate PR)

The subsession question is an interesting one, and it is going to make this more complicated.

Subsessions are serve a couple different purposes:

1. Keeping parallel access sane — Koji sessions presume sequential calls due the the callnum value, so the best way to make parallel authenticated calls is to use a subession

2. Giving the main process control over access for its children — The main session can expire its subsessions. When a task is canceled, the main kojid process cancels its subsession, which ensures that the task process can no longer do anything of importance, even if killing it somehow fails.

3. Linking related sessions — When the main session is logged out, so are its subsessions

It's worth noting that subsessions can be quite long-lived. Their original purpose was for builder tasks, which can run from a few seconds to many hours. However, we now also use them for threads in kojira, which live as long as the kojira process itself does.

Because they are long-lived, we do need a mechanism to renew subsessions, but that mechanism needs to respect the structure here. In particular,

• the session needs to remain connected to its master session
• the child task should not be able to renew its session when the main kojid process deliberately logs it out

I'm not sure we're going to be able to to do all this without some api changes and possibly some schema changes.

A few quick remarks on the current patch. Granted these may become irrelevant since we likely need more significant work anyway, but...

• the code block that handles renewal should probably be separated into a function. That's a lot of complex code to be so deeply nested
• the session should probably remember how it authenticated the current session so that it can use the same auth again, rather than going by session options

At the moment, this does not fix #3394 because it does not actually implement a timeout. Currently, it only implements a session renewal mechanism. If we're intending to cover #3394 in multiple PRs, we should change that link to Related.

I don't like having the session_key in the args to all these login calls (and therefore also in the auth_method value).

I think we could pass a boolean renew flag instead. The auth.py code can read the session key from the headers as it already does.

I'm concerned about the recursion we're adding in _callMethod. Previously there was no way for this call to recurse. Now there are two pathways:

1. _callMethod -> _renew_session -> $auth_method -> _callMethod
2. _callMethod -> _callMethod (after the renew)

_callMethod modifies self._retries in a way that this breaks, though self._retries is only used one place.

This is a relatively minor problem, but after looking at it I think the retry loop just isn't the right place to handle renewal. Instead, we should handle it a layer up. This would either mean moving the logic from _callMethod to a new function or perhaps using a decorator.

Tomas pointed this out above and it's a very good point.

What doesn't work nice now is that if (admin) forcefully logout some user it will just reconnect without even knowing that something happened.

We need a way to distinguish these cases. This will likely require a schema change. Perhaps a closed field for sessions. Alternately, we could enforce timeouts without changing the expired field just based on timestamp check, but this might be confusing.

On top of that, there is still the subsession issue. the current code doesn't allow subsessions to be renewed, which is likely to cause numerous issues.

It would probably be helpful to go ahead and include a session timeout alongside this code. That will give us a good way to test it.

At the moment, this does not fix #3394 because it does not actually implement a timeout. Currently, it only implements a session renewal mechanism. If we're intending to cover #3394 in multiple PRs, we should change that link to Related.

I renamed issue. This issue works is working without renewal timeout option.

Currently case:
We have cron job, which deletes all items from sessions, which are older than absolute or age value. So we can renew all expirated sessions, which is in the session table currently (not older than 1 day when age is used).

New issue for renewal timeout option: 3596

I don't like having the session_key in the args to all these login calls (and therefore also in the auth_method value).

I think we could pass a boolean renew flag instead. The auth.py code can read the session key from the headers as it already does.

Hm, I tried it, but when we need to you session_key from header, it is cleaned by setSession. Or did you mean other solution how we can use session_key?

I'm concerned about the recursion we're adding in _callMethod. Previously there was no way for this call to recurse. Now there are two pathways:

1. _callMethod -> _renew_session -> $auth_method -> _callMethod
2. _callMethod -> _callMethod (after the renew)

_callMethod modifies self._retries in a way that this breaks, though self._retries is only used one place.

This is a relatively minor problem, but after looking at it I think the retry loop just isn't the right place to handle renewal. Instead, we should handle it a layer up. This would either mean moving the logic from _callMethod to a new function or perhaps using a decorator.

Fixed by decorator.

Tomas pointed this out above and it's a very good point.

What doesn't work nice now is that if (admin) forcefully logout some user it will just reconnect without even knowing that something happened.

We need a way to distinguish these cases. This will likely require a schema change. Perhaps a closed field for sessions. Alternately, we could enforce timeouts without changing the expired field just based on timestamp check, but this might be confusing.

Fixed, I added closed column to session table and currently it should be working for logged_in and not closed sessions only.

On top of that, there is still the subsession issue. the current code doesn't allow subsessions to be renewed, which is likely to cause numerous issues.

This was solved earlier in this PR. Code allows renew session and subsession.

It would probably be helpful to go ahead and include a session timeout alongside this code. That will give us a good way to test it.

As I wrote more up, session timeout option will be in the other issue 3596

It still does not look to me like subsessions are going to work correctly.

When created, subsessions receive a copy of the auth_method dict from the parent. At present, this includes the parent's session_key. This is a problem -- the subsession should not have have access to the parent key.

The first time a subsession hits an AuthError, it will attempt to re-auth using the auth_method data it received at creation. I.e. its parent's, including its parents session key. So, when this login call is performed, it looks to the hub like the parent is renewing. The check in createSession will pull up the parent session, getting the wrong value for master. The client will receive a new session, but will no longer be a subsession.

Once this first renew succeeds, the login call will reset the auth_method data with the new key. So, for subsequent renewals, the session will be renewing as it is is an independent session. The connection to the master session is lost and we lose subsession behavior.

Hm, I tried it, but when we need to you session_key from header, it is cleaned by setSession. Or did you mean other solution how we can use session_key?

I think when we make a call to renew our session, we shouldn't need to clear the old session first.

Getting the subsessions right is going to be tricky. It's important to maintain the connection between subsessions and the master session. Furthermore these sessions operate independently, so they could be renewed concurrently, which could lead to a race depending on how we implement.

I'm thinking it might be safer to rekey a session rather than replace it entirely. This would preserve the subsession connections without us having to update the links.

I still think that session_key should not be in the login options (nor in the auth_method data).

Minor thing, but

@@ -2874,7 +2960,9 @@ class ClientSession(object):
                             # server correctly reporting an outage
                             tries = 0
                             continue
-                    raise err
+                    else:
+                        raise err
+
                 except (SystemExit, KeyboardInterrupt):

This raise does not need to be nested under this else. It is safer to leave it as an unconditional last step of the except clause. Also the extra blank line seems extraneous since the other clauses in the try statement do not have them.

Another minor thing. This debug output should be dropped

self.logger.error("ssl_login---------------")

Some improvements: https://pagure.io/fork/tkopecek/koji/c/02513cb4199b8d1f1b3504b5b93c5e0b98b9774c?branch=pr3543b Anyway, at least one subsession race condition still exist. If master session is exclusive, renewal of its exclusive status is delayed to another call. Subsession can believe that master is still ok and exclusive (but it was just renewed nad makeExclusive was not called yet. It can be improved with putting these two into multicall but it would be tricky.

I've added two thing into the branch
- check that session is not closed
- more problematic one - merge exclusive status renewal with login into the one call. That's probably not needed as it will lower race condition a bit but not much. There still stays situation that non-expired subsession exist and will reach to some exclusive call before master session renew itself. Options are to ignore it (which seems to work, just cluttering log with traceback) or to always check master's status and if it is expired, treat subsession also as expired. Second option could result in some unexpected situations?

Thanks for the updates! I still need to analyze this a bit, as there are a lot of deep changes here, but here are some initial thoughts.

Simple things:

• new constraint in schema update but not schema
• raise err is still moved under the else:
• test_gssapi still asserts session_key args
• elif name in 'sslLogin':

At present _forget() is a no-op if self.logged_in is false, but now we could be in a state where we still have sinfo. Perhaps best to just drop the logged_in check here, assuming we're going with this overall data design.

I'm a little concerned about possible locking issues calling makeExclusive inside a login call. Looks like a double update.

createSession (and hence the login methods) could not return a None value before. It looks like this with our code, which checks the value already anyway, but it is a nontrivial api change. I wonder it it's actually needed.

Also I'm kind of leaning towards the notion that subsessions should be able to renew themselves even if the master session has not yet. I.e. master session could be expired, but not closed. After all, the client is providing valid credentials for the user.

Also also, when a session goes exclusive, it should probably close those other sessions, not just expire them.

Also I'm kind of leaning towards the notion that subsessions should be able to renew themselves even if the master session has not yet. I.e. master session could be expired, but not closed. After all, the client is providing valid credentials for the user.

So, it means retaining exclusivity for expired master session until nobody else will pick it (dropping the constraint)? Typical case when this matters is daemon thread calling host.* endpoints. createSessions check + return None - if we're going to allow subsession renewal in any case, this could be dropped. In case master session is closed, subsession should be also closed and expiration doesn't matter in that moment.

@mikem ?

Regarding subsessions, I see a few major pathways.

First is the one I've suggested. Let subsessions renew themselves when they expire, even if the master session is still expired (but not closed).

Second is to require the master session to be renewed first. The problem here is what does the subsession do if the master has not renewed yet. There's a bit of a race, and I think we can expect to hit it often. Does the session have a special wait loop for this? How long does it wait? It's possible to go this way, but I think it's going to end up being more complex and fragile.

Beyond that, I only see more esoteric refactoring.

So, it means retaining exclusivity for expired master session until nobody else will pick it (dropping the constraint)?

Yeah, I guess so.

In essence, we're giving session.expired a new meaning (timed out but eligible for renewal). What we used to mean by session.expired is now represented by session.closed. It's unfortunate to juggle the terms like this, but "expired" really does suggest the new meaning more than the old.

So I guess we need to allow expired sessions to keep the exclusive flag. That kind of matches the notion above about the old "expired" mapping to "closed" and the new "expired" being a new thing.

Updated https://pagure.io/fork/tkopecek/koji/commits/pr3543b

I think this is about ready. There are a couple minor things.

The flow in prepCall doesn't seem to match the comments. The if getattr(self, 'sinfo') is not None stanza suggests that it is only for the renewal case, but it happens whenever we have a session, even in the old server case. This is probably ok, but the comment is confusing. Perhaps:

         if getattr(self, 'sinfo') is not None:
-            # session renewal (not logged in, but have session data)
-            # makes sense only for new method/server
+            # send sinfo in headers if we have it
+            # still needed if not logged in for renewal case
             sinfo = self.sinfo.copy()

renew_expired_session should probably be @staticmethod

It seems like it would be more readable to add a self arg to the nested _renew_expired_session so we can use self instead of args[0].

I've mentioned several times in review that the raise err statement should not be nested under an else. It is the default outcome for the Fault handler, not a special case of the if. At present this will result in the code ignoring a ServerOffline fault when offline_retry is False, silently retrying them regardless of setting.

I'm still a little concerned that we have a double update when calling makeExclusive in the login calls, but it looks like we already had double updates there, so 🤷

added updated here: https://pagure.io/fork/tkopecek/koji/c/de4a2aaa64d870212b950fc2355d7d111df2113e?branch=pr3543b

• else is removed in my branch for some time - it slipped from cherry-picks here
• I've added SELECT .. FOR UPDATE for session which should? help with double update.

Ah. There are some odd differences between this PR and your branch, perhaps from a rebase. At this point I'm reviewing the pr3543b branch instead of the current content of this PR.

The pr3543b branch addresses all of me concerns above. However, after seeing the added row lock in place, I note that we're already acquiring a row lock on the user table a bit earlier, making the new row lock redundant.

So I think we can move forward with pr3543b, minus the extra row lock.

Opened new PR #3664 directly from that branch. Added commit dropping rowlock query. Closing this PR.