Improve kraken agent logging #411

hweawer · 2025-07-11T11:34:02Z

🔧 Key Changes

Enhanced Agent Server Logging

Improved readiness check error reporting: Error messages now include component identifiers (scheduler:, build-index:, tracker:) for better debugging
Better error message formatting: Standardized error output format across all health check components
Enhanced request logging middleware: Added detailed request context logging for better observability

Code Quality & Testing Improvements

Test robustness: Updated test mocks to use gomock.Any() instead of context.Background() for more flexible testing
Field naming consistency: Fixed struct field naming (e.g., readinessCacheTTL → ReadinessCacheTTL)
Improved error assertions: Updated test expectations to match new error message formats

Robust Docker Image Building Process

Enhanced Dockerfile patterns: Implemented more reliable apt-get commands across all service Dockerfiles
Better package management: Added retry mechanisms and cleanup for package installations

Configuration & Error Handling

Removed puller dependencies: Cleaned up unused puller-related code
Fixed configuration issues: Resolved various configuration parsing and validation errors
Better timeout handling: Improved timeout context management for container operations

gkeesh7 · 2025-07-22T13:23:53Z

agent/agentserver/server.go

+// applyDefaults sets default values for configuration.
+func (c *Config) applyDefaults() {
+	if c.DownloadTimeout == 0 {
+		c.DownloadTimeout = 5 * time.Minute


Please increase this default download timeout to 15 minutes atleast.

gkeesh7

Please address the comments, Let's keep the Changeset size limited to 150 Lines per PR as much as possible.

gkeesh7 · 2025-07-22T14:10:11Z

docker/agent/Dockerfile

@@ -1,6 +1,25 @@
-FROM debian:10
+FROM debian:11


Let us use debian:12 instead ? It is the current stable release https://www.debian.org/releases/

gkeesh7 · 2025-07-22T15:17:19Z

agent/agentserver/server.go

+
+	// Validate tag format
+	if strings.TrimSpace(tag) == "" {
+		return handler.ErrorStatus(http.StatusBadRequest)


why do we need to do this check ?

This check is just a slight optimisation for not triggering build-index for invalid tags

gkeesh7 · 2025-07-23T12:22:39Z

agent/agentserver/server.go

+	logger = logger.With("namespace", namespace, "digest", d.String())
+	logger.Debugw("downloading blob")
+
+	// Try to get file from cache first


Can you explain what are we changing in the logic here ?

f, err := s.cads.Cache().GetFileReader(d.Hex()) if err != nil { if os.IsNotExist(err) || s.cads.InDownloadError(err) { if err := s.sched.Download(namespace, d); err != nil { if err == scheduler.ErrTorrentNotFound { return handler.ErrorStatus(http.StatusNotFound) } return handler.Errorf("download torrent: %s", err) } f, err = s.cads.Cache().GetFileReader(d.Hex()) if err != nil { return handler.Errorf("store: %s", err) } } else { return handler.Errorf("store: %s", err) } } if _, err := io.Copy(w, f); err != nil { return fmt.Errorf("copy file: %s", err) } return nil

The only thing which was changed in the logic is that in case of any error returned from cache blob will be downloaded, previously it was only on os.IsNotExist(err) || s.cads.InDownloadError(err). New code removes 4 ifs nesting

gkeesh7 · 2025-07-23T12:41:06Z

agent/agentserver/server.go

-	f, err := s.cads.Cache().GetFileReader(d.Hex())
+
+	// Get the downloaded file
+	reader, err = s.getCachedBlob(d)


Why are we getting the Blob from Cache twice ?

Get from cache:

If cache hit -> return

If cache miss:
2. Download

Get from cache.(Blob in the cache after the download),

gkeesh7 · 2025-07-23T13:31:02Z

agent/agentserver/server.go

+		case <-checkCtx.Done():
+		}
 	}()
-	wg.Wait()


Why aren't we waiting for all 3 checks to finish for the readiness check ?

We are waiting in select in the loop. It was possible to hang indefinitely with just waitgroup:

starting 3 goroutines

timing out on nginx

server goroutine stays in the wg.Wait

Now all the calls have timeout protection.

gkeesh7 · 2025-07-23T13:32:36Z

agent/agentserver/server.go

-	var wg sync.WaitGroup
+	results := make(chan checkResult, 3)

-	wg.Add(3)


I believe we should not modify the behaviour of readiness check if the purpose of the PR is to improve Logging in Kraken Agent.

+1, can we split into 2 PRs?

Anton-Kalpakchiev

Overall feedback - is it possible to split into smaller PRs to improve reviewability?

Anton-Kalpakchiev · 2025-07-29T15:28:05Z

agent/agentserver/server.go

+	ReadinessTimeout time.Duration `yaml:"readiness_timeout"`
+
+	// Enable detailed request logging
+	EnableRequestLogging bool `yaml:"enable_request_logging"`


How are we planning to use this flag internally? Are we gonna enable it globally or only on some agents?

Are there any performance limitations as to how many logs or what logs cardinality kraken-agent can have when deployed on Job Controller?

Anton-Kalpakchiev · 2025-07-29T15:43:17Z

agent/agentserver/server.go

-	var wg sync.WaitGroup
+	results := make(chan checkResult, 3)

-	wg.Add(3)


+1, can we split into 2 PRs?

Anton-Kalpakchiev · 2025-07-29T15:47:54Z

docker/origin/Dockerfile

   ( \
     unset http_proxy https_proxy no_proxy && \
-     apt-get update -o Acquire::Retries=3 -o Acquire::http::No-Cache=true \
+     apt-get update --allow-releaseinfo-change -o Acquire::Retries=10 -o Acquire::http::No-Cache=true -o Acquire::http::timeout=60 -o Acquire::ForceIPv4=true \


How does the retry logic work here? Is this relevant to logging? Could we please split functional changes in another PR?

hweawer added 2 commits July 11, 2025 13:32

Improve kraken agent logging

2619b1b

Remove puller

419a422

hweawer self-assigned this Jul 11, 2025

Fix errors

42cd5ca

hweawer force-pushed the improve-kraken-agent-logging branch from 159aea2 to 7247d25 Compare July 16, 2025 12:36

More robust image build process

f4bd0d9

hweawer force-pushed the improve-kraken-agent-logging branch from 7247d25 to f4bd0d9 Compare July 17, 2025 08:14

hweawer requested review from Anton-Kalpakchiev and gkeesh7 July 17, 2025 08:15

gkeesh7 reviewed Jul 22, 2025

View reviewed changes

gkeesh7 reviewed Jul 23, 2025

View reviewed changes

hweawer added 2 commits July 25, 2025 10:06

Debian 12

bc60bd2

Update timeout

27160b4

hweawer requested a review from gkeesh7 July 29, 2025 11:05

Anton-Kalpakchiev reviewed Jul 29, 2025

View reviewed changes

Improve kraken agent logging #411

Are you sure you want to change the base?

Improve kraken agent logging #411

Uh oh!

Conversation

hweawer commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkeesh7 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anton-Kalpakchiev 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hweawer commented Jul 11, 2025 •

edited

Loading