Skip to content

cubemaster: fix integration tests panicking on unopened dao #1383

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.

Sign up for GitHub

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

Open
aptacc2421 wants to merge 2 commits into
base: master
Choose a base branch
from
+96 −19
Open

cubemaster: fix integration tests panicking on unopened dao #1383

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da743e6
cubemaster: fix integration tests panicking on unopened dao
aptacc2421 Aug 18, 2026
ce9de8e
cubemaster: address review nits on dao config bootstrap
aptacc2421 Aug 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 8 additions & 17 deletions CubeMaster/cmd/cubemaster/app/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/tencentcloud/CubeSandbox/CubeDB/migrate"
"github.com/tencentcloud/CubeSandbox/CubeDB/tombstone"
"github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/config"
"github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/db"
"github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/log"
"github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/recov"
"github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/cubelet/grpcconn"
Expand Down Expand Up @@ -235,23 +236,13 @@ func initDatabaseSchema(ctx context.Context, cfg *config.Config) error {
// covering the host/node inventory tables (t_cube_host_*, t_cube_node_*)
// and the instance tables (t_cube_template_*, t_cube_instance_*,
// t_cube_sandbox_spec, ...), all in the one configured database.
src := cfg.InstanceDBConfig
if src == nil {
return fmt.Errorf("dao: instance_db_config is not set")
}
daoCfg := dao.Config{
Driver: src.Driver,
Addr: src.Addr,
User: src.User,
Pwd: src.Pwd,
DBName: src.DBName,
ConnTimeoutSeconds: src.ConnTimeout,
ReadTimeoutSeconds: src.ReadTimeout,
WriteTimeoutSeconds: src.WriteTimeout,
MaxIdleConns: src.MaxIdleConns,
MaxOpenConns: src.MaxOpenConns,
MaxConnLifeTimeSeconds: src.MaxConnLifeTimeSeconds,
MigrationLockTimeoutSeconds: src.MigrationLockTimeoutSeconds,
// Build the dao config from the same snapshot as the integration /
// mock-debug bootstrap: config hotswap could otherwise change the
// identity between the two dao.Open calls and the second one would
// fail with "dao: already opened with ... (requested ...)".
daoCfg, err := db.ConfigFromDBConfig(cfg.InstanceDBConfig)

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor precision nit on the comment: the two call sites don't actually read the same snapshot. mock_db() reads the live global via config.GetDbConfig(), while this reads cfg.InstanceDBConfig from the cfg captured at Run() start. What ConfigFromDBConfig guarantees is the same mapping, not the same snapshot — if a config hot-reload landed in the window between MockInit() and app.Run(), the two identities would still differ and this second dao.Open would fail with already opened with ... (requested ...). The comment in mock_init.go already acknowledges that window honestly; consider rewording to "same mapping as" so the claim here doesn't overstate the protection.

if err != nil {
return fmt.Errorf("dao: %w", err)

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Non-blocking: this refactor drops the actionable error text for a missing DB config. The old path reported dao: instance_db_config is not set — it names the exact yaml key an operator must add. The new path surfaces db config is nil, and because coreInit wraps initDatabaseSchema errors as dao migrate: %w, the final log reads dao migrate: dao: db config is nil — "dao" appears twice and the config key is gone.

No caller matches on the old string, and in the app this path is unreachable with a nil config because coreInit guards cfg.InstanceDBConfig == nil first. But mock-debug mode reaches ConfigFromDBConfig with whatever config.GetDbConfig() returns, so the message matters there. Keeping the field name in ConfigFromDBConfig's error (e.g. instance_db_config is not set / db config is nil) would preserve operator diagnostics.

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor diagnostic regression: the removed fmt.Errorf("dao: instance_db_config is not set") named the exact YAML key an operator must set, while this now surfaces the vaguer "dao: db config is nil" from ConfigFromDBConfig(nil). Consider preserving the key-specific wording — e.g., have ConfigFromDBConfig return an error that mentions instance_db_config, or keep a src == nil guard here so the message stays actionable.

}
if _, err := dao.Open(ctx, daoCfg); err != nil {
return fmt.Errorf("dao open: %w", err)
Expand Down
21 changes: 19 additions & 2 deletions CubeMaster/integration/mock_init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/alicebob/miniredis/v2"
"github.com/gomodule/redigo/redis"
"github.com/google/uuid"
"github.com/tencentcloud/CubeSandbox/CubeDB/dao"
"github.com/tencentcloud/CubeSandbox/CubeMaster/api/services/cubebox/v1"
cubeleterrorcode "github.com/tencentcloud/CubeSandbox/CubeMaster/api/services/errorcode/v1"
"github.com/tencentcloud/CubeSandbox/CubeMaster/api/services/images/v1"
Expand Down Expand Up @@ -654,10 +655,26 @@ func metricNow() []byte {
}

func mock_db() {
mocktest_OssDb = db.Init(config.GetDbConfig())
// db.Init is a thin wrapper that returns dao.Default(), which panics
// with "dao: Open has not been called yet" until the shared handle is
// established by dao.Open. Open it here from the loaded DB config;
// dao.Open is idempotent for the same identity, so the later
// dao.Open in app.Run's startup path (initDatabaseSchema) is a no-op.
// Schema (including t_cube_host_type) is owned by the dao.Migrate
// path that the integration test bootstrap runs before tests.
// path that the app bootstrap runs before tests. The dao config must
// be built from the same snapshot as initDatabaseSchema: config
// hotswap between MockInit and app.Run could change the identity and
// make the second dao.Open fail with "dao: already opened with ...".

@fslongjin fslongjin Aug 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the contribution! The overall behavioral change looks good to me. However, I think we can clean up the comments here a bit. We should focus on describing what the code does, rather than explaining the rationale behind this panic. Otherwise, it might confuse future readers and comes across as noise.

@aptacc2421 aptacc2421 Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review! Already changed.

daoCfg, err := db.ConfigFromDBConfig(config.GetDbConfig())
if err != nil {
stdlog.Fatalf("integration: dao config fail: %v", err)

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Non-blocking robustness note: this fix rests on dao.Open being idempotent for this config identity, and dao.Open keys identity on the full config — including Pwd, Addr, and every timeout/pool field (identityFromConfig in CubeDB/dao/dao.go). Both call sites read the same cfg.InstanceDBConfig global today, so they agree. But config.Init() starts a hotswap watcher (hotswap/file.go) that atomically swaps cfg on a config-file write; a write landing in the window between MockInit() and initDatabaseSchema would make the app's second dao.Open fail startup with dao: already opened with .... The window is tiny and unlikely in practice, but a one-line comment here (and in initDatabaseSchema) noting that the two opens must be built from a single config snapshot would make the coupling explicit and cheaper to debug.

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

mocktest_Ctx is the mock-lifecycle context, cancelled at the end of TestMain. Opening the process-lifetime shared dao handle with it semantically ties the global handle to the mock's teardown; it's benign today because both drivers ignore ctx in Open, but context.Background() would express the handle's actual lifetime more accurately.

}
if _, err := dao.Open(mocktest_Ctx, daoCfg); err != nil {
stdlog.Fatalf("dao open fail:%v", err)
}

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The fix is correct for the two known bootstrap paths, but it leaves db.Init's contract fragile: Init still ignores its cfg argument and dao.Default() panics with ErrNotOpened if any future bootstrap path calls Init before dao.Open. The pre-condition is documented on Init, but nothing enforces it, so a third pre-dao.Open caller (a new integration harness, a test helper) would hit the same panic this PR fixes. Worth considering whether Init should fall back to opening the dao from the cfg it currently ignores when the handle is unopened — that would close the whole class of init-order bugs rather than this one site (balanced against the existing "fail loudly" intent).

mocktest_OssDb = db.Init(config.GetDbConfig())
}

func mock_getstr() string {
return fmt.Sprintf("%d.%d.%d.%d", rand.Int31n(254), rand.Int31n(254), rand.Int31n(254), rand.Int31n(254))
}
Expand Down
27 changes: 27 additions & 0 deletions CubeMaster/pkg/base/db/db.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
package db

import (
"errors"

"github.com/tencentcloud/CubeSandbox/CubeDB/dao"
"github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/base/config"
"gorm.io/gorm"
Expand All @@ -20,3 +22,28 @@ func Init(cfg *config.DBConfig) *gorm.DB {
_ = cfg
return dao.Default()
}

// ConfigFromDBConfig maps a config.DBConfig to the dao.Config used to open
// the shared database handle. dao.Open keys its idempotence on this config
// identity, so every call site that opens the handle before app startup
// (schema init, integration/mock-debug bootstrap) must build the dao config
// through this helper — keeping the two mappings from drifting.
func ConfigFromDBConfig(src *config.DBConfig) (dao.Config, error) {

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Trivial: the helper is generic over *config.DBConfig, but the nil error message hardcodes the instance-DB field name (instance_db_config is not set), and the wrap in initDatabaseSchema doubles the prefix (dao: db config is nil: instance_db_config is not set). A neutral message such as db config is nil would keep the helper accurate if it is ever reused for another DB config. Note this also changes the operator-visible error text from the previous dao: instance_db_config is not set.

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This helper is a second copy of the field list that dao.Config / identityFromConfig already owns. If dao.Config ever gains a field (e.g. the existing Extra), this mapping will silently omit it and the two bootstrap dao.Open identities could drift — the exact problem this PR sets out to prevent. Consider a round-trip test (config.DBConfig → dao.Config → identity) or moving the mapping next to dao.Config so the field list lives in one place.

if src == nil {
return dao.Config{}, errors.New("db config is nil: instance_db_config is not set")
}
return dao.Config{
Driver: src.Driver,
Addr: src.Addr,
User: src.User,
Pwd: src.Pwd,
DBName: src.DBName,
ConnTimeoutSeconds: src.ConnTimeout,
ReadTimeoutSeconds: src.ReadTimeout,
WriteTimeoutSeconds: src.WriteTimeout,
MaxIdleConns: src.MaxIdleConns,
MaxOpenConns: src.MaxOpenConns,
MaxConnLifeTimeSeconds: src.MaxConnLifeTimeSeconds,
MigrationLockTimeoutSeconds: src.MigrationLockTimeoutSeconds,
}, nil
}
43 changes: 43 additions & 0 deletions CubeMaster/pkg/base/db/db_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package db_test

import (
"context"
"reflect"
"testing"
"time"

Expand Down Expand Up @@ -38,3 +39,45 @@ func TestInitReturnsDaoDefaultOnPostgreSQL(t *testing.T) {
t.Fatal("db.Init must return the global dao handle opened by dao.Open")
}
}

func TestConfigFromDBConfig(t *testing.T) {
if _, err := db.ConfigFromDBConfig(nil); err == nil {
t.Fatal("ConfigFromDBConfig(nil) must return an error")
}

src := &config.DBConfig{
Driver: "postgres",
Addr: "127.0.0.1:5432",
User: "cube",
Pwd: "cube_pass",
DBName: "cube_test",
ConnTimeout: 1,
ReadTimeout: 2,
WriteTimeout: 3,
MaxIdleConns: 4,
MaxOpenConns: 5,
MaxConnLifeTimeSeconds: 6,
MigrationLockTimeoutSeconds: 7,
}
got, err := db.ConfigFromDBConfig(src)
if err != nil {
t.Fatalf("ConfigFromDBConfig: %v", err)
}
want := dao.Config{
Driver: "postgres",
Addr: "127.0.0.1:5432",
User: "cube",
Pwd: "cube_pass",
DBName: "cube_test",
ConnTimeoutSeconds: 1,
ReadTimeoutSeconds: 2,
WriteTimeoutSeconds: 3,
MaxIdleConns: 4,
MaxOpenConns: 5,
MaxConnLifeTimeSeconds: 6,
MigrationLockTimeoutSeconds: 7,
}
if !reflect.DeepEqual(got, want) {
t.Fatalf("ConfigFromDBConfig mismatch:\n got: %+v\nwant: %+v", got, want)
}
}
Loading