session: track sessions with a group construct by tonistiigi · Pull Request #1551 · moby/buildkit

tonistiigi · 2020-06-30T22:13:41Z

Avoid hidden session passing and allow one session to drop when
multiple builds share a vertex.

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

tonistiigi · 2020-07-01T16:26:47Z

@hinshun PTAL . This fixes the issue described in #1432 (comment) . I didn't make changes to the way the session interacts with frontends atm. I think more changes are needed there to really prepare for better shared session support but this should get us into a better state for these changes.

hinshun

So to be clear, currently when multiple sessions share a vertex, if one session drops the solve is cancelled for everyone?

I looked closely at the changes, and it LGTM. I can see how various functions that depend on sessions now can choose Any of the session in the session.Group.

I didn't make changes to the way the session interacts with frontends atm.

I didn't see any cases where multiple sessions are added to a group, but I think that's what you alluded to in the PR comment above.

hinshun · 2020-07-01T18:42:25Z

cache/remotecache/local/local.go

-func getContentStore(ctx context.Context, sm *session.Manager, storeID string) (content.Store, error) {
-	sessionID := session.FromContext(ctx)
+func getContentStore(ctx context.Context, sm *session.Manager, g session.Group, storeID string) (content.Store, error) {
+	// TODO: to ensure correct session is detected, new api for finding if storeID is supported is needed


What does it mean by storeID is supported?

When there are multiple sessions daemon could send "detect" request to know which one supports the current storeID. If a specific session does not know about storeID then the next one is tried.

exporter/exporter.go

solver/llbsolver/ops/exec.go

util/resolver/resolver.go

hinshun · 2020-07-01T21:56:37Z

session/group.go

+	NextSession() string
+}
+
+func NewGroup(ids ...string) Group {


I don't see this being called with more than one session ID in this PR. How will multiple sessions come together in a group in the future?

Vertexes use their own implementation of Group defined in jobs.go. This is the group passed to ops/subbuild etc.

hinshun · 2020-07-01T22:10:02Z

solver/llbsolver/ops/exec.go

 	}
-
+	// because ssh socket remains active, to actually handle session disconnecting ssh error
+	// should restart the whole exec with new session


Can you expand on this? I don't understand.

If we have 2 builds both running the same exec() with ssh mounted the ssh remains active for the whole duration of the exec. So if one session goes away there is no way to switch this ssh socket to another session as it might be in unknown state. Atm we ignore this and only validate session works when exec starts. But if we would want to handle this case then when connection would drop from ssh, we could just restart the whole exec with the new session. If you look at the "local source" implementation now, then this is what I do there. If a transfer fails it will check if we can attempt a new transfer from another session before failing the build.

tonistiigi · 2020-07-02T00:12:19Z

So to be clear, currently when multiple sessions share a vertex, if one session drops the solve is cancelled for everyone?

If a session drops while it is being used and it was the session that was randomly chosen for the op then the whole op fails. The race window is actually quite small, even my reproducer isn't very effective.

A somewhat more interesting case is also what happens to the puller. We reuse the resolver to avoid making new registry connections but this means CacheKey and Exec may have a different active session for getting the credentials. So there is a need to "update" the resolver with the new authentication backend.

I looked closely at the changes, and it LGTM. I can see how various functions that depend on sessions now can choose Any of the session in the session.Group.

Yes, and Any automatically retries the next session, should the previous one fail.

I didn't see any cases where multiple sessions are added to a group, but I think that's what you alluded to in the PR comment above.

Multiple session are added to the group automatically with the jobs mechanism. This existed before. The difference is that when passing to ops, previously solver would pick a random one and pass it as string, and now it passes a callback (masked in Group interface) for op to get access to all the valid sessions when needed.

tonistiigi · 2020-07-02T00:13:38Z

Found an issue with ResolveImageConfig and Resolver cache that needs some refactoring.

Avoid hidden session passing and allow one session to drop when multiple builds share a vertex. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

hinshun

One question, otherwise looks good.

hinshun · 2020-07-05T18:53:29Z

util/resolver/resolver.go

+func (a *SessionAuthenticator) credentials(h string) (string, string, error) {
+	a.cacheMu.RLock()
+	c, ok := a.cache[h]
+	if ok && time.Since(c.created) < time.Minute {


Where does time.Minute come from? Is that the expiration time of creds?

Yes. I guess I could add a named constant for it so it is clearer.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi · 2020-07-08T21:43:09Z

@hinshun good to merge?

full diff: moby/buildkit@df35e98...4d1f260 - moby/buildkit#1551 session: track sessions with a group construct - moby/buildkit#1534 secrets: allow providing secrets with env - moby/buildkit#1533 git: support for token authentication - moby/buildkit#1549 progressui: fix logs time formatting Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: moby/buildkit@df35e98...4d1f260 - moby/buildkit#1551 session: track sessions with a group construct - moby/buildkit#1534 secrets: allow providing secrets with env - moby/buildkit#1533 git: support for token authentication - moby/buildkit#1549 progressui: fix logs time formatting Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 7edc00d8088795798ae4e82d2e529a9829acfe72 Component: cli

tonistiigi force-pushed the session-group branch from f1ceb4a to 5a2c799 Compare July 1, 2020 05:19

tonistiigi marked this pull request as ready for review July 1, 2020 05:19

tonistiigi force-pushed the session-group branch 3 times, most recently from ad9bb6e to 6acceaa Compare July 1, 2020 06:55

tonistiigi requested a review from hinshun July 1, 2020 16:26

hinshun approved these changes Jul 1, 2020

View reviewed changes

tonistiigi added the status/do-not-merge label Jul 2, 2020

tonistiigi added 3 commits July 5, 2020 00:15

session: track sessions with a group construct

2e9987a

Avoid hidden session passing and allow one session to drop when multiple builds share a vertex. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

pull: allow separate sessions for different parts of pull

214aa5d

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

pull: fix session updating on resolver

05013a6

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi force-pushed the session-group branch from 6acceaa to ed60a2f Compare July 5, 2020 07:15

tonistiigi removed the status/do-not-merge label Jul 5, 2020

tonistiigi requested a review from hinshun July 5, 2020 07:20

hinshun approved these changes Jul 5, 2020

View reviewed changes

resolver: add credentials cache

a8c2137

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>

tonistiigi force-pushed the session-group branch from ed60a2f to a8c2137 Compare July 5, 2020 19:07

hinshun merged commit 4881300 into moby:master Jul 8, 2020

tonistiigi mentioned this pull request Jul 9, 2020

Add support for lazily-pulled blobs in cache manager. #1475

Merged

tonistiigi mentioned this pull request Jul 23, 2020

vendor: update buildkit to 4d1f260e8 moby/moby#41234

Merged

thaJeztah mentioned this pull request Jul 28, 2020

vendor: buildkit 4d1f260e8490ec438ab66e08bb105577aca0ce06 docker/cli#2656

Merged

alefray mentioned this pull request Feb 3, 2021

port --secret and --ssh support genuinetools/img#323

Merged

Conversation

tonistiigi commented Jun 30, 2020

Uh oh!

tonistiigi commented Jul 1, 2020

Uh oh!

hinshun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Jul 2, 2020

Uh oh!

tonistiigi commented Jul 2, 2020

Uh oh!

hinshun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Jul 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants