feat(api): support allowedlocation for batch worker #898
Conversation
WalkthroughThe changes involve updates to the configuration and job submission logic. A new field, Changes
Sequence Diagram(s)sequenceDiagram
participant Repo as initBatch
participant Config as Config Struct
participant BatchCfg as BatchConfig
participant BatchRepo as BatchRepo
participant GCP as GCP Service
Repo->>Config: Retrieve configuration (includes Worker_AllowedLocations)
alt If Worker_ImageURL is empty
Repo->>Repo: Return nil (exit early)
else
Repo->>Config: Validate GCPProject and GCPRegion
Repo->>Repo: Parse boot disk size & task count with error handling
Repo->>BatchCfg: Build BatchConfig (with AllowedLocations)
Repo->>BatchRepo: Initialize BatchRepo with BatchConfig
BatchRepo->>GCP: Submit job using AllowedLocations in allocation policy
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-flow canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
server/api/internal/app/repo.go (5)
107-109: Returningnilfor empty Worker_ImageURL may obscure configuration issues.
Consider at least logging a warning or clarifying the intentional skip of batch initialization so that operators or developers are aware that no batch will be set up in this scenario.if conf.Worker_ImageURL == "" { + log.Warn("initBatch: Worker_ImageURL is empty, skipping GCP Batch initialization") return nil }
111-116: Mismatch in error handling approach.
Here, missingGCPProjectorGCPRegionleads to a fatal log, but missingWorker_ImageURLmerely returnsnil. Consider unifying the approach—either consistently skip batch setup for all missing fields or fail for all critical fields.
118-121: Consider a fallback default for invalid boot disk size.
Current logic aborts the entire app if the disk size parsing fails. If you want a more graceful approach, consider reverting to a default (e.g., 50GB) instead of killing the process.
123-126: Graceful handling of invalid task count might be desirable.
Like the disk size, a non-numeric or unexpected input immediately terminates the process. A fallback or user-friendly error message might be more suitable for production environments.
141-146: Exiting onNewBatchcreation failure.
The approach is consistent with other fatal checks in this function, ensuring issues are discovered early in initialization. If a more forgiving strategy is desired, you could log and return an error instead.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/api/internal/app/config/config.go(1 hunks)server/api/internal/app/repo.go(1 hunks)server/api/internal/infrastructure/gcpbatch/batch.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Collect information
🔇 Additional comments (4)
server/api/internal/app/repo.go (1)
128-139: Good consistency in constructing theBatchConfig.
The new fieldAllowedLocationsis cleanly integrated, and typed fields for numeric values (e.g.,bootDiskSize) enhance clarity.server/api/internal/app/config/config.go (1)
72-80: Verify numeric fields stored as strings.
Storing parameters likeWorker_TaskCountas strings in the config is workable but can be prone to parse errors. You might consider using int-based fields if environment variable parsing supports it, or at least confirm each field is indeed processed correctly.server/api/internal/infrastructure/gcpbatch/batch.go (2)
21-31: Strong internal typing inBatchConfig.
UsingintforBootDiskSizeGBandTaskCountis a good improvement over string-based representations. Also, introducingAllowedLocations []stringaligns with the new location constraints.
161-163: Ensuring location policy is applied.
AssigningAllowedLocationstoAllocationPolicy_LocationPolicyeffectively takes advantage of the newly introduced configuration. If empty, GCP’s batch service will allow default allocations, which can be valid, but verify that’s intended.
Overview
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Refactor