Skip to content

agent: delete unreferenced helpers and unread constants - #679

Open
Philip Lombardi (plombardi89) wants to merge 1 commit into
mainfrom
chore/deadcode-agent
Open

agent: delete unreferenced helpers and unread constants#679
Philip Lombardi (plombardi89) wants to merge 1 commit into
mainfrom
chore/deadcode-agent

Conversation

@plombardi89

Copy link
Copy Markdown
Collaborator

Part of the #670 dead-code sweep, split by component. The tooling that produced these findings is #673; this PR is deletions only and is independent of the other eight — the file sets are disjoint, so they can be reviewed and merged in any order and in parallel.

Why two analyzers were needed to find this

unused cannot see dead exported code under internal/. That is a documented design decision, not a gap: unused/unused.go:48-62 holds that a package uses its exported named types and a named type uses its exported methods, so every method on an exported type is transitively live because the type is exported.

x/tools/cmd/deadcode does not close that gap on its own either. x/tools/go/callgraph/rta/rta.go:281-287,433-437 — converting a value to an interface materializes its runtime type and marks every exported method of that type reachable, because reflection could call any of them. One var i any = x anywhere buys all of x's exported methods a clean bill of health.

So #673 adds both deadcode and hack/cmd/unreferenced, which resolves identifiers instead of tracing reachability. Neither subsumes the other, and findings below are attributed to whichever one saw them.


agent: delete unreferenced helpers and unread constants

Straight zero-reference removals:

  • attest.CleanupAttestArtifacts
  • provision.ResolveDownloadOverrides
  • utilio.CleanDir, utilio.DecompressTarGzFromRemote and the private
    downloadFromRemote it wrapped. Removing them left remoteHTTPClient with no
    reader, which unused then reported - the linter can see the cascade once
    the exported entry point is gone, which is the whole shape of the problem
    this series is about.
  • phases/host.CheckHostPackages

goalstates.ContainerImageArchivePath took an index and had no caller. It is not
the bootstrapartifacts.ContainerImageArchivePath of the same name, which takes
(arch, imageTag) and is called from offline_artifacts.go and the artifacts
builder. Only the goalstates one is removed.

agentbinary.installFromTarGz had no caller either. It is not the body of
InstallAndSwitchFromTarGz, which does its own download and extraction; the two
had diverged. Its two tests, TestInstallFromTarGzVerifiesInstalledBinary and
TestInstallFromTarGzRejectsUnsupportedScheme, were the only callers and go with
it. TestInstallAndSwitchFromTarGz still covers the live path.

Constants:

  • config.DefaultLocalDNSMetricsPort is a string "9253" duplicating
    goalstates.LocalDNSMetricsPort, which is the int the code actually uses.
  • goalstates.KubeletPKIDir had no reader and the literal
    "/etc/kubernetes/pki" appears nowhere else in the tree.

Three constants that the analyzers report are kept, with a comment saying why,
because their consumers are not Go:

  • LocalDNSCorefilePath and LocalDNSUpstreamsPath are hardcoded in
    pkg/agent/phases/rootfs/assets/localdns-supervisor.sh.
  • LocalDNSRuleComment is asserted by hack/agent/e2e-kind/e2e.py.

Neither analyzer sees across the language boundary, so "no caller" there means
"no Go caller". That is a different claim, and deleting on it would have
removed the only in-tree record of an interface the shell script depends on.

pkg/agent/daemon is deliberately untouched. NewMachinaMachineOperationReconciler
and NoopMachineOperationReconciler are test-only, but they are exported from
pkg/, not internal/, so the module is not the closed world for them that it is
for internal/.

Refs #670


Verification

go build ./..., go vet ./..., golangci-lint over the touched trees (0 issues), make fmt producing no diff, and go build -tags e2e,integrationtest,storageboundary ./.... Full suite runs in CI.

The eight deletion branches were reassembled onto the tooling commits and diffed against the original combined branch: byte-identical, so nothing was lost or duplicated in the split.

Straight zero-reference removals:

  - attest.CleanupAttestArtifacts
  - provision.ResolveDownloadOverrides
  - utilio.CleanDir, utilio.DecompressTarGzFromRemote and the private
    downloadFromRemote it wrapped. Removing them left remoteHTTPClient with no
    reader, which `unused` then reported - the linter can see the cascade once
    the exported entry point is gone, which is the whole shape of the problem
    this series is about.
  - phases/host.CheckHostPackages

goalstates.ContainerImageArchivePath took an index and had no caller. It is not
the bootstrapartifacts.ContainerImageArchivePath of the same name, which takes
(arch, imageTag) and is called from offline_artifacts.go and the artifacts
builder. Only the goalstates one is removed.

agentbinary.installFromTarGz had no caller either. It is not the body of
InstallAndSwitchFromTarGz, which does its own download and extraction; the two
had diverged. Its two tests, TestInstallFromTarGzVerifiesInstalledBinary and
TestInstallFromTarGzRejectsUnsupportedScheme, were the only callers and go with
it. TestInstallAndSwitchFromTarGz still covers the live path.

Constants:

  - config.DefaultLocalDNSMetricsPort is a string "9253" duplicating
    goalstates.LocalDNSMetricsPort, which is the int the code actually uses.
  - goalstates.KubeletPKIDir had no reader and the literal
    "/etc/kubernetes/pki" appears nowhere else in the tree.

Three constants that the analyzers report are kept, with a comment saying why,
because their consumers are not Go:

  - LocalDNSCorefilePath and LocalDNSUpstreamsPath are hardcoded in
    pkg/agent/phases/rootfs/assets/localdns-supervisor.sh.
  - LocalDNSRuleComment is asserted by hack/agent/e2e-kind/e2e.py.

Neither analyzer sees across the language boundary, so "no caller" there means
"no Go caller". That is a different claim, and deleting on it would have
removed the only in-tree record of an interface the shell script depends on.

pkg/agent/daemon is deliberately untouched. NewMachinaMachineOperationReconciler
and NoopMachineOperationReconciler are test-only, but they are exported from
pkg/, not internal/, so the module is not the closed world for them that it is
for internal/.

Refs #670

Copy link
Copy Markdown
Member

Inspected Azure/AKSFlexNode's default branch (currently d09ccdc7c4e668cd0c23660e1d75e4e6a786f958) for every exported identifier removed by this PR.

Result: no AKSFlexNode reference depends on any removed Unbounded API.

  • No references found for CleanupAttestArtifacts, CheckHostPackages, ContainerImageArchivePath, DefaultLocalDNSMetricsPort, or KubeletPKIDir.
  • The only ResolveDownloadOverrides text match is a call to the distinct, retained goalstates.ResolveDownloadOverridesWithOfflineArtifacts in pkg/config/adapter.go.
  • AKSFlexNode has its own local definitions of CleanDir and DecompressTarGzFromRemote under pkg/utils/utilio; these are not references to Unbounded's removed pkg/agent/internal/utilio functions (which external modules cannot import through Go's internal boundary).

No branch changes were made.

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.

2 participants