refactor(oss): extract runtime type determination logic#1584
Conversation
k8s-ci-robot
added
size/L
k8s-ci-robot
added
the
approved
Refactor NodePublishVolume to use DetermineRuntimeType function for better code clarity. The function implements a decision tree based on directAssigned, socketPath, and skipAttach to determine runtime type (COCO, RunD, RunC, ECI). Changes: - Add DetermineRuntimeType function with decision tree implementation - Add RuntimeType enum and comprehensive unit tests - Refactor NodePublishVolume to use runtime type instead of boolean checks - Improve error handling by catching invalid combinations early Maintains 100% logic equivalence with improved readability.
AlbeeSo
force-pushed
the
oss/runtime-refactor
branch
from
7f0c2bb to
466c452
Compare
| // Returns: | ||
| // - RuntimeType: the determined runtime type | ||
| // - error: if the combination is invalid (should not occur scenarios) | ||
| func DetermineRuntimeType(directAssigned bool, socketPath string, skipAttach bool) (RuntimeType, error) { |
There was a problem hiding this comment.
I don't like this change. We don't actually care about the name of each runtime. We should have an abstract level to extract what is really different between these runtimes, and write code based on those differences. We can add comments about typical runtime, but don't depends on the runtime name in code.
There was a problem hiding this comment.
The actual difference is 'skipAttach, directAssigned amd socketPath'. I write down the meaning of them but It's still difficult to manage the code if we don't name the runtime type clearly.
There was a problem hiding this comment.
I think skipAttach should be renamed to something like agentMode. Most branches should already be self-explanatory.
The only difficult case is
// Note: In non-csi-agent environment (!ns.skipAttach),
// if DirectAssigned is True, it's a confidential container scenario (coco)
if opts.DirectAssigned && !ns.skipAttach {
return ns.publishDirectVolume(ctx, req, opts)
}
and we already have comments about it.
Particularly, socketPath == "" is a lot better than runtimeType == RuntimeTypeMicroVM, because the former clearly tells us we don't have mount proxy for this case.
Similarly, ns.agentMode (renamed from skipAttach) should be better than runtimeType == RuntimeTypeRunD || runtimeType == RuntimeTypeMicroVM. When run as agent, we don't need to care about globalmount.
There was a problem hiding this comment.
If the logic is more difficult? It's hard to manage the code. I dont want to spend minute to consider every if-else.
|
We can simplify the logic by potentially removing coco and using a sidecar instead /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlbeeSo, mowangdk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
Refactor NodePublishVolume to use DetermineRuntimeType function for better code clarity. The function implements a decision tree based on directAssigned, socketPath, and skipAttach to determine runtime type (COCO, RunD, RunC, ECI).
Changes:
Maintains 100% logic equivalence with improved readability.
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: