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

WIP: Refactor operations to not import state. #14124

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

markylaing
Copy link
Contributor

Refactors the operations package so that:

  • OperationCreate accepts a context.Context (expected to be the daemon shutdown context), the operation location, an events server, and a *operations.Opts.
  • operations.Options and operations.ClusterOptions can be used to specify optional operation create arguments.
  • state.State is no longer imported by the operations package. Instead, a transaction function is required for LXD daemon operations. This can be used by the operation to add/delete itself. The transaction function has a hook with *sql.Tx rather than db.ClusterTx so that we don't import any database files.

@markylaing markylaing self-assigned this Sep 18, 2024
@markylaing
Copy link
Contributor Author

@simondeziel @tomponline considering this PR has greatly reduced the number of imports for the lxd-agent, I'm baffled that the lxd-agent size has actually gone up (https://github.com/canonical/lxd/actions/runs/10921682830/job/30314339170?pr=14124#step:16:73)

any ideas?

@tomponline
Copy link
Member

@simondeziel @tomponline considering this PR has greatly reduced the number of imports for the lxd-agent, I'm baffled that the lxd-agent size has actually gone up (https://github.com/canonical/lxd/actions/runs/10921682830/job/30314339170?pr=14124#step:16:73)

any ideas?

do you observe the size change when compiling locally?

@tomponline
Copy link
Member

btw im loving your work on removing deps from lxd-agent

@simondeziel
Copy link
Member

simondeziel commented Sep 18, 2024

@simondeziel @tomponline considering this PR has greatly reduced the number of imports for the lxd-agent, I'm baffled that the lxd-agent size has actually gone up (https://github.com/canonical/lxd/actions/runs/10921682830/job/30314339170?pr=14124#step:16:73)

Looking at the last run in main, it was very close to hitting the max limit already, 6088 bytes headroom:

+ '[' 12576824 -gt 12582912 ']'
OK: lxd-agent is between 11 and 12MiB
+ '[' 12576824 -lt 11534336 ']'
+ echo 'OK: lxd-agent is between 11 and 12MiB'

In your PR it grew by189760 bytes (compared to main):

+ '[' 12766584 -gt 12582912 ']'
FAIL: lxd-agent binary size exceeds 12MiB

I cannot explain that increase as in both cases the go.mod and the Go version are identical. It's weird and I'd like to find the explanation but I don't think that should stop you from doing that refactor :)

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@@ -13,7 +13,6 @@ github.com/canonical/lxd/lxd/metrics
github.com/canonical/lxd/lxd/operations
github.com/canonical/lxd/lxd/request
github.com/canonical/lxd/lxd/response
github.com/canonical/lxd/lxd/state
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So when using the build tags to list dependencies, this refactor only removes a single package! I guess that explains why the binary size didn't get smaller - though it doesn't explain why it got larger.

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