ioSession: Simplify token management #108
Conversation
jcvenegas
force-pushed
the
simplify-token-management
branch
from
16e8c2f to
6f65d45
Compare
tokeninfo struct is used to manage tokenStateAllocated state. We can remove move that field to ioSession and remove this structure. Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>
Token can be accessed by client.session.token Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>
tokenToSession map is used to easily find a session associated with a token. The a session reference was keep in muitples places: - proxy.tokenToSession[token] - proxy.vms[container-id].tokenToSession[token] - proxy.vms[container-id].ioSessions[sequence-number] Removing tokenToSession the references are keep only in vm struct. Also, disconnectShim was removing sessions from vm struct but not from this map leading to never free tokens added to tokenToSession. Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>
When unregisterVM is called we want to free all the tokens we register previously, all the tokens should not be valid anymore. Fixes: clearcontainers#66 Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>
jcvenegas
force-pushed
the
simplify-token-management
branch
from
6f65d45 to
f661105
Compare
1 similar comment
|
I really need to spend some time on this. Please don't merge before I review it. |
|
Popular Images qa-passed 👍 |
|
@jcvenegas what is the status of this PR, still WIP ? |
|
Popular Images qa-passed 👍 |
|
@sboeuf I remember I've seen some failing tests in semaphore related with The issues is marked as |
|
Ok I'll review it |
proxy.go
Outdated
| @@ -70,8 +56,8 @@ type proxy struct { | |||
| // vms are hashed by their containerID | |||
| vms map[string]*vm | |||
|
|
|||
| // tokenToVM maps I/O token to their per-token info | |||
| tokenToVM map[Token]*tokenInfo | |||
| // tokenToSession maps I/O token to their per-token info | |||
There was a problem hiding this comment.
The comment is now incorrect.
| @@ -103,6 +103,8 @@ type ioSession struct { | |||
| // started. This is used to stop the shim from sending stdin data to a | |||
| // non-existent process. | |||
| processStarted chan interface{} | |||
|
|
|||
| state tokenState | |||
There was a problem hiding this comment.
It would be good to put a comment on this, for consistency with the other fields.
| @@ -158,34 +140,38 @@ func (proxy *proxy) allocateTokens(vm *vm, numIOStreams int) (*api.IOResponse, e | |||
| }, nil | |||
| } | |||
|
|
|||
| func (proxy *proxy) claimToken(token Token) (*tokenInfo, error) { | |||
| func (proxy *proxy) claimToken(token Token) (*ioSession, error) { | |||
There was a problem hiding this comment.
Although it might not strictly be part of this PR, should this function check for token == nilToken as releaseToken() does?
jcvenegas
force-pushed
the
simplify-token-management
branch
from
c96be34 to
f661105
Compare
|
Popular Images qa-passed 👍 |
|
@jcvenegas Okay I am trying to review this PR but I would really appreciate more comments and bigger commit messages to explain the reason why we can do those simplifications. I am not saying this PR is wrong but I want to make sure we're not missing something here. If we can simplify those structures, we should be able to explain it properly. |
|
And apparently something is wrong because Semaphore and Jenkins are consistently failing on the same CRI-O test (15). |
|
@jcvenegas What is the status on this PR? Can you address @sboeuf's comment for adding verbose commit messages explaining the reasoning for the simplifications. It will help review the PR. |
|
kubernetes qa-passed 👍 |
|
@jcvenegas any update/progress on this one ? |
|
kubernetes qa-failed 👎 |
|
@jcvenegas I think we should close this PR. What do you think ? |
|
@sboeuf yes, closing PR. |
proxy: unregisterVM free token
proxy: Remove tokenToSession map from proxy.
client: Remove token from client struct
token: remove tokeninfo struct
Fixes: #66