Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Add a TTL on build containers for cleanup #819

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jsun-m
Copy link
Contributor

@jsun-m jsun-m commented Dec 31, 2024

Added a key event manager to image service with a imageContainerTTL key to identified expired build containers

@jsun-m jsun-m changed the title Fix: Add a TTL on build containers Fix: Add a TTL on build containers for cleanup Jan 2, 2025
Copy link
Contributor

@luke-lombardi luke-lombardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small comments, otherwise LGTM

case <-done:
return
case <-ticker.C:
b.rdb.Set(ctx, Keys.imageContainerTTL(containerId), "1", time.Duration(imageContainerTtlS)*time.Second).Err()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be called imageBuildContainerTTL? I know it's technically the "image" service, but the prefix on containers is build-

@@ -194,3 +245,15 @@ func convertBuildSteps(buildSteps []*pb.BuildStep) []BuildStep {
}
return steps
}

var (
imageContainerTTL string = "image:container_ttl:%s"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe call this image:build_container_ttl and var imageBuildContainerTTL

}

const containerKeepAliveIntervalS int = 30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Var names could be improved here. Maybe buildContainerKeepAliveIntervalS
and imageBuildContainerTtlS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants