Skip to content

Conversation

ErikJiang
Copy link
Contributor

@ErikJiang ErikJiang commented Jun 19, 2025

Proposed Changes

This PR adds a new e2e test to verify SQLite to etcd datastore migration functionality. The test:

  1. Starts K3s with SQLite datastore
  2. Creates test resources to verify data persistence
  3. Migrates to etcd datastore
  4. Verifies all nodes, pods, and resources remain functional after migration

Types of Changes

New Feature

Linked Issues

#12507

User-Facing Change

NONE

@ErikJiang ErikJiang requested a review from a team as a code owner June 19, 2025 11:26
@ErikJiang ErikJiang marked this pull request as draft June 19, 2025 11:26
@ErikJiang ErikJiang force-pushed the add_etcdmigration_e2e branch 2 times, most recently from f105544 to 7ad28bf Compare June 20, 2025 11:24
Copy link
Member

@dereknola dereknola left a comment

Choose a reason for hiding this comment

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

Looks good, just needs some function swaps.

Comment on lines 37 to 51
// StartK3sWithSQLite starts K3s with SQLite as the datastore
func StartK3sWithSQLite(nodes []e2e.VagrantNode) error {
for _, node := range nodes {
var startCmd string
if strings.Contains(node.String(), "server") {
startCmd = "systemctl start k3s"
} else {
startCmd = "systemctl start k3s-agent"
}
if _, err := node.RunCmdOnNode(startCmd); err != nil {
return &e2e.NodeError{Node: node, Cmd: startCmd, Err: err}
}
}
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to do this, the startup test you copied this from is special. The regular create cluster functions can handle starting k3s.

See https://github.com/k3s-io/k3s/blob/master/tests/e2e/embeddedmirror/embeddedmirror_test.go#L40-L53 for a better example of what you should use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your advice.

Comment on lines 54 to 71
func MigrateToEtcd(server e2e.VagrantNode) error {
// Stop K3s service
stopCmd := "systemctl stop k3s"
if _, err := server.RunCmdOnNode(stopCmd); err != nil {
return &e2e.NodeError{Node: server, Cmd: stopCmd, Err: err}
}

// Update config to use etcd
configCmd := "echo 'cluster-init: true' >> /etc/rancher/k3s/config.yaml"
if _, err := server.RunCmdOnNode(configCmd); err != nil {
return &e2e.NodeError{Node: server, Cmd: configCmd, Err: err}
}

// Start K3s with etcd
startCmd := "systemctl start k3s"
if _, err := server.RunCmdOnNode(startCmd); err != nil {
return &e2e.NodeError{Node: server, Cmd: startCmd, Err: err}
}
Copy link
Member

Choose a reason for hiding this comment

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

Since the config.yaml is not hot loaded, you can just append to the config, then restart k3s on each node using the buildin e2e.RestartCluster. See https://github.com/k3s-io/k3s/blob/master/tests/e2e/rotateca/rotateca_test.go#L82-L88 as an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is indeed better.

}

// KillK3sCluster kills the K3s cluster
func KillK3sCluster(nodes []e2e.VagrantNode) error {
Copy link
Member

Choose a reason for hiding this comment

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

We should move this function inside the e2e testutils.go file and call that for both startup and this new test function. Probably before this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Expect(result).To(ContainSubstring("before-migration"))
})

It("Kills the cluster", func() {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: no need to kill the cluster, the next step is to cleanup and destroy everything in AfterSuite.

@brandond
Copy link
Member

Could this just go in with the rest of the startup tests?

@ErikJiang ErikJiang force-pushed the add_etcdmigration_e2e branch from 7ad28bf to 32db248 Compare June 23, 2025 06:56
@ErikJiang
Copy link
Contributor Author

ErikJiang commented Jun 23, 2025

@brandond @dereknola

I've integrated the etcd migration-related e2e tests into the startup tests.
Can I use the GitHub Actions workflow linked to this PR to verify these e2e tests?
I attempted to run the tests in WSL2, but encountered some compatibility issues.

@ErikJiang ErikJiang marked this pull request as ready for review June 23, 2025 08:44
@ErikJiang ErikJiang requested a review from dereknola June 23, 2025 08:49
Copy link

codecov bot commented Jun 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 19.91%. Comparing base (e8062bf) to head (f059a99).
Report is 5 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (e8062bf) and HEAD (f059a99). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (e8062bf) HEAD (f059a99)
e2etests 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #12528       +/-   ##
===========================================
- Coverage   40.52%   19.91%   -20.62%     
===========================================
  Files         187      184        -3     
  Lines       19330    19265       -65     
===========================================
- Hits         7834     3836     -3998     
- Misses      10307    14997     +4690     
+ Partials     1189      432      -757     
Flag Coverage Δ
e2etests ?
unittests 19.91% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ErikJiang ErikJiang force-pushed the add_etcdmigration_e2e branch 2 times, most recently from 53a5eda to 40159a9 Compare June 24, 2025 05:41
@ErikJiang ErikJiang requested a review from brandond June 24, 2025 05:47
@ErikJiang ErikJiang force-pushed the add_etcdmigration_e2e branch from 40159a9 to 9fedc75 Compare June 24, 2025 05:52
@ErikJiang ErikJiang force-pushed the add_etcdmigration_e2e branch from 9fedc75 to f059a99 Compare June 24, 2025 09:54
@ErikJiang ErikJiang requested a review from brandond June 24, 2025 09:56
Copy link
Member

@brandond brandond left a comment

Choose a reason for hiding this comment

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

thanks!

@brandond brandond merged commit be35453 into k3s-io:master Jun 24, 2025
40 checks passed
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.

3 participants